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

[Ian Jackson <Ian.Jackson@xxxxxxxxxxxxx>]

Roger Pau Monne writes ("[Xen-devel] [PATCH 2 of 2] libxl: add support for 
booting PV domains from NetBSD using raw files as disks"):
> libxl: add support for booting PV domains from NetBSD using raw files as 
> disks.

Thanks, I have some comments:

> +        if (S_ISBLK(a->stab.st_mode))
> +                return backend;
> +#ifdef HAVE_PHY_BACKEND_FILE_SUPPORT
> +        if (S_ISREG(a->stab.st_mode))
> +            return backend;

I think we would prefer not to have #ifdefs in the code.  That can
make the logic quite hard to follow.  Instead, invent a helper
function which answers the "does the phy backend support files" which
is provided in two versions, from osdep.c.

> @@ -366,14 +371,26 @@ int libxl__device_destroy(libxl__gc *gc,
>      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);

We want to get away from the hotplug scripts for disks at least on
Linux with libxl.  Rather, any scripts that are needed should be run
from libxl directly.

How does that fit with NetBSD's disk backend approach ?
What goes wrong on NetBSD without this additional code ?

> @@ -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--;
>              }

I'm not sure I understand this change, or why it's needed.

Ian.

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