Re: [Xen-devel] [PATCH] libxl: add support for booting PV domains from NetBSD using raw files as disks. Fixed the shutdown race problem by checking "hotplug-status" instead of "state" xenstore variable in NetBSD

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

2011/9/15 Ian Campbell <Ian.Campbell@xxxxxxxxxx>:
> On Thu, 2011-09-15 at 09:03 -0400, Roger Pau Monne wrote:
>> # HG changeset patch
>> # User Roger Pau Monne <roger.pau@xxxxxxxxxxxxx>
>> # Date 1316091721 -7200
>> # Node ID 015617579cd36fc58318aaf350bec5f7cc07ef2f
>> # Parent Â63e254468d6e8d81baeafaed68f05791dc21eb4e
>> libxl: add support for booting PV domains from NetBSD using raw files
>> as disks. Fixed the shutdown race problem by checking "hotplug-status"
>> instead of "state" xenstore variable in NetBSD.
>
> This was one long line which mercurial will use as a summary, it's a
> good idea to make sure that the first line stands somewhat alone as a
> summary so the e.g. "hg log" is useful.
> Also this sounds on the face of it like two bugfixes, if that's the case
> they should be submitted separately.

Well, it's not a bugfix, because NetBSD was not supported by libxl, so
I think it's just part of the patch to add support for NetBSD to
libxl. I will split the line, and the patch if it's necessary, but one
doesn't make sense without the other, and I don't really know what
would be in each patch.

>>
>> Signed-off-by: Roger Pau Monne <roger.pau@xxxxxxxxxxxxx>
>>
>> diff -r 63e254468d6e -r 015617579cd3 tools/hotplug/NetBSD/block
>> --- a/tools/hotplug/NetBSD/block   ÂWed Sep 14 14:18:40 2011 +0200
>> +++ b/tools/hotplug/NetBSD/block   ÂThu Sep 15 15:02:01 2011 +0200
>> @@ -19,7 +19,7 @@ error() {
>>
>> Âxpath=$1
>> Âxstatus=$2
>> -xtype=$(xenstore-read "$xpath/type")
>> +xtype=$3
>> Âxparams=$(xenstore-read "$xpath/params")
>>
>> Âcase $xstatus in
>> @@ -38,6 +38,8 @@ 6)
>> Â Â Â Â Â Â Â echo "unknown type $xtype" >&2
>> Â Â Â Â Â Â Â ;;
>> Â Â Â esac
>> + Â Â echo xenstore-write $xpath/hotplug-status disconnected
>
> leftover debugging?

The block NetBSD script contains some echos, so I feel it would be
interesting to add this one too.

>> + Â Â xenstore-write $xpath/hotplug-status disconnected
>> Â Â Â xenstore-rm $xpath
>> Â Â Â exit 0
>> Â Â Â ;;
>> Â Â Â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;
>> +
>> +#ifdef HAVE_PHY_BACKEND_FILE_SUPPORT
>> + Â Âif (!strstr(be_path, "vbd")) {
>> + Â Â Â Â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;
>> + Â Â Â Â}
>> + Â Â}
>
> Do the other backend types also write this node? It looks like Linux
> does, at least in some circumstances (tap and vbd AFAICT), in which case
> perhaps this is suitable as the only test here (i.e. drop the #ifdef and
> the #else case). That would remove a lot of the conditional code in this
> patch.

If Linux also uses this, I will have to modify the Linux block script,
so that it sets hotplug-status to "disconnected" also. This will be
nice, I don't like code with #ifdefs as it tends to get messy and it's
not easy to understand.

Regards, Roger.

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