Xen devel - Jan 2012 - [PATCH 0 of 2] libxl_pci: verify device is assignable, rename misleading function (v2)

This series contains one patch that changes the behavior of 
libxl__device_pci_add to verify the device is properly assignable. 
There''s also
a patch that renames a function so it''s not misleading.

This second version explicitly frees the values returned by 
libxl_device_pci_list_assignable.

All this function does is check to see if a device is in an array of pcidevs
passed by the caller.  The function name can be misleading if ever used to check
against a list of devices other than those assigned to a domain.

Signed-off-by: Doug Magee <djmagee@mageenet.net>

diff -r 5b2676ac1321 -r eb59afed10a6 tools/libxl/libxl_pci.c
--- a/tools/libxl/libxl_pci.c	Mon Jan 09 16:01:44 2012 +0100
+++ b/tools/libxl/libxl_pci.c	Tue Jan 17 12:20:09 2012 -0500
@@ -461,7 +461,7 @@ static int get_all_assigned_devices(libx
     return 0;
 }
 
-static int is_assigned(libxl_device_pci *assigned, int num_assigned,
+static int is_pcidev_in_array(libxl_device_pci *assigned, int num_assigned,
                        int dom, int bus, int dev, int func)
 {
     int i;
@@ -510,7 +510,7 @@ libxl_device_pci *libxl_device_pci_list_
         if ( sscanf(de->d_name, PCI_BDF, &dom, &bus, &dev,
&func) != 4 )
             continue;
 
-        if ( is_assigned(assigned, num_assigned, dom, bus, dev, func) )
+        if ( is_pcidev_in_array(assigned, num_assigned, dom, bus, dev, func) )
             continue;
 
         new = realloc(pcidevs, ((*num) + 1) * sizeof(*new));
@@ -802,7 +802,7 @@ int libxl__device_pci_add(libxl__gc *gc,
         LIBXL__LOG(ctx, LIBXL__LOG_ERROR, "cannot determine if device is
assigned, refusing to continue");
         goto out;
     }
-    if ( is_assigned(assigned, num_assigned, pcidev->domain,
+    if ( is_pcidev_in_array(assigned, num_assigned, pcidev->domain,
                      pcidev->bus, pcidev->dev, pcidev->func) ) {
         LIBXL__LOG(ctx, LIBXL__LOG_ERROR, "PCI device already attached to
a domain");
         rc = ERROR_FAIL;
@@ -906,7 +906,7 @@ static int do_pci_remove(libxl__gc *gc, 
         return ERROR_FAIL;
 
     rc = ERROR_INVAL;
-    if ( !is_assigned(assigned, num, pcidev->domain,
+    if ( !is_pcidev_in_array(assigned, num, pcidev->domain,
                       pcidev->bus, pcidev->dev, pcidev->func) ) {
         LIBXL__LOG(ctx, LIBXL__LOG_ERROR, "PCI device not attached to this
domain");
         goto out_fail;

Previously, libxl__device_pci_add only checked the device to be added against
the list of currently assigned devices.  This patch changes the behavior to
check that the device to be assigned is in the list of assignable devices,
which only includes those owned by pciback and not assigned to another domain.

Signed-off-by: Doug Magee <djmagee@mageenet.net>

diff -r eb59afed10a6 -r 6a3f270992eb tools/libxl/libxl_pci.c
--- a/tools/libxl/libxl_pci.c	Tue Jan 17 12:20:09 2012 -0500
+++ b/tools/libxl/libxl_pci.c	Tue Jan 17 12:24:07 2012 -0500
@@ -793,18 +793,15 @@ int libxl__device_pci_add(libxl__gc *gc,
 {
     libxl_ctx *ctx = libxl__gc_owner(gc);
     unsigned int orig_vdev, pfunc_mask;
-    libxl_device_pci *assigned;
-    int num_assigned, i, rc;
+    libxl_device_pci *assignable;
+    int num_assignable, i, rc;
     int stubdomid = 0;
 
-    rc = get_all_assigned_devices(gc, &assigned, &num_assigned);
-    if ( rc ) {
-        LIBXL__LOG(ctx, LIBXL__LOG_ERROR, "cannot determine if device is
assigned, refusing to continue");
-        goto out;
-    }
-    if ( is_pcidev_in_array(assigned, num_assigned, pcidev->domain,
+    assignable = libxl_device_pci_list_assignable(ctx, &num_assignable);
+
+    if ( !is_pcidev_in_array(assignable, num_assignable, pcidev->domain,
                      pcidev->bus, pcidev->dev, pcidev->func) ) {
-        LIBXL__LOG(ctx, LIBXL__LOG_ERROR, "PCI device already attached to
a domain");
+        LIBXL__LOG(ctx, LIBXL__LOG_ERROR, "PCI device is either assigned
to another domain, or not owned by pciback");
         rc = ERROR_FAIL;
         goto out;
     }
@@ -856,6 +853,9 @@ int libxl__device_pci_add(libxl__gc *gc,
     }
 
 out:
+    for ( i = 0; i < num_assignable; i++ )
+        libxl_device_pci_dispose(&assignable[i]);
+    free(assignable);
     return rc;
 }

Doug Magee writes ("[PATCH 2 of 2] libxl_pci: verify device is assignable
before adding to domain"):
> Previously, libxl__device_pci_add only checked the device to be added
against
> the list of currently assigned devices.  This patch changes the behavior to
> check that the device to be assigned is in the list of assignable devices,
> which only includes those owned by pciback and not assigned to another
domain.
...
> +    libxl_device_pci *assignable;
> +    int num_assignable, i, rc;
...
> +    assignable = libxl_device_pci_list_assignable(ctx,
&num_assignable);
> +
What about failure ?  libxl_device_pci_list_assignable might return
NULL on failure, I think.  So you need to add a check for that.

And in that case it won''t assign to num_assignable either, leading to
an uninitialised variable use and possible crash in out.  I think you
should initialise assignable and num_assignable to 0 to avoid this.
> -        LIBXL__LOG(ctx, LIBXL__LOG_ERROR, "PCI device already
attached to a domain");
> +        LIBXL__LOG(ctx, LIBXL__LOG_ERROR, "PCI device is either
assigned to another domain, or not owned by pciback");
While you''re here, would you please wrap this line to within 75-80
columns ?

Thanks,
Ian.

Ian Jackson writes ("Re: [Xen-devel] [PATCH 2 of 2] libxl_pci: verify
device is assignable before adding to domain"):
> Doug Magee writes ("[PATCH 2 of 2] libxl_pci: verify device is
assignable before adding to domain"):
> > Previously, libxl__device_pci_add only checked the device to be added
against
> > the list of currently assigned devices.  This patch changes the
behavior to
> > check that the device to be assigned is in the list of assignable
devices,
> > which only includes those owned by pciback and not assigned to another
domain.
> ...
> > +    libxl_device_pci *assignable;
> > +    int num_assignable, i, rc;
> ...
> > +    assignable = libxl_device_pci_list_assignable(ctx,
&num_assignable);
> > +
> 
> What about failure ?  libxl_device_pci_list_assignable might return
> NULL on failure, I think.  So you need to add a check for that.
Yes.
> And in that case it won''t assign to num_assignable either, leading
to
> an uninitialised variable use and possible crash in out.  I think you
> should initialise assignable and num_assignable to 0 to avoid this.
Yes.

Thanks,
Ian.