libxl_pci: verify device is assignable, rename misleading function

PATCH New Doug Magee 3 participants 6 messages Jan 17, 2012 – Jan 24, 2012

Share & tools

Doug Magee

author

2012-01-17 15:25 UTC #0 lore raw

PATCH 1/1 libxl_pci: verify device is assignable, rename misleading function

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.

Doug Magee

author

2012-01-17 15:25 UTC #1 lore raw

PATCH 1/1 libxl_pci: rename is_assigned to is_pcidev_in_array

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 <[email protected]>

tools/libxl/libxl_pci.c +4 -4

diff -r 5b2676ac1321 -r 3becc1652693 tools/libxl/libxl_pci.c
@@ -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;

Ian Campbell

Acked-by on v1 1/1

2012-01-17 16:38 UTC #2 lore raw

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

Quoted 1 line · 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.

In the future please try and wrap your commit messages to <80 columns but otherwise looks good, thanks.

Quoted 1 line

Signed-off-by Doug Magee <[email protected]>

Acked-by Ian Campbell <[email protected]>

Quoted 45 lines

diff -r 5b2676ac1321 -r 3becc1652693 tools/libxl/libxl_pci.c

tools/libxl/libxl_pci.c +4 -4

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

_______________________________________________
Xen-devel mailing list
[email protected]
http://lists.xensource.com/xen-devel

Ian Jackson

2012-01-24 15:37 UTC #3 lore raw

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

In reply to Doug Magee

Doug Magee writes ("[PATCH 1 of 2] libxl_pci: rename is_assigned to is_pcidev_in_array"):

Quoted 1 line

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.

This is a good change and I have committed it, but I think it would be good to change the name of the formal parameter to is_pcidev_in_array, too, to "array" or "devices" or "haystack" or something. And a corresponding change to num_assigned.

Thanks,
Ian.

Doug Magee

author

2012-01-17 15:25 UTC #4 lore raw

PATCH 1/1 libxl_pci: verify device is assignable before adding to a domain

In reply to Doug Magee

Previously, libxl__device_pci_add only checks that the device is not assigned to another domain. This patch updates this function to check against the list of assignable devices, which only includes devices owned by pciback and already excludes devices assigned to other domains.

Signed-off-by Doug Magee <[email protected]>

tools/libxl/libxl_pci.c +6 -9

diff -r 3becc1652693 -r be1313a6b489 tools/libxl/libxl_pci.c
@@ -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;
     }

Ian Campbell

2012-01-17 16:41 UTC #5 lore raw

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

Quoted 14 lines · Doug Magee

Previously, libxl__device_pci_add only checks that the device is not assigned to another domain. This patch updates this function to check against the list of assignable devices, which only includes devices owned by pciback and already excludes devices assigned to other domains.
Signed-off-by Doug Magee <[email protected]>
diff -r 3becc1652693 -r be1313a6b489 tools/libxl/libxl_pci.c
tools/libxl/libxl_pci.c +1 -2
@@ -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;

tools/libxl/libxl_pci.c int libxl__device_pci_add(libxl__gc *gc, · line 800

Quoted 10 lines

tools/libxl/libxl_pci.c +2 -6

+    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);

Unlike get_all_assigned_devices you need to free the returned assignable array explicitly (since it is in a different category per the comment near the top of libxl.h).

Ian.

Quoted 13 lines

tools/libxl/libxl_pci.c +3 -1

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

_______________________________________________
Xen-devel mailing list
[email protected]
http://lists.xensource.com/xen-devel