[Lguest] [lguest] Reboot Implemented

[Rusty Russell]

On Tuesday 18 December 2007 07:23:19 Balaji Rao wrote:
> Hi Rusty,

Hi Balaji!

> As you have told, i have implemented reboot functionality for the guest.

Cool!

> I am not confident about the patch, so didn't send it to the list! Please
> review and tell me my mistakes. I will resend it after corrections.

That was a mistake, because this is a good patch!  I've fixed your mistake by 
cc'ing the list on my reply :)

> I have not renamed the CRASH hypercall, as i could not findout a good
> replacement!

LGUEST_SHUTDOWN?

Because this is your first patch to lguest, I'll go through every change and 
be really picky.  You don't need to agree with all my suggestions, but 
they're more a "this is how I would have done it".

> ===================================================================
> --- linux.orig/arch/x86/lguest/boot.c
> +++ linux/arch/x86/lguest/boot.c
> @@ -67,7 +67,7 @@
>  #include <asm/mce.h>
>  #include <asm/io.h>
>  #include <asm/i387.h>
> -
> +#include <asm/reboot.h>

Hmm, it's not obvious to me why this header is needed.  Perhaps we should use 
the normal reboot syscall magic numbers?

> @@ -1059,6 +1064,7 @@ __init void lguest_init(void)
>  	 * the Guest routine to power off. */
>  	pm_power_off = lguest_power_off;
>
> +	machine_ops.emergency_restart = lguest_restart;

A comment here would be nice: something like
	/* This is the badly named "reboot" hook; it's not just for
	 * emergencies. */

> ===================================================================
> --- linux.orig/Documentation/lguest/lguest.c
> +++ linux/Documentation/lguest/lguest.c
> @@ -153,6 +153,11 @@ struct virtqueue
>  	void (*handle_output)(int fd, struct virtqueue *me);
>  };
>
> +/* We need to store the arguements passed to us, as we need to do an exec
> + * when the guest needs to be restarted.
> + */
> +char **main_args;
> +

I try to keep the line count to a minimum, so I don't usually put */ on a line 
by itself.  Also, this variable should be static.

>  /* Since guest is UP and we don't run at the same time, we don't need
> barriers.
>   * But I include them in the code in case others copy it. */
>  #define wmb()
> @@ -1489,7 +1494,7 @@ static void setup_block_file(const char
>
>  	/* Create stack for thread and run it */
>  	stack = malloc(32768);
> -	if (clone(io_thread, stack + 32768, CLONE_VM, dev) == -1)
> +	if ((children[0] = clone(io_thread, stack + 32768,  CLONE_VM | SIGCHLD,
> dev) == -1))
>  		err(1, "Creating clone");
>

Hmm, where's children[] defined?  I think this is unused?

>  	/* We don't need to keep the I/O thread's end of the pipes open. */
> @@ -1505,6 +1510,7 @@ static void setup_block_file(const char
>   * its input and output, and finally, lays it to rest. */
>  static void __attribute__((noreturn)) run_guest(int lguest_fd)
>  {
> +	int i;
>  	for (;;) {
>  		unsigned long args[] = { LHREQ_BREAK, 0 };
>  		unsigned long notify_addr;
> @@ -1525,6 +1531,13 @@ static void __attribute__((noreturn)) ru
>  			errx(1, "%s", reason);
>  		/* EAGAIN means the Waker wanted us to look at some input.
>  		 * Anything else means a bug or incompatible change. */
> +		} else if (errno == ERESTART) {
> +			/* close all open fd(s), so that we cause our two threads to die */
> +				for (i=3;i<15;i++) {
> +					close (i);
> +				}
> +				execv(main_args[0],main_args);
> +				return 1;
>  		} else if (errno != EAGAIN)
>  			err(1, "Running guest failed");

OK, the comment is now wrong (it should mention ERESTART).  It'd also be nice 
to move this closing code out to a separate function (reboot_guest()?); I 
think it should loop through the struct device chain to see what file 
descriptors to close.  Finally, it should use err() if execv fails.

> @@ -1561,6 +1574,9 @@ static void usage(void)
>  /*L:105 The main routine is where the real work begins: */
>  int main(int argc, char *argv[])
>  {
> +	main_args = argv;
> +	/* Dont care about the status of children */
> +	signal(SIGCHLD, SIG_IGN);
>  	/* Memory, top-level pagetable, code startpoint and size of the
>  	 * (optional) initrd. */
>  	unsigned long mem = 0, pgdir, start, initrd_size = 0;

Those should be moved below the declarations, and main_args deserves a comment 
like /* Remember the original arguments so we can "reboot" */.  And it'd be 
nicer to give a little more explanation about ignoring SIGCHLD (that it means 
our children don't become zombies so we don't have to wait() for them).  Or 
we could wait for them on reboot maybe.

> Index: linux/drivers/lguest/hypercalls.c
> ===================================================================
> --- linux.orig/drivers/lguest/hypercalls.c
> +++ linux/drivers/lguest/hypercalls.c
> @@ -22,6 +22,7 @@
>  */
>  #include <linux/uaccess.h>
>  #include <linux/syscalls.h>
> +#include <linux/lguest.h>
>  #include <linux/mm.h>
>  #include <asm/page.h>
>  #include <asm/pgtable.h>
> @@ -50,6 +51,8 @@ static void do_hcall(struct lguest *lg,
>  		__lgread(lg, msg, args->arg1, sizeof(msg));
>  		msg[sizeof(msg)-1] = '\0';
>  		kill_guest(lg, "CRASH: %s", msg);
> +		if ((int)(args->arg2) == LGUEST_RESTART)
> +			lg->dead = -ERESTART;
>  		break;

The (int) cast here seems strange, and I think that LGUEST_RESTART etc should 
be called LHCALL_SHUTDOWN_RESTART and put in asm-x86/lguest_hcall.h.

>  	}
>  	case LHCALL_FLUSH_TLB:
> Index: linux/drivers/lguest/core.c
> ===================================================================
> --- linux.orig/drivers/lguest/core.c
> +++ linux/drivers/lguest/core.c
> @@ -236,6 +236,7 @@ int run_guest(struct lguest *lg, unsigne
>  	}
>
>  	/* The Guest is dead => "No such file or directory" */
> +	if (lg->dead == -ERESTART) return -ERESTART;
>  	return -ENOENT;
>  }

The comment needs updating, and the if statement should take two lines:
	if (lg->dead == -ERESTART)
		return -ERESTART;
	return -ENOENT;

Thanks for the patch!
Rusty.