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

[Roger Pau Monnà <roger.pau@xxxxxxxxxxxxx>]

2011/9/27 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:

This is an old version of the patch, I've split this change across
several patches, that are on the list also, and include more exact
descriptions of every change.

>
>> + Â Â Â Â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.

Ok, I don't like ifdefs also.

>
>> @@ -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 ?

NetBSD still uses the "loop" ("vnd" on NetBSD) device for files,
because it doesn't have support for qdisk or blktap. If we don't check
the hotplug-status before removing the vbd from xenstore (and only
look at state) it might be removed before the hotplug scripts are
executed, and the disk is never unmounted. This is why we need to
check the hotplug-status before removing vbd from xenstore. Of course,
I could call the hotplug scripts from libxl directly (for disk and
inet interfaces), and we could get rid of xenbackendd.

>
>> @@ -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.

This change is more explained in the series, basically libxl was not
waiting for all devices to disconnect, because when it returned from
wait_for_dev_destroy exited the loop immediately, even if we where
watching for more than one device to disconnect.

>
> Ian.
>
> _______________________________________________
> 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