[PATCH 0/3] Misc patches for libxl_device_disk functions

[PATCH 0/3] Misc patches for libxl_device_disk functions

[Wei Liu]

The first two patches are cleaning up and refactoring.

The third patch fixes a bug discovered by Jim's libvirt test case.

Wei.

Wei Liu (3):
  libxl, xl: don't init/dispose when not necessary
  libxl: factor out libxl__disk_backend_from_xs_be
  libxl: libxl__device_from_disk should retrieve backend from xenstore

 tools/libxl/libxl.c      | 58 +++++++++++++++++++++++++++++++++++-------------
 tools/libxl/libxl.h      |  7 ++++++
 tools/libxl/xl_cmdimpl.c |  1 +
 3 files changed, 50 insertions(+), 16 deletions(-)

-- 
1.9.1

[PATCH 1/3] libxl, xl: don't init/dispose when not necessary

[Wei Liu]

Functions like libxl__device_disk_from_xs_be and
libxl_vdev_to_device_disk should not touch the disk struct passed in.
It's caller's responsibility to do that.

This leads to some changes to the caller to prepare the disk struct.

Note that libxl_vdev_to_device_disk is a public API so a macro is
defined in libxl.h to indicate the change of behaviour.

Signed-off-by: Wei Liu <wei.liu2@citrix.com>
Cc: Ian Campbell <ian.campbell@citrix.com>
Cc: Ian Jackson <ian.jackson@eu.citrix.com>
---
 tools/libxl/libxl.c      | 6 +-----
 tools/libxl/libxl.h      | 7 +++++++
 tools/libxl/xl_cmdimpl.c | 1 +
 3 files changed, 9 insertions(+), 5 deletions(-)

diff --git a/tools/libxl/libxl.c b/tools/libxl/libxl.c
index cd6f42c..ef48550 100644
--- a/tools/libxl/libxl.c
+++ b/tools/libxl/libxl.c
@@ -2556,8 +2556,6 @@ static int libxl__device_disk_from_xs_be(libxl__gc *gc,
     char *tmp;
     int rc;
 
-    libxl_device_disk_init(disk);
-
     rc = sscanf(be_path, "/local/domain/%d/", &disk->backend_domid);
     if (rc != 1) {
         LOG(ERROR, "Unable to fetch device backend domid from %s", be_path);
@@ -2620,7 +2618,6 @@ static int libxl__device_disk_from_xs_be(libxl__gc *gc,
 
     return 0;
 cleanup:
-    libxl_device_disk_dispose(disk);
     return ERROR_FAIL;
 }
 
@@ -2635,8 +2632,6 @@ int libxl_vdev_to_device_disk(libxl_ctx *ctx, uint32_t domid,
     if (devid < 0)
         return ERROR_INVAL;
 
-    libxl_device_disk_init(disk);
-
     dompath = libxl__xs_get_dompath(gc, domid);
     if (!dompath) {
         goto out;
@@ -2675,6 +2670,7 @@ static int libxl__append_disk_list_of_type(libxl__gc *gc,
         tmp = realloc(*disks, sizeof (libxl_device_disk) * (*ndisks + n));
         if (tmp == NULL)
             return ERROR_NOMEM;
+        libxl_device_disk_init(tmp);
         *disks = tmp;
         pdisk = *disks + initial_disks;
         pdisk_end = *disks + initial_disks + n;
diff --git a/tools/libxl/libxl.h b/tools/libxl/libxl.h
index c219f59..8f6ed3c 100644
--- a/tools/libxl/libxl.h
+++ b/tools/libxl/libxl.h
@@ -135,6 +135,13 @@
 #define LIBXL_HAVE_LIBXL_DEVICE_DISK_DISCARD_ENABLE 1
 
 /*
+ * The caller of libxl_vdev_to_device_disk should call
+ * libxl_device_disk_init before passing in the disk struct because
+ * libxl_vdev_to_device_disk doesn't do that anymore.
+ */
+#define LIBXL_VDEV_TO_DEVICE_DISK_NO_INIT 1
+
+/*
  * LIBXL_HAVE_BUILDINFO_IOMEM_START_GFN indicates that it is possible
  * to specify the start guest frame number used to map a range of I/O
  * memory machine frame numbers via the 'gfn' field (of type uint64)
diff --git a/tools/libxl/xl_cmdimpl.c b/tools/libxl/xl_cmdimpl.c
index b7eac29..ef101c3 100644
--- a/tools/libxl/xl_cmdimpl.c
+++ b/tools/libxl/xl_cmdimpl.c
@@ -6411,6 +6411,7 @@ int main_blockdetach(int argc, char **argv)
     }
 
     domid = find_domain(argv[optind]);
+    libxl_device_disk_init(&disk);
 
     if (libxl_vdev_to_device_disk(ctx, domid, argv[optind+1], &disk)) {
         fprintf(stderr, "Error: Device %s not connected.\n", argv[optind+1]);
-- 
1.9.1

[PATCH 2/3] libxl: factor out libxl__disk_backend_from_xs_be

[Wei Liu]

Signed-off-by: Wei Liu <wei.liu2@citrix.com>
Cc: Ian Campbell <ian.campbell@citrix.com>
Cc: Ian Jackson <ian.jackson@eu.citrix.com>
---
 tools/libxl/libxl.c | 31 +++++++++++++++++++++++--------
 1 file changed, 23 insertions(+), 8 deletions(-)

diff --git a/tools/libxl/libxl.c b/tools/libxl/libxl.c
index ef48550..01a2d11 100644
--- a/tools/libxl/libxl.c
+++ b/tools/libxl/libxl.c
@@ -2258,6 +2258,27 @@ int libxl__device_disk_setdefault(libxl__gc *gc, libxl_device_disk *disk)
     return rc;
 }
 
+static int libxl__disk_backend_from_xs_be(libxl__gc *gc,
+                                          const char *be_path,
+                                          libxl_device_disk *disk)
+{
+    char *type;
+    int rc = ERROR_FAIL;
+
+    type = libxl__xs_read(gc, XBT_NULL,
+                          libxl__sprintf(gc, "%s/type", be_path));
+    if (!type) {
+        LOG(ERROR, "Missing xenstore node %s/type", be_path);
+        goto out;
+    }
+
+    libxl_string_to_backend(CTX, type, &disk->backend);
+
+    rc = 0;
+out:
+    return rc;
+}
+
 int libxl__device_from_disk(libxl__gc *gc, uint32_t domid,
                                    libxl_device_disk *disk,
                                    libxl__device *device)
@@ -2572,14 +2593,8 @@ static int libxl__device_disk_from_xs_be(libxl__gc *gc,
         disk->pdev_path = tmp;
     }
 
-
-    tmp = libxl__xs_read(gc, XBT_NULL,
-                         libxl__sprintf(gc, "%s/type", be_path));
-    if (!tmp) {
-        LOG(ERROR, "Missing xenstore node %s/type", be_path);
-        goto cleanup;
-    }
-    libxl_string_to_backend(ctx, tmp, &(disk->backend));
+    rc = libxl__disk_backend_from_xs_be(gc, be_path, disk);
+    if (rc) goto cleanup;
 
     disk->vdev = xs_read(ctx->xsh, XBT_NULL,
                          libxl__sprintf(gc, "%s/dev", be_path), &len);
-- 
1.9.1

[PATCH 3/3] libxl: libxl__device_from_disk should retrieve backend from xenstore

[Wei Liu]

... if backend is not set by caller.

Also change the function to use "goto" idiom while I was there.

Signed-off-by: Wei Liu <wei.liu2@citrix.com>
Cc: Ian Campbell <ian.campbell@citrix.com>
Cc: Ian Jackson <ian.jackson@eu.citrix.com>
---
 tools/libxl/libxl.c | 21 ++++++++++++++++++---
 1 file changed, 18 insertions(+), 3 deletions(-)

diff --git a/tools/libxl/libxl.c b/tools/libxl/libxl.c
index 01a2d11..56c2e09 100644
--- a/tools/libxl/libxl.c
+++ b/tools/libxl/libxl.c
@@ -2285,12 +2285,24 @@ int libxl__device_from_disk(libxl__gc *gc, uint32_t domid,
 {
     libxl_ctx *ctx = libxl__gc_owner(gc);
     int devid;
+    int rc;
 
     devid = libxl__device_disk_dev_number(disk->vdev, NULL, NULL);
     if (devid==-1) {
         LIBXL__LOG(ctx, LIBXL__LOG_ERROR, "Invalid or unsupported"
                " virtual disk identifier %s", disk->vdev);
-        return ERROR_INVAL;
+        rc = ERROR_INVAL;
+        goto out;
+    }
+
+    if (disk->backend == LIBXL_DISK_BACKEND_UNKNOWN) {
+        char *be_path;
+
+        be_path = GCSPRINTF("/local/domain/%u/backend/vbd/%u/%d",
+                            disk->backend_domid,
+                            domid, devid);
+        rc = libxl__disk_backend_from_xs_be(gc, be_path, disk);
+        if (rc) goto out;
     }
 
     device->backend_domid = disk->backend_domid;
@@ -2309,14 +2321,17 @@ int libxl__device_from_disk(libxl__gc *gc, uint32_t domid,
         default:
             LIBXL__LOG(ctx, LIBXL__LOG_ERROR, "unrecognized disk backend type: %d\n",
                        disk->backend);
-            return ERROR_INVAL;
+            rc = ERROR_INVAL;
+            goto out;
     }
 
     device->domid = domid;
     device->devid = devid;
     device->kind  = LIBXL__DEVICE_KIND_VBD;
 
-    return 0;
+    rc = 0;
+out:
+    return rc;
 }
 
 /* Specific function called directly only by local disk attach,
-- 
1.9.1

Re: [PATCH 1/3] libxl, xl: don't init/dispose when not necessary

[Wei Liu]

Er... The title should be

"libxl, xl: don't init/dispose when not unnecessary"
                                        ----------

Re: [PATCH 1/3] libxl, xl: don't init/dispose when not necessary

[Wei Liu]

On Mon, Feb 09, 2015 at 01:23:26PM +0000, Wei Liu wrote:
> Er... The title should be
> 
> "libxl, xl: don't init/dispose when not unnecessary"
>                                         ----------

Grrr... What was I thinking. The initial title was correct.

Sorry for the noise and spam.

Re: [PATCH 1/3] libxl, xl: don't init/dispose when not necessary

[Ian Jackson]

Wei Liu writes ("[PATCH 1/3] libxl, xl: don't init/dispose when not necessary"):
> Functions like libxl__device_disk_from_xs_be and
> libxl_vdev_to_device_disk should not touch the disk struct passed in.
> It's caller's responsibility to do that.
...
> Note that libxl_vdev_to_device_disk is a public API so a macro is
> defined in libxl.h to indicate the change of behaviour.

I don't think this is really on, in the public API.

Why not make libxl_..._free idempotent ?  Then extra calls to _init
are harmless.

Ian.

Re: [PATCH 2/3] libxl: factor out libxl__disk_backend_from_xs_be

[Ian Jackson]

Wei Liu writes ("[PATCH 2/3] libxl: factor out libxl__disk_backend_from_xs_be"):
> Signed-off-by: Wei Liu <wei.liu2@citrix.com>

Commit messages should include the phrase "No functional change" where
appropriate.

I think it is in this case.

Thanks,
Ian.

Re: [PATCH 3/3] libxl: libxl__device_from_disk should retrieve backend from xenstore

[Ian Jackson]

Wei Liu writes ("[PATCH 3/3] libxl: libxl__device_from_disk should retrieve backend from xenstore"):
> ... if backend is not set by caller.

Acked-by: Ian Jackson <ian.jackson@eu.citrix.com>

as far as it goes, but I think you may want a more radical change -
see below.

> Also change the function to use "goto" idiom while I was there.

(Although usually it would be better to split this kind of thing into
a pre-patch, in this case it's small and easily reviewed.)

Is the backend type the only missing or potentially-wrong
information ?  ISTM that perhaps the caller might not know the target,
either.

What should happen if the caller specifies a different target in disk
to the one the device is actually using ?  The documentation should
specify which of the fields are important.

Maybe libxl_device_disk_remove needs to call libxl_vdev_to_device_disk
and check that the supplied disk struct is plausible somehow.  In that
case it might be nice for the caller to be able to fill in only the
vdev.

Ian.

Re: [PATCH 1/3] libxl, xl: don't init/dispose when not necessary

[Wei Liu]

On Tue, Feb 10, 2015 at 10:54:47AM +0000, Ian Jackson wrote:
> Wei Liu writes ("[PATCH 1/3] libxl, xl: don't init/dispose when not necessary"):
> > Functions like libxl__device_disk_from_xs_be and
> > libxl_vdev_to_device_disk should not touch the disk struct passed in.
> > It's caller's responsibility to do that.
> ...
> > Note that libxl_vdev_to_device_disk is a public API so a macro is
> > defined in libxl.h to indicate the change of behaviour.
> 
> I don't think this is really on, in the public API.
> 

Yes, it's public API -- not in libxl.h, but in libxl_utils.h. And xl is
using it.

The problem is that calling _init inside libxl_vdev_to_device_disk
violates our pattern of using libxl types.

> Why not make libxl_..._free idempotent ?  Then extra calls to _init
> are harmless.
> 

That would be a good idea and it should be done in a separate patch set.

Strictly speaking this patch is not required to fix the bug Jim spotted.
I will just drop it in this series and rework in while I make _dispose
idempotent.

Wei.

> Ian.

Re: [PATCH 2/3] libxl: factor out libxl__disk_backend_from_xs_be

[Wei Liu]

On Tue, Feb 10, 2015 at 10:56:24AM +0000, Ian Jackson wrote:
> Wei Liu writes ("[PATCH 2/3] libxl: factor out libxl__disk_backend_from_xs_be"):
> > Signed-off-by: Wei Liu <wei.liu2@citrix.com>
> 
> Commit messages should include the phrase "No functional change" where
> appropriate.
> 
> I think it is in this case.
> 

Yes. I will add that to commit message.

Wei.

> Thanks,
> Ian.

Re: [PATCH 3/3] libxl: libxl__device_from_disk should retrieve backend from xenstore

[Wei Liu]

On Tue, Feb 10, 2015 at 11:01:46AM +0000, Ian Jackson wrote:
> Wei Liu writes ("[PATCH 3/3] libxl: libxl__device_from_disk should retrieve backend from xenstore"):
> > ... if backend is not set by caller.
> 
> Acked-by: Ian Jackson <ian.jackson@eu.citrix.com>
> 
> as far as it goes, but I think you may want a more radical change -
> see below.
> 
> > Also change the function to use "goto" idiom while I was there.
> 
> (Although usually it would be better to split this kind of thing into
> a pre-patch, in this case it's small and easily reviewed.)
> 
> Is the backend type the only missing or potentially-wrong
> information ?  ISTM that perhaps the caller might not know the target,
> either.
> 
> What should happen if the caller specifies a different target in disk
> to the one the device is actually using ?  The documentation should
> specify which of the fields are important.
> 

I'm not sure because it's not documented.

We should take a step back to define the important fields first.

> Maybe libxl_device_disk_remove needs to call libxl_vdev_to_device_disk
> and check that the supplied disk struct is plausible somehow.  In that
> case it might be nice for the caller to be able to fill in only the
> vdev.
> 

If so we need to make clear in the documentation. I'm of course fine
with this behaviour.

Jim, does libvirt (as an example of libxl user) actually cares
specifying every fields in that struct? The other user (xl) doesn't seem
to care that much.

Wei.

> Ian.

Re: [PATCH 3/3] libxl: libxl__device_from_disk should retrieve backend from xenstore

[Jim Fehlig]

Wei Liu wrote:
> On Tue, Feb 10, 2015 at 11:01:46AM +0000, Ian Jackson wrote:
>   
>> Wei Liu writes ("[PATCH 3/3] libxl: libxl__device_from_disk should retrieve backend from xenstore"):
>>     
>>> ... if backend is not set by caller.
>>>       
>> Acked-by: Ian Jackson <ian.jackson@eu.citrix.com>
>>
>> as far as it goes, but I think you may want a more radical change -
>> see below.
>>
>>     
>>> Also change the function to use "goto" idiom while I was there.
>>>       
>> (Although usually it would be better to split this kind of thing into
>> a pre-patch, in this case it's small and easily reviewed.)
>>
>> Is the backend type the only missing or potentially-wrong
>> information ?  ISTM that perhaps the caller might not know the target,
>> either.
>>
>> What should happen if the caller specifies a different target in disk
>> to the one the device is actually using ?  The documentation should
>> specify which of the fields are important.
>>
>>     
>
> I'm not sure because it's not documented.
>
> We should take a step back to define the important fields first.
>
>   
>> Maybe libxl_device_disk_remove needs to call libxl_vdev_to_device_disk
>> and check that the supplied disk struct is plausible somehow.  In that
>> case it might be nice for the caller to be able to fill in only the
>> vdev.
>>
>>     
>
> If so we need to make clear in the documentation. I'm of course fine
> with this behaviour.
>
> Jim, does libvirt (as an example of libxl user) actually cares
> specifying every fields in that struct? The other user (xl) doesn't seem
> to care that much.
>   

At minimum, libvirt will populate the pdev_path, vdev, backend, and
format fields. If backend and format (which, in libvirt-speack
correspond to the 'name' and 'type' attributes on the optional <driver>
element) are not specified, they are set to LIBXL_DISK_BACKEND_UNKNOWN
and LIBXL_DISK_FORMAT_RAW respectively.

Regards,
Jim

Re: [PATCH 3/3] libxl: libxl__device_from_disk should retrieve backend from xenstore

[Ian Jackson]

Jim Fehlig writes ("Re: [PATCH 3/3] libxl: libxl__device_from_disk should retrieve backend from xenstore"):
> Wei Liu wrote:
> > On Tue, Feb 10, 2015 at 11:01:46AM +0000, Ian Jackson wrote:
> >> What should happen if the caller specifies a different target in disk
> >> to the one the device is actually using ?  The documentation should
> >> specify which of the fields are important.
> >
> > I'm not sure because it's not documented.

I know :-).  I meant: what should we write in the documentation ?
(And, obviously, implement.)

> > We should take a step back to define the important fields first.

Right.

> >> Maybe libxl_device_disk_remove needs to call libxl_vdev_to_device_disk
> >> and check that the supplied disk struct is plausible somehow.  In that
> >> case it might be nice for the caller to be able to fill in only the
> >> vdev.
> >
> > If so we need to make clear in the documentation. I'm of course fine
> > with this behaviour.

Well, feel free to object if you think my (rather vague) suggestion is
wrong.

> > Jim, does libvirt (as an example of libxl user) actually cares
> > specifying every fields in that struct? The other user (xl) doesn't seem
> > to care that much.
> 
> At minimum, libvirt will populate the pdev_path, vdev, backend, and
> format fields. If backend and format (which, in libvirt-speack
> correspond to the 'name' and 'type' attributes on the optional <driver>
> element) are not specified, they are set to LIBXL_DISK_BACKEND_UNKNOWN
> and LIBXL_DISK_FORMAT_RAW respectively.

I think for fields libvirt has gone to the trouble of specifying,
libxl should check that they match.

Ian.

Re: [PATCH 3/3] libxl: libxl__device_from_disk should retrieve backend from xenstore

[Wei Liu]

On Wed, Feb 11, 2015 at 10:18:18AM -0700, Jim Fehlig wrote:
> Wei Liu wrote:
> > On Tue, Feb 10, 2015 at 11:01:46AM +0000, Ian Jackson wrote:
> >   
> >> Wei Liu writes ("[PATCH 3/3] libxl: libxl__device_from_disk should retrieve backend from xenstore"):
> >>     
> >>> ... if backend is not set by caller.
> >>>       
> >> Acked-by: Ian Jackson <ian.jackson@eu.citrix.com>
> >>
> >> as far as it goes, but I think you may want a more radical change -
> >> see below.
> >>
> >>     
> >>> Also change the function to use "goto" idiom while I was there.
> >>>       
> >> (Although usually it would be better to split this kind of thing into
> >> a pre-patch, in this case it's small and easily reviewed.)
> >>
> >> Is the backend type the only missing or potentially-wrong
> >> information ?  ISTM that perhaps the caller might not know the target,
> >> either.
> >>
> >> What should happen if the caller specifies a different target in disk
> >> to the one the device is actually using ?  The documentation should
> >> specify which of the fields are important.
> >>
> >>     
> >
> > I'm not sure because it's not documented.
> >
> > We should take a step back to define the important fields first.
> >
> >   
> >> Maybe libxl_device_disk_remove needs to call libxl_vdev_to_device_disk
> >> and check that the supplied disk struct is plausible somehow.  In that
> >> case it might be nice for the caller to be able to fill in only the
> >> vdev.
> >>
> >>     
> >
> > If so we need to make clear in the documentation. I'm of course fine
> > with this behaviour.
> >
> > Jim, does libvirt (as an example of libxl user) actually cares
> > specifying every fields in that struct? The other user (xl) doesn't seem
> > to care that much.
> >   
> 
> At minimum, libvirt will populate the pdev_path, vdev, backend, and
> format fields. If backend and format (which, in libvirt-speack
> correspond to the 'name' and 'type' attributes on the optional <driver>
> element) are not specified, they are set to LIBXL_DISK_BACKEND_UNKNOWN
> and LIBXL_DISK_FORMAT_RAW respectively.
> 

Since libvirt has a tendency of specifying everything, how come there is
no "name" and "type" in <driver>? Can we actually generate all the
fields needed when attaching a disk and store in libvirt's diskspec?

Wei.

> Regards,
> Jim

Re: [PATCH 3/3] libxl: libxl__device_from_disk should retrieve backend from xenstore

[Jim Fehlig]

Wei Liu wrote:
> On Wed, Feb 11, 2015 at 10:18:18AM -0700, Jim Fehlig wrote:
>   
>> At minimum, libvirt will populate the pdev_path, vdev, backend, and
>> format fields. If backend and format (which, in libvirt-speack
>> correspond to the 'name' and 'type' attributes on the optional <driver>
>> element) are not specified, they are set to LIBXL_DISK_BACKEND_UNKNOWN
>> and LIBXL_DISK_FORMAT_RAW respectively.
>>
>>     
>
> Since libvirt has a tendency of specifying everything, how come there is
> no "name" and "type" in <driver>?

The <driver> element is optional. From
http://libvirt.org/formatdomain.html#elementsDisks

"|driver: |The optional driver element allows specifying further details
related to the hypervisor driver used to provide the disk"

And when not specified, Ian C. recommended allowing libxl to pick
suitable defaults

https://www.redhat.com/archives/libvir-list/2013-February/msg01126.html

>  Can we actually generate all the
>   
> fields needed when attaching a disk and store in libvirt's diskspec?

Yes, it was this way before the suggested change.

Regards,
Jim

Re: [PATCH 3/3] libxl: libxl__device_from_disk should retrieve backend from xenstore

[Wei Liu]

Sorry for the late reply.

On Thu, Feb 26, 2015 at 04:49:21PM -0700, Jim Fehlig wrote:
> Wei Liu wrote:
> > On Wed, Feb 11, 2015 at 10:18:18AM -0700, Jim Fehlig wrote:
> >   
> >> At minimum, libvirt will populate the pdev_path, vdev, backend, and
> >> format fields. If backend and format (which, in libvirt-speack
> >> correspond to the 'name' and 'type' attributes on the optional <driver>
> >> element) are not specified, they are set to LIBXL_DISK_BACKEND_UNKNOWN
> >> and LIBXL_DISK_FORMAT_RAW respectively.
> >>
> >>     
> >
> > Since libvirt has a tendency of specifying everything, how come there is
> > no "name" and "type" in <driver>?
> 
> The <driver> element is optional. From
> http://libvirt.org/formatdomain.html#elementsDisks
> 
> "|driver: |The optional driver element allows specifying further details
> related to the hypervisor driver used to provide the disk"
> 
> And when not specified, Ian C. recommended allowing libxl to pick
> suitable defaults
> 
> https://www.redhat.com/archives/libvir-list/2013-February/msg01126.html
> 
> >  Can we actually generate all the
> >   
> > fields needed when attaching a disk and store in libvirt's diskspec?
> 
> Yes, it was this way before the suggested change.
> 

I'm now of the opinion that we shouldn't change libxl__device_from_disk
to fill in specific information, otherwise it becomes a special case for
all the libxl__device_from_$foo  functions.

And from the information you provide above it seems to be easily fixable
on libvirt side -- you have all the information at hand when attaching
the disk, so why not just store it for reuse later?

Ian C, what do you think?

Wei.

> Regards,
> Jim