Re: [Xen-devel] [PATCH 2 of 2] libxl: add support for booting PV domains from NetBSD using raw files as disks

[Ian Campbell <Ian.Campbell@xxxxxxxxxx>]

On Fri, 2011-09-16 at 16:36 +0100, Roger Pau Monne wrote:
> # HG changeset patch
> # User Roger Pau Monne <roger.pau@xxxxxxxxxxxxx>
> # Date 1316187362 -7200
> # Node ID aff3960421768180410c8d553acf8881435bc3b4
> # Parent  00949e363f6f2c70001da548403475628df93b97
> libxl: add support for booting PV domains from NetBSD using raw files as 
> disks.
> 
> Used the hotplug-status attribute in xenstore for vbd to check when the 
> device is offline.
> 
> Signed-off-by: Roger Pau Monne <roger.pau@xxxxxxxxxxxxx>

Again this generally looks good but I think the functional split between
the patches isn't quite right.

> 
> diff -r 00949e363f6f -r aff396042176 tools/libxl/libxl_device.c
> --- a/tools/libxl/libxl_device.c      Fri Sep 16 17:29:45 2011 +0200
> +++ b/tools/libxl/libxl_device.c      Fri Sep 16 17:36:02 2011 +0200
> @@ -366,14 +371,26 @@ int libxl__device_destroy(libxl__gc *gc,


This bit (and most of this patch) goes along with the hotplug script
changes in patch 1/2, doesn't it?

>      libxl_ctx *ctx = libxl__gc_owner(gc);
>      xs_transaction_t t;
>      char *state_path = libxl__sprintf(gc, "%s/state", be_path);
> +    char *hotplug_path = libxl__sprintf(gc, "%s/hotplug-status", be_path);
>      char *state = libxl__xs_read(gc, XBT_NULL, state_path);
> +    char *hotplug = libxl__xs_read(gc, XBT_NULL, hotplug_path);
>      int rc = 0;
>  
>      if (!state)
>          goto out;
> -    if (atoi(state) != 4) {
> -        xs_rm(ctx->xsh, XBT_NULL, be_path);
> -        goto out;
> +
> +    if (!strstr(be_path, "vbd")) {

I've got a WIP patch which passes a libxl__device to functions like this
which will be helpful for removing these strstr things, since you can
just use the DEVICE_* enum.

> +        if (atoi(state) != 4) {
> +            xs_rm(ctx->xsh, XBT_NULL, be_path);
> +            goto out;
> +        }
> +    } else {
> +        if (!hotplug)
> +            goto out;
> +        if (!strcmp(hotplug, "disconnected")) {
> +            xs_rm(ctx->xsh, XBT_NULL, be_path);
> +            goto out;
> +        }
>      }
>  
>  retry_transaction:

> @@ -389,8 +406,13 @@ retry_transaction:
>          }
>      }
>      if (!force) {
> -        xs_watch(ctx->xsh, state_path, be_path);
> -        rc = 1;
> +        if (strstr(be_path, "vbd")) {
> +            xs_watch(ctx->xsh, hotplug_path, be_path);
> +            rc = 1;
> +        } else {
> +            xs_watch(ctx->xsh, state_path, be_path);
> +            rc = 1;
> +        }
>      } else {
>          xs_rm(ctx->xsh, XBT_NULL, be_path);
>      }
> @@ -405,6 +427,7 @@ static int wait_for_dev_destroy(libxl__g
>      unsigned int n;
>      fd_set rfds;
>      char **l1 = NULL;
> +    char *state, *hotplug;
>  
>      rc = 1;
>      nfds = xs_fileno(ctx->xsh) + 1;
> @@ -413,15 +436,29 @@ static int wait_for_dev_destroy(libxl__g
>      if (select(nfds, &rfds, NULL, NULL, tv) > 0) {
>          l1 = xs_read_watch(ctx->xsh, &n);
>          if (l1 != NULL) {
> -            char *state = libxl__xs_read(gc, XBT_NULL, l1[XS_WATCH_PATH]);
> -            if (!state || atoi(state) == 6) {
> -                xs_unwatch(ctx->xsh, l1[0], l1[1]);
> -                xs_rm(ctx->xsh, XBT_NULL, l1[XS_WATCH_TOKEN]);
> -                LIBXL__LOG(ctx, LIBXL__LOG_DEBUG, "Destroyed device backend 
> at %s", l1[XS_WATCH_TOKEN]);
> -                rc = 0;
> +            if (strstr(l1[XS_WATCH_PATH], "hotplug-status")) {
> +                hotplug = libxl__xs_read(gc, XBT_NULL, l1[XS_WATCH_PATH]);
> +                if (!hotplug || !strcmp(hotplug, "disconnected")) {
> +                    xs_unwatch(ctx->xsh, l1[0], l1[1]);
> +                    xs_rm(ctx->xsh, XBT_NULL, l1[XS_WATCH_TOKEN]);
> +                    LIBXL__LOG(ctx, LIBXL__LOG_DEBUG, "Destroyed device 
> backend at %s hotplug-status: %s", 
> +                               l1[XS_WATCH_TOKEN], hotplug);
> +                    rc = 0;
> +                }
> +            } else {
> +                state = libxl__xs_read(gc, XBT_NULL, l1[XS_WATCH_PATH]);
> +                if (!state || atoi(state) == 6) {
> +                    xs_unwatch(ctx->xsh, l1[0], l1[1]);
> +                    xs_rm(ctx->xsh, XBT_NULL, l1[XS_WATCH_TOKEN]);
> +                    LIBXL__LOG(ctx, LIBXL__LOG_DEBUG, "Destroyed device 
> backend at %s", l1[XS_WATCH_TOKEN]);
> +                    rc = 0;
> +                }
>              }
>              free(l1);
>          }
> +    } else {
> +        /* timeout reached */
> +        rc = 0;
>      }
>      return rc;
>  }
> @@ -482,7 +519,7 @@ int libxl__devices_destroy(libxl__gc *gc
>          tv.tv_usec = 0;
>          while (n_watches > 0) {
>              if (wait_for_dev_destroy(gc, &tv)) {
> -                break;
> +                continue;
>              } else {
>                  n_watches--;
>              }

This looks like an independent bug fix?

> diff -r 00949e363f6f -r aff396042176 tools/libxl/libxl_osdeps.h
> --- a/tools/libxl/libxl_osdeps.h      Fri Sep 16 17:29:45 2011 +0200
> +++ b/tools/libxl/libxl_osdeps.h      Fri Sep 16 17:36:02 2011 +0200
> @@ -25,6 +25,7 @@
>  
>  #if defined(__NetBSD__) || defined(__OpenBSD__)
>  #include <util.h>
> +#define HAVE_PHY_BACKEND_FILE_SUPPORT
>  #elif defined(__linux__)
>  #include <pty.h>
>  #elif defined(__sun__)
> 
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@xxxxxxxxxxxxxxxxxxx
> http://lists.xensource.com/xen-devel



_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxx
http://lists.xensource.com/xen-devel