[PATCH 0 of 2] libxl_pci: verify device is assignable, rename misleading function (v2)

[PATCH 0 of 2] libxl_pci: verify device is assignable, rename misleading function (v2)

[Doug Magee]

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.

[PATCH 1 of 2] libxl_pci: rename is_assigned to is_pcidev_in_array

[Doug Magee]

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;

[PATCH 2 of 2] libxl_pci: verify device is assignable before adding to domain

[Doug Magee]

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;
 }

Re: [PATCH 2 of 2] libxl_pci: verify device is assignable before adding to domain

[Ian Jackson]

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.

Re: [PATCH 2 of 2] libxl_pci: verify device is assignable before adding to domain

[Ian Jackson]

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.