Xen devel - Jan 2012 - [PATCH 0 of 2] 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.

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 3becc1652693 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 10:14:15 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 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 <djmagee@mageenet.net>

diff -r 3becc1652693 -r be1313a6b489 tools/libxl/libxl_pci.c
--- a/tools/libxl/libxl_pci.c	Tue Jan 17 10:14:15 2012 -0500
+++ b/tools/libxl/libxl_pci.c	Tue Jan 17 10:19:24 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;
     }

On Tue, 2012-01-17 at 15:25 +0000, Doug Magee wrote:
> 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.
> Signed-off-by: Doug Magee <djmagee@mageenet.net>
Acked-by: Ian Campbell <ian.campbell@citrix.com>
> 
> diff -r 5b2676ac1321 -r 3becc1652693 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 10:14:15 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;
> 
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xensource.com
> http://lists.xensource.com/xen-devel

On Tue, 2012-01-17 at 15:25 +0000, Doug Magee wrote:
> 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 <djmagee@mageenet.net>
> 
> diff -r 3becc1652693 -r be1313a6b489 tools/libxl/libxl_pci.c
> --- a/tools/libxl/libxl_pci.c	Tue Jan 17 10:14:15 2012 -0500
> +++ b/tools/libxl/libxl_pci.c	Tue Jan 17 10:19:24 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);
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.
> +
> +    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
> Xen-devel@lists.xensource.com
> http://lists.xensource.com/xen-devel

Doug Magee writes ("[PATCH 1 of 2] 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.
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.