[PATCH 0 of 3] xl and hotplug: Introduce and use shutdown and reboot xm compatibility options

[PATCH 0 of 3] xl and hotplug: Introduce and use shutdown and reboot xm compatibility options

[Sander Eikelenboom]

The /etc/init.d/xendomains script makes use of options for the shutdown command defined in xendomains-sysconfig/default.
These options were not implemented in xl, this patch series implements the options and by using the short variant makes it compatible with both xm and xl.

Signed-off-by: Sander Eikelenboom <linux@eikelenboom.it>

[PATCH 1 of 3] xl: Introduce shutdown xm compatibility option -a

[Sander Eikelenboom]

  * Add missing option -a to shutdown all guest domains

Signed-off-by: Sander Eikelenboom <linux@eikelenboom.it>


---

Changed since v2:
  * fix error occuring when using both -a and -w options
    * Due to mixing local and global domid variable


Changed since v1:
  * address review comments.
    * Change shutdown_domain to take domid instead of domname
    * Docs: Make it more clear -a only shuts down GUEST domains

diff -r 9dc729b75595 -r 4c3d49787cea docs/man/xl.pod.1
--- a/docs/man/xl.pod.1	Mon Sep 03 11:22:02 2012 +0100
+++ b/docs/man/xl.pod.1	Thu Sep 06 21:36:14 2012 +0200
@@ -527,7 +527,7 @@ List specifically for that domain. Other
 
 =back
 
-=item B<shutdown> [I<OPTIONS>] I<domain-id>
+=item B<shutdown> [I<OPTIONS>] I<-a|domain-id>
 
 Gracefully shuts down a domain.  This coordinates with the domain OS
 to perform graceful shutdown, so there is no guarantee that it will
@@ -550,6 +550,10 @@ B<OPTIONS>
 
 =over 4
 
+=item B<-a>
+
+-a  Shutdown all guest domains.  Often used when doing a complete shutdown of a Xen system.
+
 =item B<-w>
 
 Wait for the domain to complete shutdown before returning.
diff -r 9dc729b75595 -r 4c3d49787cea tools/libxl/xl_cmdimpl.c
--- a/tools/libxl/xl_cmdimpl.c	Mon Sep 03 11:22:02 2012 +0100
+++ b/tools/libxl/xl_cmdimpl.c	Thu Sep 06 21:36:14 2012 +0200
@@ -2683,13 +2683,14 @@ static void destroy_domain(const char *p
     if (rc) { fprintf(stderr,"destroy failed (rc=%d)\n",rc); exit(-1); }
 }
 
-static void shutdown_domain(const char *p, int wait, int fallback_trigger)
+static void shutdown_domain(uint32_t domain_id, int wait, int fallback_trigger)
 {
     int rc;
     libxl_event *event;
 
-    find_domain(p);
-    rc=libxl_domain_shutdown(ctx, domid);
+    domid = domain_id;
+    rc = libxl_domain_shutdown(ctx, domid);
+
     if (rc == ERROR_NOPARAVIRT) {
         if (fallback_trigger) {
             fprintf(stderr, "PV control interface not available:"
@@ -3670,14 +3671,19 @@ int main_destroy(int argc, char **argv)
 
 int main_shutdown(int argc, char **argv)
 {
-    int opt;
+    libxl_dominfo *dominfo;
+    int opt, i, nb_domain;
+    int all = 0;
     int wait = 0;
     int fallback_trigger = 0;
 
-    while ((opt = def_getopt(argc, argv, "wF", "shutdown", 1)) != -1) {
+    while ((opt = def_getopt(argc, argv, "awF", "shutdown", 0)) != -1) {
         switch (opt) {
         case 0: case 2:
             return opt;
+        case 'a':
+            all = 1;
+            break;
         case 'w':
             wait = 1;
             break;
@@ -3687,7 +3693,30 @@ int main_shutdown(int argc, char **argv)
         }
     }
 
-    shutdown_domain(argv[optind], wait, fallback_trigger);
+    if (!argv[optind] && !all) {
+        fprintf(stderr, "You must specify -a or a domain id.\n\n");
+        return opt;
+    }
+
+    if (all) {
+        if (!(dominfo = libxl_list_domain(ctx, &nb_domain))) {
+            fprintf(stderr, "libxl_list_domain failed.\n");
+            return -1;
+        }
+
+        for (i = 0; i<nb_domain; i++) {
+            if (dominfo[i].domid == 0)
+                continue;
+
+            shutdown_domain(dominfo[i].domid, wait, fallback_trigger);
+        }
+
+        libxl_dominfo_list_free(dominfo, nb_domain);
+    } else {
+        find_domain(argv[optind]);
+        shutdown_domain(domid, wait, fallback_trigger);
+    }
+
     return 0;
 }
 
diff -r 9dc729b75595 -r 4c3d49787cea tools/libxl/xl_cmdtable.c
--- a/tools/libxl/xl_cmdtable.c	Mon Sep 03 11:22:02 2012 +0100
+++ b/tools/libxl/xl_cmdtable.c	Thu Sep 06 21:36:14 2012 +0200
@@ -60,7 +60,8 @@ struct cmd_spec cmd_table[] = {
     { "shutdown",
       &main_shutdown, 0, 1,
       "Issue a shutdown signal to a domain",
-      "[options] <Domain>",
+      "[options] <-a|Domain>",
+      "-a                      Shutdown all guest domains.\n"
       "-h                      Print this help.\n"
       "-F                      Fallback to ACPI power event for HVM guests with\n"
       "                        no PV drivers.\n"

Re: [PATCH 1 of 3] xl: Introduce shutdown xm compatibility option -a

[Ian Jackson]

Sander Eikelenboom writes ("[Xen-devel] [PATCH 1 of 3] xl: Introduce shutdown xm compatibility option -a"):
>   * Add missing option -a to shutdown all guest domains

> +        for (i = 0; i<nb_domain; i++) {
> +            if (dominfo[i].domid == 0)
> +                continue;
> +
> +            shutdown_domain(dominfo[i].domid, wait, fallback_trigger);
> +        }

This isn't quite right, I think.  Surely it should initiate the
shutdown for all the domains right away, and then wait for them all to
finish ?

That's going to make the patch more complicated of course...

Ian.

[PATCH 2 of 3] xl: Introduce reboot xm compatibility option -a and -w

[Sander Eikelenboom]

  * Add missing option -a to reboot all guest domains
  * Add missing option -w to wait for the domain to reboot before returning


Signed-off-by: Sander Eikelenboom <linux@eikelenboom.it>

diff -r 4c3d49787cea -r 780eae92908a docs/man/xl.pod.1
--- a/docs/man/xl.pod.1	Thu Sep 06 21:36:14 2012 +0200
+++ b/docs/man/xl.pod.1	Thu Sep 06 21:36:41 2012 +0200
@@ -432,7 +432,7 @@ Pause a domain.  When in a paused state 
 allocated resources such as memory, but will not be eligible for
 scheduling by the Xen hypervisor.
 
-=item B<reboot> [I<OPTIONS>] I<domain-id>
+=item B<reboot> [I<OPTIONS>] I<-a|domain-id>
 
 Reboot a domain.  This acts just as if the domain had the B<reboot>
 command run from the console.  The command returns as soon as it has
@@ -452,6 +452,12 @@ B<OPTIONS>
 
 =over 4
 
+-a  Shutdown all guest domains.  Often used when doing a complete shutdown of a Xen system.
+
+=item B<-w>
+
+Wait for the domain to complete shutdown before returning.
+
 =item B<-F>
 
 If the guest does not support PV reboot control then fallback to
diff -r 4c3d49787cea -r 780eae92908a tools/libxl/xl_cmdimpl.c
--- a/tools/libxl/xl_cmdimpl.c	Thu Sep 06 21:36:14 2012 +0200
+++ b/tools/libxl/xl_cmdimpl.c	Thu Sep 06 21:36:41 2012 +0200
@@ -2743,11 +2743,14 @@ static void shutdown_domain(uint32_t dom
     }
 }
 
-static void reboot_domain(const char *p, int fallback_trigger)
+static void reboot_domain(uint32_t domain_id, int wait, int fallback_trigger)
 {
     int rc;
-    find_domain(p);
-    rc=libxl_domain_reboot(ctx, domid);
+    libxl_event *event;
+
+    domid = domain_id;
+    rc = libxl_domain_reboot(ctx, domid);
+    
     if (rc == ERROR_NOPARAVIRT) {
         if (fallback_trigger) {
             fprintf(stderr, "PV control interface not available:"
@@ -2762,6 +2765,42 @@ static void reboot_domain(const char *p,
     if (rc) {
         fprintf(stderr,"reboot failed (rc=%d)\n",rc);exit(-1);
     }
+
+    if (wait) {
+        libxl_evgen_domain_death *deathw;
+
+        rc = libxl_evenable_domain_death(ctx, domid, 0, &deathw);
+        if (rc) {
+            fprintf(stderr,"wait for death failed (evgen, rc=%d)\n",rc);
+            exit(-1);
+        }
+
+        for (;;) {
+            rc = domain_wait_event(&event);
+            if (rc) exit(-1);
+
+            switch (event->type) {
+
+            case LIBXL_EVENT_TYPE_DOMAIN_DEATH:
+                LOG("Domain %d has been destroyed", domid);
+                goto done;
+
+            case LIBXL_EVENT_TYPE_DOMAIN_SHUTDOWN:
+                LOG("Domain %d has been shut down, reason code %d %x", domid,
+                    event->u.domain_shutdown.shutdown_reason,
+                    event->u.domain_shutdown.shutdown_reason);
+                goto done;
+
+            default:
+                LOG("Unexpected event type %d", event->type);
+                break;
+            }
+            libxl_event_free(ctx, event);
+        }
+    done:
+        libxl_event_free(ctx, event);
+        libxl_evdisable_domain_death(ctx, deathw);
+    }
 }
 
 static void list_domains_details(const libxl_dominfo *info, int nb_domain)
@@ -3722,20 +3761,52 @@ int main_shutdown(int argc, char **argv)
 
 int main_reboot(int argc, char **argv)
 {
-    int opt;
+    libxl_dominfo *dominfo;
+    int opt, i, nb_domain;
+    int all = 0;
+    int wait = 0;
     int fallback_trigger = 0;
 
-    while ((opt = def_getopt(argc, argv, "F", "reboot", 1)) != -1) {
+    while ((opt = def_getopt(argc, argv, "awF", "reboot", 0)) != -1) {
         switch (opt) {
         case 0: case 2:
             return opt;
+        case 'a':
+            all = 1;
+            break;
+        case 'w':
+            wait = 1;
+            break;
         case 'F':
             fallback_trigger = 1;
             break;
         }
     }
 
-    reboot_domain(argv[optind], fallback_trigger);
+    if (!argv[optind] && !all) {
+        fprintf(stderr, "You must specify -a or a domain id.\n\n");
+        return opt;
+    }
+
+    if (all) {
+        if (!(dominfo = libxl_list_domain(ctx, &nb_domain))) {
+            fprintf(stderr, "libxl_list_domain failed.\n");
+            return -1;
+        }
+
+        for (i = 0; i<nb_domain; i++) {
+            if (dominfo[i].domid == 0)
+                continue;
+
+            reboot_domain(dominfo[i].domid, wait, fallback_trigger);
+        }
+
+        libxl_dominfo_list_free(dominfo, nb_domain);
+    } else {
+        find_domain(argv[optind]);
+        reboot_domain(domid, wait, fallback_trigger);
+    }
+
     return 0;
 }
 
diff -r 4c3d49787cea -r 780eae92908a tools/libxl/xl_cmdtable.c
--- a/tools/libxl/xl_cmdtable.c	Thu Sep 06 21:36:14 2012 +0200
+++ b/tools/libxl/xl_cmdtable.c	Thu Sep 06 21:36:41 2012 +0200
@@ -70,10 +70,12 @@ struct cmd_spec cmd_table[] = {
     { "reboot",
       &main_reboot, 0, 1,
       "Issue a reboot signal to a domain",
-      "[options] <Domain>",
+      "[options] <-a|Domain>",
+      "-a                      Reboot all guest domains.\n"
       "-h                      Print this help.\n"
       "-F                      Fallback to ACPI reset event for HVM guests with\n"
       "                        no PV drivers.\n"
+      "-w                      Wait for guest to reboot.\n"
     },
     { "pci-attach",
       &main_pciattach, 0, 1,

Re: [PATCH 2 of 3] xl: Introduce reboot xm compatibility option -a and -w

[Ian Campbell]

On Thu, 2012-09-06 at 20:41 +0100, Sander Eikelenboom wrote:
> * Add missing option -a to reboot all guest domains
>   * Add missing option -w to wait for the domain to reboot before returning

If, as I suspect, the code to do this waiting is a cut-and-paste of the
same code in the shutdown case please can you refactor it into
domain_wait_shutdown and call it from both places.

> 
> 
> Signed-off-by: Sander Eikelenboom <linux@eikelenboom.it>
> 
> diff -r 4c3d49787cea -r 780eae92908a docs/man/xl.pod.1
> --- a/docs/man/xl.pod.1	Thu Sep 06 21:36:14 2012 +0200
> +++ b/docs/man/xl.pod.1	Thu Sep 06 21:36:41 2012 +0200
> @@ -432,7 +432,7 @@ Pause a domain.  When in a paused state 
>  allocated resources such as memory, but will not be eligible for
>  scheduling by the Xen hypervisor.
>  
> -=item B<reboot> [I<OPTIONS>] I<domain-id>
> +=item B<reboot> [I<OPTIONS>] I<-a|domain-id>
>  
>  Reboot a domain.  This acts just as if the domain had the B<reboot>
>  command run from the console.  The command returns as soon as it has
> @@ -452,6 +452,12 @@ B<OPTIONS>
>  
>  =over 4
>  
> +-a  Shutdown all guest domains.  Often used when doing a complete shutdown of a Xen system.

Please can you wrap to < 80 columns. (and in patch 1/3 and your commit
messages too)

Thanks,
Ian.

> +
> +=item B<-w>
> +
> +Wait for the domain to complete shutdown before returning.
> +
>  =item B<-F>
>  
>  If the guest does not support PV reboot control then fallback to
> diff -r 4c3d49787cea -r 780eae92908a tools/libxl/xl_cmdimpl.c
> --- a/tools/libxl/xl_cmdimpl.c	Thu Sep 06 21:36:14 2012 +0200
> +++ b/tools/libxl/xl_cmdimpl.c	Thu Sep 06 21:36:41 2012 +0200
> @@ -2743,11 +2743,14 @@ static void shutdown_domain(uint32_t dom
>      }
>  }
>  
> -static void reboot_domain(const char *p, int fallback_trigger)
> +static void reboot_domain(uint32_t domain_id, int wait, int fallback_trigger)
>  {
>      int rc;
> -    find_domain(p);
> -    rc=libxl_domain_reboot(ctx, domid);
> +    libxl_event *event;
> +
> +    domid = domain_id;
> +    rc = libxl_domain_reboot(ctx, domid);
> +    
>      if (rc == ERROR_NOPARAVIRT) {
>          if (fallback_trigger) {
>              fprintf(stderr, "PV control interface not available:"
> @@ -2762,6 +2765,42 @@ static void reboot_domain(const char *p,
>      if (rc) {
>          fprintf(stderr,"reboot failed (rc=%d)\n",rc);exit(-1);
>      }
> +
> +    if (wait) {
> +        libxl_evgen_domain_death *deathw;
> +
> +        rc = libxl_evenable_domain_death(ctx, domid, 0, &deathw);
> +        if (rc) {
> +            fprintf(stderr,"wait for death failed (evgen, rc=%d)\n",rc);
> +            exit(-1);
> +        }
> +
> +        for (;;) {
> +            rc = domain_wait_event(&event);
> +            if (rc) exit(-1);
> +
> +            switch (event->type) {
> +
> +            case LIBXL_EVENT_TYPE_DOMAIN_DEATH:
> +                LOG("Domain %d has been destroyed", domid);
> +                goto done;
> +
> +            case LIBXL_EVENT_TYPE_DOMAIN_SHUTDOWN:
> +                LOG("Domain %d has been shut down, reason code %d %x", domid,
> +                    event->u.domain_shutdown.shutdown_reason,
> +                    event->u.domain_shutdown.shutdown_reason);
> +                goto done;
> +
> +            default:
> +                LOG("Unexpected event type %d", event->type);
> +                break;
> +            }
> +            libxl_event_free(ctx, event);
> +        }
> +    done:
> +        libxl_event_free(ctx, event);
> +        libxl_evdisable_domain_death(ctx, deathw);
> +    }
>  }
>  
>  static void list_domains_details(const libxl_dominfo *info, int nb_domain)
> @@ -3722,20 +3761,52 @@ int main_shutdown(int argc, char **argv)
>  
>  int main_reboot(int argc, char **argv)
>  {
> -    int opt;
> +    libxl_dominfo *dominfo;
> +    int opt, i, nb_domain;
> +    int all = 0;
> +    int wait = 0;
>      int fallback_trigger = 0;
>  
> -    while ((opt = def_getopt(argc, argv, "F", "reboot", 1)) != -1) {
> +    while ((opt = def_getopt(argc, argv, "awF", "reboot", 0)) != -1) {
>          switch (opt) {
>          case 0: case 2:
>              return opt;
> +        case 'a':
> +            all = 1;
> +            break;
> +        case 'w':
> +            wait = 1;
> +            break;
>          case 'F':
>              fallback_trigger = 1;
>              break;
>          }
>      }
>  
> -    reboot_domain(argv[optind], fallback_trigger);
> +    if (!argv[optind] && !all) {
> +        fprintf(stderr, "You must specify -a or a domain id.\n\n");
> +        return opt;
> +    }
> +
> +    if (all) {
> +        if (!(dominfo = libxl_list_domain(ctx, &nb_domain))) {
> +            fprintf(stderr, "libxl_list_domain failed.\n");
> +            return -1;
> +        }
> +
> +        for (i = 0; i<nb_domain; i++) {
> +            if (dominfo[i].domid == 0)
> +                continue;
> +
> +            reboot_domain(dominfo[i].domid, wait, fallback_trigger);
> +        }
> +
> +        libxl_dominfo_list_free(dominfo, nb_domain);
> +    } else {
> +        find_domain(argv[optind]);
> +        reboot_domain(domid, wait, fallback_trigger);
> +    }
> +
>      return 0;
>  }
>  
> diff -r 4c3d49787cea -r 780eae92908a tools/libxl/xl_cmdtable.c
> --- a/tools/libxl/xl_cmdtable.c	Thu Sep 06 21:36:14 2012 +0200
> +++ b/tools/libxl/xl_cmdtable.c	Thu Sep 06 21:36:41 2012 +0200
> @@ -70,10 +70,12 @@ struct cmd_spec cmd_table[] = {
>      { "reboot",
>        &main_reboot, 0, 1,
>        "Issue a reboot signal to a domain",
> -      "[options] <Domain>",
> +      "[options] <-a|Domain>",
> +      "-a                      Reboot all guest domains.\n"
>        "-h                      Print this help.\n"
>        "-F                      Fallback to ACPI reset event for HVM guests with\n"
>        "                        no PV drivers.\n"
> +      "-w                      Wait for guest to reboot.\n"
>      },
>      { "pci-attach",
>        &main_pciattach, 0, 1,

[PATCH 3 of 3] hotplug: Change options used for shutdown command in xendomains script to be compatible with both xm and xl

[Sander Eikelenboom]

  * Use short options for shutdown command in xendomains script to be compatible with both xm and xl
  * Drop using the --halt: Linux in a guest treats "halt" and "poweroff" identically, so the --halt is pointless and not implemented in xl

Signed-off-by: Sander Eikelenboom <linux@eikelenboom.it>

diff -r 780eae92908a -r 261ce3cdaee8 tools/hotplug/Linux/init.d/sysconfig.xendomains
--- a/tools/hotplug/Linux/init.d/sysconfig.xendomains	Thu Sep 06 21:36:41 2012 +0200
+++ b/tools/hotplug/Linux/init.d/sysconfig.xendomains	Thu Sep 06 21:36:44 2012 +0200
@@ -56,29 +56,29 @@ XENDOMAINS_MIGRATE=""
 XENDOMAINS_SAVE=/var/lib/xen/save
 
 ## Type: string
-## Default: "--halt --wait"
+## Default: "-w"
 #
 # If neither MIGRATE nor SAVE were enabled or if they failed, you can
 # try to shut down a domain by sending it a shutdown request. To do this,
-# set this to "--halt --wait". Omit the "--wait" flag to avoid waiting
+# set this to "-w". Omit the "-w" flag to avoid waiting
 # for the domain to be really down. Leave empty to skip domain shutdown.
 #
-XENDOMAINS_SHUTDOWN="--halt --wait"
+XENDOMAINS_SHUTDOWN="-w"
 
 ## Type: string
-## Default: "--all --halt --wait"
+## Default: "-a -w"
 #
 # After we have gone over all virtual machines (resp. all automatically
 # started ones, see XENDOMAINS_AUTO_ONLY below) in a loop and sent SysRq,
 # migrated, saved and/or shutdown according to the settings above, we
 # might want to shutdown the virtual machines that are still running
 # for some reason or another. To do this, set this variable to
-# "--all --halt --wait", it will be passed to xm shutdown.
+# "-a -w", it will be passed to xm shutdown.
 # Leave it empty not to do anything special here.
 # (Note: This will hit all virtual machines, even if XENDOMAINS_AUTO_ONLY
 # is set.)
 # 
-XENDOMAINS_SHUTDOWN_ALL="--all --halt --wait"
+XENDOMAINS_SHUTDOWN_ALL="-a -w"
 
 ## Type: boolean
 ## Default: true
diff -r 780eae92908a -r 261ce3cdaee8 tools/hotplug/Linux/init.d/xendomains
--- a/tools/hotplug/Linux/init.d/xendomains	Thu Sep 06 21:36:41 2012 +0200
+++ b/tools/hotplug/Linux/init.d/xendomains	Thu Sep 06 21:36:44 2012 +0200
@@ -434,7 +434,7 @@ stop()
 	    fi
 	fi
 	if test -n "$XENDOMAINS_SHUTDOWN"; then
-	    # XENDOMAINS_SHUTDOWN should be "--halt --wait"
+	    # XENDOMAINS_SHUTDOWN should be "-w"
 	    echo -n "(shut)"
 	    watchdog_xencmd shutdown &
 	    WDOG_PID=$!
@@ -453,7 +453,7 @@ stop()
     # This is because it's easier to do ;-) but arguably if this script is run
     # on system shutdown then it's also the right thing to do.
     if ! all_zombies && test -n "$XENDOMAINS_SHUTDOWN_ALL"; then
-	# XENDOMAINS_SHUTDOWN_ALL should be "--all --halt --wait"
+	# XENDOMAINS_SHUTDOWN_ALL should be "-a -w"
 	echo -n " SHUTDOWN_ALL "
 	watchdog_xencmd shutdown 1 false &
 	WDOG_PID=$!
diff -r 780eae92908a -r 261ce3cdaee8 tools/hotplug/NetBSD/rc.d/xendomains
--- a/tools/hotplug/NetBSD/rc.d/xendomains	Thu Sep 06 21:36:41 2012 +0200
+++ b/tools/hotplug/NetBSD/rc.d/xendomains	Thu Sep 06 21:36:44 2012 +0200
@@ -94,7 +94,7 @@ xendomains_stop()
 	#
 	echo "Stopping xen domains."
 	for domain in $(xendomains_list); do
-		${ctl_command} shutdown --halt $domain
+		${ctl_command} shutdown $domain
 	done
 	while [ $timeout -gt 0 ]; do
 		livedomains=$(xendomains_list)

Re: [PATCH 3 of 3] hotplug: Change options used for shutdown command in xendomains script to be compatible with both xm and xl

[Ian Jackson]

Sander Eikelenboom writes ("[Xen-devel] [PATCH 3 of 3] hotplug: Change options used for shutdown command in xendomains script to be compatible with both xm and xl"):
>   * Use short options for shutdown command in xendomains script to be compatible with both xm and xl
>   * Drop using the --halt: Linux in a guest treats "halt" and "poweroff" identically, so the --halt is pointless and not implemented in xl

I think we should fix xl to support the long options too.

xl is supposed to be a command-line compatible replacement.

Both this and your support for shutting down all domains are IMO 4.2.1
material.

Ian.

Re: [PATCH 0 of 3] xl and hotplug: Introduce and use shutdown and reboot xm compatibility options

[Ian Campbell]

On Thu, 2012-09-06 at 20:41 +0100, Sander Eikelenboom wrote:
> The /etc/init.d/xendomains script makes use of options for the
> shutdown command defined in xendomains-sysconfig/default.
> These options were not implemented in xl, this patch series implements
> the options and by using the short variant makes it compatible with
> both xm and xl.
> 
> Signed-off-by: Sander Eikelenboom <linux@eikelenboom.it>

Hi Sander,

Are you still looking into the review feedback on these patches?

Ian.

Re: [PATCH 0 of 3] xl and hotplug: Introduce and use shutdown and reboot xm compatibility options

[Sander Eikelenboom]

Tuesday, September 25, 2012, 11:35:12 AM, you wrote:

> On Thu, 2012-09-06 at 20:41 +0100, Sander Eikelenboom wrote:
>> The /etc/init.d/xendomains script makes use of options for the
>> shutdown command defined in xendomains-sysconfig/default.
>> These options were not implemented in xl, this patch series implements
>> the options and by using the short variant makes it compatible with
>> both xm and xl.
>> 
>> Signed-off-by: Sander Eikelenboom <linux@eikelenboom.it>

> Hi Sander,

> Are you still looking into the review feedback on these patches?

> Ian.

Hi Ian,

Yes but had a little time last week and i'm a bit stuck on IanJ's comment  :-(


> This isn't quite right, I think.  Surely it should initiate the
> shutdown for all the domains right away, and then wait for them all to
> finish ?
>
> That's going to make the patch more complicated of course...
>
> Ian.

The only relative simple implementation i thought of was direct shutting down all, and when the -w parameter was set, just loop and wait on events until the only running domain is domain-0.
Although this exactly does what has to be done, it somehow sounds a bit dirty.


--
Sander

Re: [PATCH 0 of 3] xl and hotplug: Introduce and use shutdown and reboot xm compatibility options

[Ian Campbell]

On Tue, 2012-09-25 at 16:11 +0100, Sander Eikelenboom wrote:

> The only relative simple implementation i thought of was direct
> shutting down all, and when the -w parameter was set, just loop and
> wait on events until the only running domain is domain-0.
> Although this exactly does what has to be done, it somehow sounds a
> bit dirty.

I think you can allocate an array of libxl_evenable_domain_death*, of
nr_doms in size and then as you shutdown each domain call
libxl_evenable_domain_death for it too.

Then you loop waiting for destroy events, calling
libxl_evdisable_domain_death as each domain dies, while counting back
from nr_doms until 0. When it reaches 0 everything you asked for has
been shutdown.

Or something like that, I've not actually implemented it ;-)

Ian.

Re: [PATCH 0 of 3] xl and hotplug: Introduce and use shutdown and reboot xm compatibility options

[Sander Eikelenboom]

Tuesday, September 25, 2012, 5:29:09 PM, you wrote:

> On Tue, 2012-09-25 at 16:11 +0100, Sander Eikelenboom wrote:

>> The only relative simple implementation i thought of was direct
>> shutting down all, and when the -w parameter was set, just loop and
>> wait on events until the only running domain is domain-0.
>> Although this exactly does what has to be done, it somehow sounds a
>> bit dirty.

> I think you can allocate an array of libxl_evenable_domain_death*, of
> nr_doms in size and then as you shutdown each domain call
> libxl_evenable_domain_death for it too.

> Then you loop waiting for destroy events, calling
> libxl_evdisable_domain_death as each domain dies, while counting back
> from nr_doms until 0. When it reaches 0 everything you asked for has
> been shutdown.

Depends a bit on what you expect -a to do, especially in combination with -w.
The wait gives quite a window for a new guest to be created in the mean time.

The usage in the init.d/xendomains script is as a sort of "catch all", since the iteration is in the script it self.
So for that purpose i would assume that a "xl shutdown -a -w" would shutdown all domains until only dom-0 remains, and not only shutdown the domains that were running at the time the command was actually issued.

> Or something like that, I've not actually implemented it ;-)

> Ian.

Re: [PATCH 0 of 3] xl and hotplug: Introduce and use shutdown and reboot xm compatibility options

[Ian Jackson]

Sander Eikelenboom writes ("Re: [PATCH 0 of 3] xl and hotplug: Introduce and use shutdown and reboot xm compatibility options"):
> Depends a bit on what you expect -a to do, especially in combination with -w.
> The wait gives quite a window for a new guest to be created in the mean time.

I guess.  I think it would be acceptable to say "don't do that then"
but your argument has merit.

If you go down this route, you'll need to remember which domains
you've asked for shutdown for.  I'm not sure that asking again is
safe.

Ian.

Re: [PATCH 0 of 3] xl and hotplug: Introduce and use shutdown and reboot xm compatibility options

[Ian Campbell]

On Tue, 2012-09-25 at 16:47 +0100, Sander Eikelenboom wrote:
> Tuesday, September 25, 2012, 5:29:09 PM, you wrote:
> 
> > On Tue, 2012-09-25 at 16:11 +0100, Sander Eikelenboom wrote:
> 
> >> The only relative simple implementation i thought of was direct
> >> shutting down all, and when the -w parameter was set, just loop and
> >> wait on events until the only running domain is domain-0.
> >> Although this exactly does what has to be done, it somehow sounds a
> >> bit dirty.
> 
> > I think you can allocate an array of libxl_evenable_domain_death*, of
> > nr_doms in size and then as you shutdown each domain call
> > libxl_evenable_domain_death for it too.
> 
> > Then you loop waiting for destroy events, calling
> > libxl_evdisable_domain_death as each domain dies, while counting back
> > from nr_doms until 0. When it reaches 0 everything you asked for has
> > been shutdown.
> 
> Depends a bit on what you expect -a to do, especially in combination with -w.
> The wait gives quite a window for a new guest to be created in the mean time.
> 
> The usage in the init.d/xendomains script is as a sort of "catch all", since the iteration is in the script it self.
> So for that purpose i would assume that a "xl shutdown -a -w" would
> shutdown all domains until only dom-0 remains, and not only shutdown
> the domains that were running at the time the command was actually
> issued.

I'd be happy to assume that no one is starting new domains while the
system is in the process of shutting down...

Or maybe we could just drop a marker in xenstore to prevent new domains
being started. Sort of like how initscripts create /nologin to stop new
loggins during shutdown.

Ian.

Re: [PATCH 0 of 3] xl and hotplug: Introduce and use shutdown and reboot xm compatibility options

[Ian Jackson]

Ian Campbell writes ("Re: [PATCH 0 of 3] xl and hotplug: Introduce and use shutdown and reboot xm compatibility options"):
> On Tue, 2012-09-25 at 16:11 +0100, Sander Eikelenboom wrote:
> > The only relative simple implementation i thought of was direct
> > shutting down all, and when the -w parameter was set, just loop and
> > wait on events until the only running domain is domain-0.
> > Although this exactly does what has to be done, it somehow sounds a
> > bit dirty.
> 
> I think you can allocate an array of libxl_evenable_domain_death*, of
> nr_doms in size and then as you shutdown each domain call
> libxl_evenable_domain_death for it too.

Yes.

> Then you loop waiting for destroy events, calling
> libxl_evdisable_domain_death as each domain dies, while counting back
> from nr_doms until 0. When it reaches 0 everything you asked for has
> been shutdown.

You can do the disable any time later; leaving it lying around is
harmless (although it may leak some memory).

Ian.

Re: [PATCH 0 of 3] xl and hotplug: Introduce and use shutdown and reboot xm compatibility options

[Sander Eikelenboom]

[-- Attachment #1: Type: text/plain, Size: 9313 bytes --]


Tuesday, September 25, 2012, 5:29:09 PM, you wrote:

> On Tue, 2012-09-25 at 16:11 +0100, Sander Eikelenboom wrote:

>> The only relative simple implementation i thought of was direct
>> shutting down all, and when the -w parameter was set, just loop and
>> wait on events until the only running domain is domain-0.
>> Although this exactly does what has to be done, it somehow sounds a
>> bit dirty.

> I think you can allocate an array of libxl_evenable_domain_death*, of
> nr_doms in size and then as you shutdown each domain call
> libxl_evenable_domain_death for it too.

> Then you loop waiting for destroy events, calling
> libxl_evdisable_domain_death as each domain dies, while counting back
> from nr_doms until 0. When it reaches 0 everything you asked for has
> been shutdown.

> Or something like that, I've not actually implemented it ;-)

> Ian.

It's time to call defeat, without proper C skills it seems a bit too hard to figure it out.
Can't seem to get the hang of how to keep a reference to libxl_evgen_domain_death
and use it to check if the domid of the event is the same as that from a domain we are waiting to shutdown.
The rest of the code doesn't give me much pointers since all commands seem to operate on a single domid at once.

The concoction below leads to:
xl_cmdimpl.c: In function âshutdown_domainâ:
xl_cmdimpl.c:2766: error: dereferencing pointer to incomplete type


--------------------------- tools/libxl/xl_cmdimpl.c ---------------------------
index 1627cac..b900811 100644
@@ -2684,67 +2684,98 @@ static void destroy_domain(uint32_t domid)
     }
     rc = libxl_domain_destroy(ctx, domid, 0);
     if (rc) { fprintf(stderr,"destroy failed (rc=%d)\n",rc); exit(-1); }
 }
 
-static void shutdown_domain(uint32_t domid, int wait_for_it,
-                            int fallback_trigger)
+static int shutdown_domain(const libxl_dominfo *dominfo, int nb_domain,
+                            int wait_for_it, int fallback_trigger)
 {
-    int rc;
-    libxl_event *event;
+    int i, rc, nb_shutdown_pending;
+    int ret = 0;
+    libxl_evgen_domain_death *domains_wait_shutdown[nb_domain];
+       
+    for (i = 0; i < nb_domain; i++) {
+        uint32_t domid = dominfo[i].domid;
+        if(!libxl_domid_valid_guest(domid))
+            continue;
 
-    rc=libxl_domain_shutdown(ctx, domid);
-    if (rc == ERROR_NOPARAVIRT) {
-        if (fallback_trigger) {
-            fprintf(stderr, "PV control interface not available:"
-                    " sending ACPI power button event.\n");
-            rc = libxl_send_trigger(ctx, domid, LIBXL_TRIGGER_POWER, 0);
-        } else {
-            fprintf(stderr, "PV control interface not available:"
-                    " external graceful shutdown not possible.\n");
-            fprintf(stderr, "Use \"-F\" to fallback to ACPI power event.\n");
+        rc = libxl_domain_shutdown(ctx, domid);
+
+        if (rc == ERROR_NOPARAVIRT) {
+            if (fallback_trigger) {
+                fprintf(stderr, "PV control interface not available:"
+                        " sending ACPI power button event.\n");
+                rc = libxl_send_trigger(ctx, domid, LIBXL_TRIGGER_POWER, 0);
+            } else {
+                fprintf(stderr, "PV control interface not available:"
+                        " external graceful shutdown not possible.\n");
+                fprintf(stderr,
+                    "Use \"-F\" to fallback to ACPI power event.\n");
+            }
+        }
+        
+        if (rc) {
+            fprintf(stderr,"shutdown of domain %d failed (rc=%d)\n", domid, rc);
+            ret = -1;
+            continue;
+        }
+
+        if (wait_for_it) {
+            rc = libxl_evenable_domain_death(ctx, domid, 0, &domains_wait_shutdown[i]);
+            if (rc) {
+                fprintf(stderr,"wait for death of domain %d failed"
+                    " (evgen, rc=%d)\n", domid, rc);
+                ret = -1;
+            } else {
+                nb_shutdown_pending++;
+            } 
         }
-    }
-    if (rc) {
-        fprintf(stderr,"shutdown failed (rc=%d)\n",rc);exit(-1);
     }
 
     if (wait_for_it) {
-        libxl_evgen_domain_death *deathw;
+        while (nb_shutdown_pending > 0) {
+            libxl_event *event;
+            rc = libxl_event_wait(ctx, &event, LIBXL_EVENTMASK_ALL, 0,0);
+            if (rc) {
+                LOG("Failed to get event (rc=%d)", rc);
+                return -1;
+            }
 
-        rc = libxl_evenable_domain_death(ctx, domid, 0, &deathw);
-        if (rc) {
-            fprintf(stderr,"wait for death failed (evgen, rc=%d)\n",rc);
-            exit(-1);
-        }
+            uint32_t event_domid = event->domid;
+            switch (event->type) {
 
-        for (;;) {
-            rc = domain_wait_event(domid, &event);
-            if (rc) exit(-1);
+                case LIBXL_EVENT_TYPE_DOMAIN_DEATH:
+                    LOG("Domain %d has been destroyed", event_domid);
+                    break;
 
-            switch (event->type) {
+                case LIBXL_EVENT_TYPE_DOMAIN_SHUTDOWN:
+                    LOG("Domain %d has been shut down, reason code %d %x",
+                        event_domid,
+                        event->u.domain_shutdown.shutdown_reason,
+                        event->u.domain_shutdown.shutdown_reason);
+                    break;
 
-            case LIBXL_EVENT_TYPE_DOMAIN_DEATH:
-                LOG("Domain %d has been destroyed", domid);
-                goto done;
+                default:
+                    continue;
+                    break;
+            }
 
-            case LIBXL_EVENT_TYPE_DOMAIN_SHUTDOWN:
-                LOG("Domain %d has been shut down, reason code %d %x", domid,
-                    event->u.domain_shutdown.shutdown_reason,
-                    event->u.domain_shutdown.shutdown_reason);
-                goto done;
+            for (i = 0; i < nb_domain; i++) {
+                if (!domains_wait_shutdown[i])
+                    continue;
 
-            default:
-                LOG("Unexpected event type %d", event->type);
-                break;
+                if (domains_wait_shutdown[i]->domid == event_domid){
+                    libxl_evdisable_domain_death(ctx, domains_wait_shutdown[i]);
+                    domains_wait_shutdown[i] = NULL;
+                    nb_shutdown_pending--;
+                }
             }
             libxl_event_free(ctx, event);
         }
-    done:
-        libxl_event_free(ctx, event);
-        libxl_evdisable_domain_death(ctx, deathw);
     }
+
+    return ret;
 }
 
 static void reboot_domain(uint32_t domid, int fallback_trigger)
 {
     int rc;
@@ -3674,29 +3705,82 @@ int main_destroy(int argc, char **argv)
     return 0;
 }
 
 int main_shutdown(int argc, char **argv)
 {
+    libxl_dominfo dominfo_buf;
+    libxl_dominfo *dominfo, *dominfo_free = NULL;
+    int nb_domain, rc;
     int opt;
+    int option_index = 0;
+    int all = 0;
     int wait_for_it = 0;
     int fallback_trigger = 0;
+    static struct option long_options[] = {
+        {"all", 0, 0, 'a'},
+        {"wait", 0, 0, 'w'},
+        {0, 0, 0, 0}
+    };
 
-    while ((opt = def_getopt(argc, argv, "wF", "shutdown", 1)) != -1) {
+    while ((opt = getopt_long(argc, argv, "awF", long_options, &option_index)) != -1) {
         switch (opt) {
         case 0: case 2:
             return opt;
+        case 'a':
+            all = 1;
+            break;
         case 'w':
             wait_for_it = 1;
             break;
         case 'F':
             fallback_trigger = 1;
             break;
         }
     }
 
-    shutdown_domain(find_domain(argv[optind]), wait_for_it, fallback_trigger);
-    return 0;
+    if (!argv[optind] && !all) {
+        fprintf(stderr, "You must specify -a or a valid domain id.\n\n");
+        return opt;
+    }
+
+    if (all) {
+        dominfo = libxl_list_domain(ctx, &nb_domain);
+        if (!dominfo) {
+            fprintf(stderr, "libxl_domain_infolist failed.\n");
+            return -1;
+        }
+        dominfo_free = dominfo;
+    } else {
+        uint32_t domid = find_domain(argv[optind]);
+        if (domid == 0) {
+            fprintf(stderr, "Error: Domain-0 can't be shutdown by this command\n");
+            return -1;
+        }
+
+        rc = libxl_domain_info(ctx, &dominfo_buf, domid);
+
+        if (rc == ERROR_INVAL) {
+            fprintf(stderr, "Error: Domain \'%s\' does not exist.\n",
+                argv[optind]);
+            return -rc;
+        }
+        if (rc) {
+            fprintf(stderr, "libxl_domain_info failed (code %d).\n", rc);
+            return -rc;
+        }
+        dominfo = &dominfo_buf;
+        nb_domain = 1;
+    }
+
+    rc = shutdown_domain(dominfo, nb_domain, wait_for_it, fallback_trigger);
+
+    if (dominfo_free)
+        libxl_dominfo_list_free(dominfo_free, nb_domain);
+    else
+        libxl_dominfo_dispose(dominfo);
+
+    return -rc;
 }
 
 int main_reboot(int argc, char **argv)
 {
     int opt;





[-- Attachment #2: patch.diff --]
[-- Type: application/octet-stream, Size: 7567 bytes --]

diff --git a/tools/libxl/xl_cmdimpl.c b/tools/libxl/xl_cmdimpl.c
index 1627cac..b900811 100644
--- a/tools/libxl/xl_cmdimpl.c
+++ b/tools/libxl/xl_cmdimpl.c
@@ -2686,63 +2686,94 @@ static void destroy_domain(uint32_t domid)
     if (rc) { fprintf(stderr,"destroy failed (rc=%d)\n",rc); exit(-1); }
 }
 
-static void shutdown_domain(uint32_t domid, int wait_for_it,
-                            int fallback_trigger)
+static int shutdown_domain(const libxl_dominfo *dominfo, int nb_domain,
+                            int wait_for_it, int fallback_trigger)
 {
-    int rc;
-    libxl_event *event;
+    int i, rc, nb_shutdown_pending;
+    int ret = 0;
+    libxl_evgen_domain_death *domains_wait_shutdown[nb_domain];
+       
+    for (i = 0; i < nb_domain; i++) {
+        uint32_t domid = dominfo[i].domid;
+        if(!libxl_domid_valid_guest(domid))
+            continue;
 
-    rc=libxl_domain_shutdown(ctx, domid);
-    if (rc == ERROR_NOPARAVIRT) {
-        if (fallback_trigger) {
-            fprintf(stderr, "PV control interface not available:"
-                    " sending ACPI power button event.\n");
-            rc = libxl_send_trigger(ctx, domid, LIBXL_TRIGGER_POWER, 0);
-        } else {
-            fprintf(stderr, "PV control interface not available:"
-                    " external graceful shutdown not possible.\n");
-            fprintf(stderr, "Use \"-F\" to fallback to ACPI power event.\n");
+        rc = libxl_domain_shutdown(ctx, domid);
+
+        if (rc == ERROR_NOPARAVIRT) {
+            if (fallback_trigger) {
+                fprintf(stderr, "PV control interface not available:"
+                        " sending ACPI power button event.\n");
+                rc = libxl_send_trigger(ctx, domid, LIBXL_TRIGGER_POWER, 0);
+            } else {
+                fprintf(stderr, "PV control interface not available:"
+                        " external graceful shutdown not possible.\n");
+                fprintf(stderr,
+                    "Use \"-F\" to fallback to ACPI power event.\n");
+            }
+        }
+        
+        if (rc) {
+            fprintf(stderr,"shutdown of domain %d failed (rc=%d)\n", domid, rc);
+            ret = -1;
+            continue;
+        }
+
+        if (wait_for_it) {
+            rc = libxl_evenable_domain_death(ctx, domid, 0, &domains_wait_shutdown[i]);
+            if (rc) {
+                fprintf(stderr,"wait for death of domain %d failed"
+                    " (evgen, rc=%d)\n", domid, rc);
+                ret = -1;
+            } else {
+                nb_shutdown_pending++;
+            } 
         }
-    }
-    if (rc) {
-        fprintf(stderr,"shutdown failed (rc=%d)\n",rc);exit(-1);
     }
 
     if (wait_for_it) {
-        libxl_evgen_domain_death *deathw;
+        while (nb_shutdown_pending > 0) {
+            libxl_event *event;
+            rc = libxl_event_wait(ctx, &event, LIBXL_EVENTMASK_ALL, 0,0);
+            if (rc) {
+                LOG("Failed to get event (rc=%d)", rc);
+                return -1;
+            }
 
-        rc = libxl_evenable_domain_death(ctx, domid, 0, &deathw);
-        if (rc) {
-            fprintf(stderr,"wait for death failed (evgen, rc=%d)\n",rc);
-            exit(-1);
-        }
+            uint32_t event_domid = event->domid;
+            switch (event->type) {
 
-        for (;;) {
-            rc = domain_wait_event(domid, &event);
-            if (rc) exit(-1);
+                case LIBXL_EVENT_TYPE_DOMAIN_DEATH:
+                    LOG("Domain %d has been destroyed", event_domid);
+                    break;
 
-            switch (event->type) {
+                case LIBXL_EVENT_TYPE_DOMAIN_SHUTDOWN:
+                    LOG("Domain %d has been shut down, reason code %d %x",
+                        event_domid,
+                        event->u.domain_shutdown.shutdown_reason,
+                        event->u.domain_shutdown.shutdown_reason);
+                    break;
 
-            case LIBXL_EVENT_TYPE_DOMAIN_DEATH:
-                LOG("Domain %d has been destroyed", domid);
-                goto done;
+                default:
+                    continue;
+                    break;
+            }
 
-            case LIBXL_EVENT_TYPE_DOMAIN_SHUTDOWN:
-                LOG("Domain %d has been shut down, reason code %d %x", domid,
-                    event->u.domain_shutdown.shutdown_reason,
-                    event->u.domain_shutdown.shutdown_reason);
-                goto done;
+            for (i = 0; i < nb_domain; i++) {
+                if (!domains_wait_shutdown[i])
+                    continue;
 
-            default:
-                LOG("Unexpected event type %d", event->type);
-                break;
+                if (domains_wait_shutdown[i]->domid == event_domid){
+                    libxl_evdisable_domain_death(ctx, domains_wait_shutdown[i]);
+                    domains_wait_shutdown[i] = NULL;
+                    nb_shutdown_pending--;
+                }
             }
             libxl_event_free(ctx, event);
         }
-    done:
-        libxl_event_free(ctx, event);
-        libxl_evdisable_domain_death(ctx, deathw);
     }
+
+    return ret;
 }
 
 static void reboot_domain(uint32_t domid, int fallback_trigger)
@@ -3676,14 +3707,27 @@ int main_destroy(int argc, char **argv)
 
 int main_shutdown(int argc, char **argv)
 {
+    libxl_dominfo dominfo_buf;
+    libxl_dominfo *dominfo, *dominfo_free = NULL;
+    int nb_domain, rc;
     int opt;
+    int option_index = 0;
+    int all = 0;
     int wait_for_it = 0;
     int fallback_trigger = 0;
+    static struct option long_options[] = {
+        {"all", 0, 0, 'a'},
+        {"wait", 0, 0, 'w'},
+        {0, 0, 0, 0}
+    };
 
-    while ((opt = def_getopt(argc, argv, "wF", "shutdown", 1)) != -1) {
+    while ((opt = getopt_long(argc, argv, "awF", long_options, &option_index)) != -1) {
         switch (opt) {
         case 0: case 2:
             return opt;
+        case 'a':
+            all = 1;
+            break;
         case 'w':
             wait_for_it = 1;
             break;
@@ -3693,8 +3737,48 @@ int main_shutdown(int argc, char **argv)
         }
     }
 
-    shutdown_domain(find_domain(argv[optind]), wait_for_it, fallback_trigger);
-    return 0;
+    if (!argv[optind] && !all) {
+        fprintf(stderr, "You must specify -a or a valid domain id.\n\n");
+        return opt;
+    }
+
+    if (all) {
+        dominfo = libxl_list_domain(ctx, &nb_domain);
+        if (!dominfo) {
+            fprintf(stderr, "libxl_domain_infolist failed.\n");
+            return -1;
+        }
+        dominfo_free = dominfo;
+    } else {
+        uint32_t domid = find_domain(argv[optind]);
+        if (domid == 0) {
+            fprintf(stderr, "Error: Domain-0 can't be shutdown by this command\n");
+            return -1;
+        }
+
+        rc = libxl_domain_info(ctx, &dominfo_buf, domid);
+
+        if (rc == ERROR_INVAL) {
+            fprintf(stderr, "Error: Domain \'%s\' does not exist.\n",
+                argv[optind]);
+            return -rc;
+        }
+        if (rc) {
+            fprintf(stderr, "libxl_domain_info failed (code %d).\n", rc);
+            return -rc;
+        }
+        dominfo = &dominfo_buf;
+        nb_domain = 1;
+    }
+
+    rc = shutdown_domain(dominfo, nb_domain, wait_for_it, fallback_trigger);
+
+    if (dominfo_free)
+        libxl_dominfo_list_free(dominfo_free, nb_domain);
+    else
+        libxl_dominfo_dispose(dominfo);
+
+    return -rc;
 }
 
 int main_reboot(int argc, char **argv)

[-- Attachment #3: Type: text/plain, Size: 126 bytes --]

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

Re: [PATCH 0 of 3] xl and hotplug: Introduce and use shutdown and reboot xm compatibility options

[Ian Campbell]

On Thu, 2012-09-27 at 21:11 +0100, Sander Eikelenboom wrote:
> Tuesday, September 25, 2012, 5:29:09 PM, you wrote:
> 
> > On Tue, 2012-09-25 at 16:11 +0100, Sander Eikelenboom wrote:
> 
> >> The only relative simple implementation i thought of was direct
> >> shutting down all, and when the -w parameter was set, just loop and
> >> wait on events until the only running domain is domain-0.
> >> Although this exactly does what has to be done, it somehow sounds a
> >> bit dirty.
> 
> > I think you can allocate an array of libxl_evenable_domain_death*, of
> > nr_doms in size and then as you shutdown each domain call
> > libxl_evenable_domain_death for it too.
> 
> > Then you loop waiting for destroy events, calling
> > libxl_evdisable_domain_death as each domain dies, while counting back
> > from nr_doms until 0. When it reaches 0 everything you asked for has
> > been shutdown.
> 
> > Or something like that, I've not actually implemented it ;-)
> 
> > Ian.
> 
> It's time to call defeat, without proper C skills it seems a bit too
> hard to figure it out.

That's ok, I'll add it to my TODO to pickup where you left off (unless
someone beats me to it).

Thanks for taking it this far! FWIW you seemed to be pretty close.

> Can't seem to get the hang of how to keep a reference to
> libxl_evgen_domain_death and use it to check if the domid of the event
> is the same as that from a domain we are waiting to shutdown.
> The rest of the code doesn't give me much pointers since all commands
> seem to operate on a single domid at once.
> 
> The concoction below leads to:
> xl_cmdimpl.c: In function âshutdown_domainâ:
> xl_cmdimpl.c:2766: error: dereferencing pointer to incomplete type

In case you are interested this is because the libxl_evgen_domain_death
type is opaque to users of libxl so constructs like
domains_wait_shutdown[i]->domid are not possible.

The answer is to use the 3rd argument to libxl_evenable_domain_death
(the one of type libxl_ev_user) to pass a "closure", this value is
returned in the event's "for_user" field. The application can use this
to store data specific to this particular event -- in this case it would
probably be sufficient to just pass the domid here but in principal one
could pass a pointer to any datastructure.

Thanks again,
Ian.

> 
> 
> --------------------------- tools/libxl/xl_cmdimpl.c ---------------------------
> index 1627cac..b900811 100644
> @@ -2684,67 +2684,98 @@ static void destroy_domain(uint32_t domid)
>      }
>      rc = libxl_domain_destroy(ctx, domid, 0);
>      if (rc) { fprintf(stderr,"destroy failed (rc=%d)\n",rc); exit(-1); }
>  }
>  
> -static void shutdown_domain(uint32_t domid, int wait_for_it,
> -                            int fallback_trigger)
> +static int shutdown_domain(const libxl_dominfo *dominfo, int nb_domain,
> +                            int wait_for_it, int fallback_trigger)
>  {
> -    int rc;
> -    libxl_event *event;
> +    int i, rc, nb_shutdown_pending;
> +    int ret = 0;
> +    libxl_evgen_domain_death *domains_wait_shutdown[nb_domain];
> +       
> +    for (i = 0; i < nb_domain; i++) {
> +        uint32_t domid = dominfo[i].domid;
> +        if(!libxl_domid_valid_guest(domid))
> +            continue;
>  
> -    rc=libxl_domain_shutdown(ctx, domid);
> -    if (rc == ERROR_NOPARAVIRT) {
> -        if (fallback_trigger) {
> -            fprintf(stderr, "PV control interface not available:"
> -                    " sending ACPI power button event.\n");
> -            rc = libxl_send_trigger(ctx, domid, LIBXL_TRIGGER_POWER, 0);
> -        } else {
> -            fprintf(stderr, "PV control interface not available:"
> -                    " external graceful shutdown not possible.\n");
> -            fprintf(stderr, "Use \"-F\" to fallback to ACPI power event.\n");
> +        rc = libxl_domain_shutdown(ctx, domid);
> +
> +        if (rc == ERROR_NOPARAVIRT) {
> +            if (fallback_trigger) {
> +                fprintf(stderr, "PV control interface not available:"
> +                        " sending ACPI power button event.\n");
> +                rc = libxl_send_trigger(ctx, domid, LIBXL_TRIGGER_POWER, 0);
> +            } else {
> +                fprintf(stderr, "PV control interface not available:"
> +                        " external graceful shutdown not possible.\n");
> +                fprintf(stderr,
> +                    "Use \"-F\" to fallback to ACPI power event.\n");
> +            }
> +        }
> +        
> +        if (rc) {
> +            fprintf(stderr,"shutdown of domain %d failed (rc=%d)\n", domid, rc);
> +            ret = -1;
> +            continue;
> +        }
> +
> +        if (wait_for_it) {
> +            rc = libxl_evenable_domain_death(ctx, domid, 0, &domains_wait_shutdown[i]);
> +            if (rc) {
> +                fprintf(stderr,"wait for death of domain %d failed"
> +                    " (evgen, rc=%d)\n", domid, rc);
> +                ret = -1;
> +            } else {
> +                nb_shutdown_pending++;
> +            } 
>          }
> -    }
> -    if (rc) {
> -        fprintf(stderr,"shutdown failed (rc=%d)\n",rc);exit(-1);
>      }
>  
>      if (wait_for_it) {
> -        libxl_evgen_domain_death *deathw;
> +        while (nb_shutdown_pending > 0) {
> +            libxl_event *event;
> +            rc = libxl_event_wait(ctx, &event, LIBXL_EVENTMASK_ALL, 0,0);
> +            if (rc) {
> +                LOG("Failed to get event (rc=%d)", rc);
> +                return -1;
> +            }
>  
> -        rc = libxl_evenable_domain_death(ctx, domid, 0, &deathw);
> -        if (rc) {
> -            fprintf(stderr,"wait for death failed (evgen, rc=%d)\n",rc);
> -            exit(-1);
> -        }
> +            uint32_t event_domid = event->domid;
> +            switch (event->type) {
>  
> -        for (;;) {
> -            rc = domain_wait_event(domid, &event);
> -            if (rc) exit(-1);
> +                case LIBXL_EVENT_TYPE_DOMAIN_DEATH:
> +                    LOG("Domain %d has been destroyed", event_domid);
> +                    break;
>  
> -            switch (event->type) {
> +                case LIBXL_EVENT_TYPE_DOMAIN_SHUTDOWN:
> +                    LOG("Domain %d has been shut down, reason code %d %x",
> +                        event_domid,
> +                        event->u.domain_shutdown.shutdown_reason,
> +                        event->u.domain_shutdown.shutdown_reason);
> +                    break;
>  
> -            case LIBXL_EVENT_TYPE_DOMAIN_DEATH:
> -                LOG("Domain %d has been destroyed", domid);
> -                goto done;
> +                default:
> +                    continue;
> +                    break;
> +            }
>  
> -            case LIBXL_EVENT_TYPE_DOMAIN_SHUTDOWN:
> -                LOG("Domain %d has been shut down, reason code %d %x", domid,
> -                    event->u.domain_shutdown.shutdown_reason,
> -                    event->u.domain_shutdown.shutdown_reason);
> -                goto done;
> +            for (i = 0; i < nb_domain; i++) {
> +                if (!domains_wait_shutdown[i])
> +                    continue;
>  
> -            default:
> -                LOG("Unexpected event type %d", event->type);
> -                break;
> +                if (domains_wait_shutdown[i]->domid == event_domid){
> +                    libxl_evdisable_domain_death(ctx, domains_wait_shutdown[i]);
> +                    domains_wait_shutdown[i] = NULL;
> +                    nb_shutdown_pending--;
> +                }
>              }
>              libxl_event_free(ctx, event);
>          }
> -    done:
> -        libxl_event_free(ctx, event);
> -        libxl_evdisable_domain_death(ctx, deathw);
>      }
> +
> +    return ret;
>  }
>  
>  static void reboot_domain(uint32_t domid, int fallback_trigger)
>  {
>      int rc;
> @@ -3674,29 +3705,82 @@ int main_destroy(int argc, char **argv)
>      return 0;
>  }
>  
>  int main_shutdown(int argc, char **argv)
>  {
> +    libxl_dominfo dominfo_buf;
> +    libxl_dominfo *dominfo, *dominfo_free = NULL;
> +    int nb_domain, rc;
>      int opt;
> +    int option_index = 0;
> +    int all = 0;
>      int wait_for_it = 0;
>      int fallback_trigger = 0;
> +    static struct option long_options[] = {
> +        {"all", 0, 0, 'a'},
> +        {"wait", 0, 0, 'w'},
> +        {0, 0, 0, 0}
> +    };
>  
> -    while ((opt = def_getopt(argc, argv, "wF", "shutdown", 1)) != -1) {
> +    while ((opt = getopt_long(argc, argv, "awF", long_options, &option_index)) != -1) {
>          switch (opt) {
>          case 0: case 2:
>              return opt;
> +        case 'a':
> +            all = 1;
> +            break;
>          case 'w':
>              wait_for_it = 1;
>              break;
>          case 'F':
>              fallback_trigger = 1;
>              break;
>          }
>      }
>  
> -    shutdown_domain(find_domain(argv[optind]), wait_for_it, fallback_trigger);
> -    return 0;
> +    if (!argv[optind] && !all) {
> +        fprintf(stderr, "You must specify -a or a valid domain id.\n\n");
> +        return opt;
> +    }
> +
> +    if (all) {
> +        dominfo = libxl_list_domain(ctx, &nb_domain);
> +        if (!dominfo) {
> +            fprintf(stderr, "libxl_domain_infolist failed.\n");
> +            return -1;
> +        }
> +        dominfo_free = dominfo;
> +    } else {
> +        uint32_t domid = find_domain(argv[optind]);
> +        if (domid == 0) {
> +            fprintf(stderr, "Error: Domain-0 can't be shutdown by this command\n");
> +            return -1;
> +        }
> +
> +        rc = libxl_domain_info(ctx, &dominfo_buf, domid);
> +
> +        if (rc == ERROR_INVAL) {
> +            fprintf(stderr, "Error: Domain \'%s\' does not exist.\n",
> +                argv[optind]);
> +            return -rc;
> +        }
> +        if (rc) {
> +            fprintf(stderr, "libxl_domain_info failed (code %d).\n", rc);
> +            return -rc;
> +        }
> +        dominfo = &dominfo_buf;
> +        nb_domain = 1;
> +    }
> +
> +    rc = shutdown_domain(dominfo, nb_domain, wait_for_it, fallback_trigger);
> +
> +    if (dominfo_free)
> +        libxl_dominfo_list_free(dominfo_free, nb_domain);
> +    else
> +        libxl_dominfo_dispose(dominfo);
> +
> +    return -rc;
>  }
>  
>  int main_reboot(int argc, char **argv)
>  {
>      int opt;
> 
> 
> 
> 



_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

Re: [PATCH 0 of 3] xl and hotplug: Introduce and use shutdown and reboot xm compatibility options

[Sander Eikelenboom]

Tuesday, October 2, 2012, 4:02:18 PM, you wrote:

> On Thu, 2012-09-27 at 21:11 +0100, Sander Eikelenboom wrote:
>> Tuesday, September 25, 2012, 5:29:09 PM, you wrote:
>> 
>> > On Tue, 2012-09-25 at 16:11 +0100, Sander Eikelenboom wrote:
>> 
>> >> The only relative simple implementation i thought of was direct
>> >> shutting down all, and when the -w parameter was set, just loop and
>> >> wait on events until the only running domain is domain-0.
>> >> Although this exactly does what has to be done, it somehow sounds a
>> >> bit dirty.
>> 
>> > I think you can allocate an array of libxl_evenable_domain_death*, of
>> > nr_doms in size and then as you shutdown each domain call
>> > libxl_evenable_domain_death for it too.
>> 
>> > Then you loop waiting for destroy events, calling
>> > libxl_evdisable_domain_death as each domain dies, while counting back
>> > from nr_doms until 0. When it reaches 0 everything you asked for has
>> > been shutdown.
>> 
>> > Or something like that, I've not actually implemented it ;-)
>> 
>> > Ian.
>> 
>> It's time to call defeat, without proper C skills it seems a bit too
>> hard to figure it out.

> That's ok, I'll add it to my TODO to pickup where you left off (unless
> someone beats me to it).

Thx !
Would be nice for 4.2.1 since the xendomains stop script doesn't function properly with xl as toolstack, and not shutting down the domains but just pulls the plug.
Would it be helpful to you, to send the doc patches as well ?


> Thanks for taking it this far! FWIW you seemed to be pretty close.

>> Can't seem to get the hang of how to keep a reference to
>> libxl_evgen_domain_death and use it to check if the domid of the event
>> is the same as that from a domain we are waiting to shutdown.
>> The rest of the code doesn't give me much pointers since all commands
>> seem to operate on a single domid at once.
>> 
>> The concoction below leads to:
>> xl_cmdimpl.c: In function âshutdown_domainâ:
>> xl_cmdimpl.c:2766: error: dereferencing pointer to incomplete type

> In case you are interested this is because the libxl_evgen_domain_death
> type is opaque to users of libxl so constructs like
> domains_wait_shutdown[i]->domid are not possible.

> The answer is to use the 3rd argument to libxl_evenable_domain_death
> (the one of type libxl_ev_user) to pass a "closure", this value is
> returned in the event's "for_user" field. The application can use this
> to store data specific to this particular event -- in this case it would
> probably be sufficient to just pass the domid here but in principal one
> could pass a pointer to any datastructure.

Thanks for the explanation, although i still don't see how to actually change the code the right way.


> Thanks again,
> Ian.






_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

Re: [PATCH 0 of 3] xl and hotplug: Introduce and use shutdown and reboot xm compatibility options

[Ian Campbell]

On Wed, 2012-10-03 at 10:03 +0100, Sander Eikelenboom wrote:
> Tuesday, October 2, 2012, 4:02:18 PM, you wrote:
> 
> > On Thu, 2012-09-27 at 21:11 +0100, Sander Eikelenboom wrote:
> >> Tuesday, September 25, 2012, 5:29:09 PM, you wrote:
> >> 
> >> > On Tue, 2012-09-25 at 16:11 +0100, Sander Eikelenboom wrote:
> >> 
> >> >> The only relative simple implementation i thought of was direct
> >> >> shutting down all, and when the -w parameter was set, just loop and
> >> >> wait on events until the only running domain is domain-0.
> >> >> Although this exactly does what has to be done, it somehow sounds a
> >> >> bit dirty.
> >> 
> >> > I think you can allocate an array of libxl_evenable_domain_death*, of
> >> > nr_doms in size and then as you shutdown each domain call
> >> > libxl_evenable_domain_death for it too.
> >> 
> >> > Then you loop waiting for destroy events, calling
> >> > libxl_evdisable_domain_death as each domain dies, while counting back
> >> > from nr_doms until 0. When it reaches 0 everything you asked for has
> >> > been shutdown.
> >> 
> >> > Or something like that, I've not actually implemented it ;-)
> >> 
> >> > Ian.
> >> 
> >> It's time to call defeat, without proper C skills it seems a bit too
> >> hard to figure it out.
> 
> > That's ok, I'll add it to my TODO to pickup where you left off (unless
> > someone beats me to it).
> 
> Thx !
> Would be nice for 4.2.1 since the xendomains stop script doesn't
> function properly with xl as toolstack, and not shutting down the
> domains but just pulls the plug.

Agreed.

> Would it be helpful to you, to send the doc patches as well ?

Absolutely!

> 
> 
> > Thanks for taking it this far! FWIW you seemed to be pretty close.
> 
> >> Can't seem to get the hang of how to keep a reference to
> >> libxl_evgen_domain_death and use it to check if the domid of the event
> >> is the same as that from a domain we are waiting to shutdown.
> >> The rest of the code doesn't give me much pointers since all commands
> >> seem to operate on a single domid at once.
> >> 
> >> The concoction below leads to:
> >> xl_cmdimpl.c: In function âshutdown_domainâ:
> >> xl_cmdimpl.c:2766: error: dereferencing pointer to incomplete type
> 
> > In case you are interested this is because the libxl_evgen_domain_death
> > type is opaque to users of libxl so constructs like
> > domains_wait_shutdown[i]->domid are not possible.
> 
> > The answer is to use the 3rd argument to libxl_evenable_domain_death
> > (the one of type libxl_ev_user) to pass a "closure", this value is
> > returned in the event's "for_user" field. The application can use this
> > to store data specific to this particular event -- in this case it would
> > probably be sufficient to just pass the domid here but in principal one
> > could pass a pointer to any datastructure.
> 
> Thanks for the explanation, although i still don't see how to actually change the code the right way.
> 
> 
> > Thanks again,
> > Ian.
> 
> 
> 
> 
> 



_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel