Patchwork pcibus_get_dev_path: correct pci device path construction

login
register
mail settings
Submitter Igor V. Kovalenko
Date Jan. 18, 2011, 9:10 p.m.
Message ID <20110118211016.9850.63478.stgit@skyserv>
Download mbox | patch
Permalink /patch/79364/
State New
Headers show

Comments

Igor V. Kovalenko - Jan. 18, 2011, 9:10 p.m.
From: Igor V. Kovalenko <igor.v.kovalenko@gmail.com>

- fix snprintf off by one
  pci domain and slot number formatting snprintf calls
  require extra space for trailing null character

  without this change devices are assigned the same path name
  which triggers assertion in vmstate_register_with_alias_id

- while iterating over devices from root pci device
  use PCI_SLOT and PCI_FUNC of each device on the path
  instead of always extracting PCI_FUNC of original device

Signed-off-by: Igor V. Kovalenko <igor.v.kovalenko@gmail.com>
---
 hw/pci.c |   28 ++++++++++++++++++++--------
 1 files changed, 20 insertions(+), 8 deletions(-)
Mark Cave-Ayland - Jan. 31, 2011, 2:56 p.m.
On 18/01/11 21:10, Igor V. Kovalenko wrote:

> From: Igor V. Kovalenko<igor.v.kovalenko@gmail.com>
>
> - fix snprintf off by one
>    pci domain and slot number formatting snprintf calls
>    require extra space for trailing null character
>
>    without this change devices are assigned the same path name
>    which triggers assertion in vmstate_register_with_alias_id
>
> - while iterating over devices from root pci device
>    use PCI_SLOT and PCI_FUNC of each device on the path
>    instead of always extracting PCI_FUNC of original device
>
> Signed-off-by: Igor V. Kovalenko<igor.v.kovalenko@gmail.com>
> ---
>   hw/pci.c |   28 ++++++++++++++++++++--------
>   1 files changed, 20 insertions(+), 8 deletions(-)
>
> diff --git a/hw/pci.c b/hw/pci.c
> index 8d0e3df..182ee25 100644
> --- a/hw/pci.c
> +++ b/hw/pci.c
> @@ -2036,6 +2036,8 @@ static char *pcibus_get_dev_path(DeviceState *dev)
>       int slot_len = strlen(":SS.F");
>       int path_len;
>       char *path, *p;
> +    PCIDevice** pci_path;
> +    int i;
>
>       /* Calculate # of slots on path between device and root. */;
>       slot_depth = 0;
> @@ -2045,21 +2047,31 @@ static char *pcibus_get_dev_path(DeviceState *dev)
>
>       path_len = domain_len + slot_len * slot_depth;
>
> -    /* Allocate memory, fill in the terminating null byte. */
> +    /* Allocate memory. String will be null-terminated by snprintf calls. */
>       path = malloc(path_len + 1 /* For '\0' */);
> -    path[path_len] = '\0';
>
>       /* First field is the domain. */
> -    snprintf(path, domain_len, "%04x:00", pci_find_domain(d->bus));
> +    snprintf(path, domain_len+1, "%04x:00", pci_find_domain(d->bus));
> +
> +    /* Store pci devices on the path walking up from device to root.
> +     * We need them later in the reverse order, last to first. */
> +    pci_path = qemu_malloc(slot_depth * sizeof(PCIDevice *));
>
> -    /* Fill in slot numbers. We walk up from device to root, so need to print
> -     * them in the reverse order, last to first. */
> -    p = path + path_len;
> +    i = slot_depth;
>       for (t = d; t; t = t->bus->parent_dev) {
> -        p -= slot_len;
> -        snprintf(p, slot_len, ":%02x.%x", PCI_SLOT(t->devfn), PCI_FUNC(d->devfn));
> +        pci_path[--i] = t;
>       }
>
> +    /* Fill in slot numbers using stored path from root pci device. */
> +    p = path + domain_len;
> +    for (i = 0; i<  slot_depth; ++i) {
> +        t = pci_path[i];
> +        snprintf(p + i * slot_len, slot_len+1,
> +                 ":%02x.%x", PCI_SLOT(t->devfn), PCI_FUNC(t->devfn));
> +    }
> +
> +    qemu_free(pci_path);
> +
>       return path;
>   }

Has anyone had a chance to look at this patch yet?


ATB,

Mark.
Igor V. Kovalenko - Jan. 31, 2011, 7:01 p.m.
On Mon, Jan 31, 2011 at 5:56 PM, Mark Cave-Ayland
<mark.cave-ayland@siriusit.co.uk> wrote:
> On 18/01/11 21:10, Igor V. Kovalenko wrote:
>
>> From: Igor V. Kovalenko<igor.v.kovalenko@gmail.com>
>>
>> - fix snprintf off by one
>>   pci domain and slot number formatting snprintf calls
>>   require extra space for trailing null character
>>
>>   without this change devices are assigned the same path name
>>   which triggers assertion in vmstate_register_with_alias_id
>>
>> - while iterating over devices from root pci device
>>   use PCI_SLOT and PCI_FUNC of each device on the path
>>   instead of always extracting PCI_FUNC of original device
>>
>> Signed-off-by: Igor V. Kovalenko<igor.v.kovalenko@gmail.com>
>> ---
>>  hw/pci.c |   28 ++++++++++++++++++++--------
>>  1 files changed, 20 insertions(+), 8 deletions(-)
>>
>
> Has anyone had a chance to look at this patch yet?
>

The issue is already fixed in the tree differently by
commits e10990c3f0c39e92ab5f74004b89a24fcc36fa14
and 2991181aaa026d8b1444bfaa9c4bcd82065ba5a3

Patch

diff --git a/hw/pci.c b/hw/pci.c
index 8d0e3df..182ee25 100644
--- a/hw/pci.c
+++ b/hw/pci.c
@@ -2036,6 +2036,8 @@  static char *pcibus_get_dev_path(DeviceState *dev)
     int slot_len = strlen(":SS.F");
     int path_len;
     char *path, *p;
+    PCIDevice** pci_path;
+    int i;
 
     /* Calculate # of slots on path between device and root. */;
     slot_depth = 0;
@@ -2045,21 +2047,31 @@  static char *pcibus_get_dev_path(DeviceState *dev)
 
     path_len = domain_len + slot_len * slot_depth;
 
-    /* Allocate memory, fill in the terminating null byte. */
+    /* Allocate memory. String will be null-terminated by snprintf calls. */
     path = malloc(path_len + 1 /* For '\0' */);
-    path[path_len] = '\0';
 
     /* First field is the domain. */
-    snprintf(path, domain_len, "%04x:00", pci_find_domain(d->bus));
+    snprintf(path, domain_len+1, "%04x:00", pci_find_domain(d->bus));
+
+    /* Store pci devices on the path walking up from device to root.
+     * We need them later in the reverse order, last to first. */
+    pci_path = qemu_malloc(slot_depth * sizeof(PCIDevice *));
 
-    /* Fill in slot numbers. We walk up from device to root, so need to print
-     * them in the reverse order, last to first. */
-    p = path + path_len;
+    i = slot_depth;
     for (t = d; t; t = t->bus->parent_dev) {
-        p -= slot_len;
-        snprintf(p, slot_len, ":%02x.%x", PCI_SLOT(t->devfn), PCI_FUNC(d->devfn));
+        pci_path[--i] = t;
     }
 
+    /* Fill in slot numbers using stored path from root pci device. */
+    p = path + domain_len;
+    for (i = 0; i < slot_depth; ++i) {
+        t = pci_path[i];
+        snprintf(p + i * slot_len, slot_len+1,
+                 ":%02x.%x", PCI_SLOT(t->devfn), PCI_FUNC(t->devfn));
+    }
+
+    qemu_free(pci_path);
+
     return path;
 }