* [PATCH 0 of 2] libxl_pci: verify device is assignable, rename misleading function
@ 2012-01-17 15:25 Doug Magee
2012-01-17 15:25 ` [PATCH 1 of 2] libxl_pci: rename is_assigned to is_pcidev_in_array Doug Magee
2012-01-17 15:25 ` [PATCH 2 of 2] libxl_pci: verify device is assignable before adding to a domain Doug Magee
0 siblings, 2 replies; 6+ messages in thread
From: Doug Magee @ 2012-01-17 15:25 UTC (permalink / raw)
To: xen-devel; +Cc: ian.jackson, stefano.stabellini
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.
^ permalink raw reply [flat|nested] 6+ messages in thread
* [PATCH 1 of 2] libxl_pci: rename is_assigned to is_pcidev_in_array
2012-01-17 15:25 [PATCH 0 of 2] libxl_pci: verify device is assignable, rename misleading function Doug Magee
@ 2012-01-17 15:25 ` Doug Magee
2012-01-17 16:38 ` Ian Campbell
2012-01-24 15:37 ` Ian Jackson
2012-01-17 15:25 ` [PATCH 2 of 2] libxl_pci: verify device is assignable before adding to a domain Doug Magee
1 sibling, 2 replies; 6+ messages in thread
From: Doug Magee @ 2012-01-17 15:25 UTC (permalink / raw)
To: xen-devel; +Cc: ian.jackson, stefano.stabellini
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;
^ permalink raw reply [flat|nested] 6+ messages in thread
* [PATCH 2 of 2] libxl_pci: verify device is assignable before adding to a domain
2012-01-17 15:25 [PATCH 0 of 2] libxl_pci: verify device is assignable, rename misleading function Doug Magee
2012-01-17 15:25 ` [PATCH 1 of 2] libxl_pci: rename is_assigned to is_pcidev_in_array Doug Magee
@ 2012-01-17 15:25 ` Doug Magee
2012-01-17 16:41 ` Ian Campbell
1 sibling, 1 reply; 6+ messages in thread
From: Doug Magee @ 2012-01-17 15:25 UTC (permalink / raw)
To: xen-devel; +Cc: ian.jackson, stefano.stabellini
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;
}
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH 1 of 2] libxl_pci: rename is_assigned to is_pcidev_in_array
2012-01-17 15:25 ` [PATCH 1 of 2] libxl_pci: rename is_assigned to is_pcidev_in_array Doug Magee
@ 2012-01-17 16:38 ` Ian Campbell
2012-01-24 15:37 ` Ian Jackson
1 sibling, 0 replies; 6+ messages in thread
From: Ian Campbell @ 2012-01-17 16:38 UTC (permalink / raw)
To: Doug Magee; +Cc: xen-devel@lists.xensource.com, Ian Jackson, Stefano Stabellini
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
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH 2 of 2] libxl_pci: verify device is assignable before adding to a domain
2012-01-17 15:25 ` [PATCH 2 of 2] libxl_pci: verify device is assignable before adding to a domain Doug Magee
@ 2012-01-17 16:41 ` Ian Campbell
0 siblings, 0 replies; 6+ messages in thread
From: Ian Campbell @ 2012-01-17 16:41 UTC (permalink / raw)
To: Doug Magee; +Cc: xen-devel@lists.xensource.com, Ian Jackson, Stefano Stabellini
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
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH 1 of 2] libxl_pci: rename is_assigned to is_pcidev_in_array
2012-01-17 15:25 ` [PATCH 1 of 2] libxl_pci: rename is_assigned to is_pcidev_in_array Doug Magee
2012-01-17 16:38 ` Ian Campbell
@ 2012-01-24 15:37 ` Ian Jackson
1 sibling, 0 replies; 6+ messages in thread
From: Ian Jackson @ 2012-01-24 15:37 UTC (permalink / raw)
To: Doug Magee; +Cc: xen-devel@lists.xensource.com, Stefano Stabellini
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.
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2012-01-24 15:37 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2012-01-17 15:25 [PATCH 0 of 2] libxl_pci: verify device is assignable, rename misleading function Doug Magee
2012-01-17 15:25 ` [PATCH 1 of 2] libxl_pci: rename is_assigned to is_pcidev_in_array Doug Magee
2012-01-17 16:38 ` Ian Campbell
2012-01-24 15:37 ` Ian Jackson
2012-01-17 15:25 ` [PATCH 2 of 2] libxl_pci: verify device is assignable before adding to a domain Doug Magee
2012-01-17 16:41 ` Ian Campbell
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).