Re: [Xen-devel] [PATCH] libxl: added libxl compatibility with physical backend file for NetBSD

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

2011/7/22 Ian Campbell <Ian.Campbell@xxxxxxxxxx>:
> On Fri, 2011-07-22 at 13:38 +0100, Roger Pau Monnà wrote:
>> Hello,
>>
>> This patch fixes the problems described in a previous thread regarding
>> PV machines not booting on NetBSD with libxl toolstack. The patch also
>> prevents libxl from adding PCI entries to xenstore if there is no PCI
>> device configured in the guest.
>
> Thanks Roger.
>
> In general we prefer to see individual fixes split up into separate
> patches for review and commit. Also please can you add to your ~/.hgrc:
> Â Â Â Â[diff]
> Â Â Â Âshowfunc = True
> It prints the function name in the hunk header which makes it easier to
> review.

Ok done, sorry, I didn't know about that.

>
> The change to the hotplug script seems odd, if the "xtype" paramter is
> not needed then just remove it altogether, otherwise we need to teach
> libxl what to write.

The block script needs the parameter xtype because it needs to know if
the vbd is a block device (phy) or a regular file (file). Since libxl
sets the param type based on disk->backend, and it is always
LIBXL_DISK_BACKEND_PHY (even if image is a regular file or a block
device, libxl.c:998), the "type" atribute in the xenstore is no longer
helpful to choose whether the "params" is a block device or a regular
file. That's why I asked to create a new DISK_BACKEND define,
something like VND or LOOP. Or maybe if disk->backend is PHY, use
disk->format to set "type" in xenstore (using RAW for "file" and EMPTY
for "phy").

>
> The PCI change looks good, although you might as well throw the for()
> loop under the check too.
>
> "#ifdef" etc should be in column 0, but the content should be indented
> normally.

Ok, will fix that.

>
> The checks on LIBXL_DISK_FORMAT_EMPTY and S_ISBLK are not really related
> so I would not combine them even though the return is the same.
> Otherwise the HAVE_PHY_BACKEND_FILE_SUPPORT change itself looks ok
>
> What is DONT_REMOVE_VBD_FROM_STORE for? Is this because xenbackendd does
> it for you or something else?

xenbackendd is a little tricky, and can cause trouble regarding
sincronization. libxl and xenbackendd are not synchronized, so
sometimes xenstore entries where removed from the store before
xenbackend had time to read them, and perform the necessary
operations, this resulted in vnd devices not being detached, so I just
disabled the remove of vbd entries from libxl. I think it would be
better to implement some kind of synchronization between libxl and
xenbackend, so the flow would be like:

Stop guest ----> xenbackend: detach devices -----> libxl: remove entries

Don't know about the best way to synchronize this, I can only thing
about synchronizing them using the "hotplug-status" attribute, but
that would mean changing the way libxl__device_destroy works.

>
> Does your changed xenbackendd still work with xend? Oh, I see what the
> hotplug script change was about now -- I guess just remove the commented
> out line?

Yes, I missed that one, I will remove it.

>
> Ian.
>
>

Thanks for the feedback, Roger.

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