[Qemu-devel] [PATCH] check for available room when formatting OpenFirmware device path

[Qemu-devel] [PATCH] check for available room when formatting OpenFirmware device path

[Laszlo Ersek]

Signed-off-by: Laszlo Ersek <lersek@redhat.com>
---
 hw/qdev.c |   14 +++++++++++++-
 vl.c      |    7 ++++++-
 2 files changed, 19 insertions(+), 2 deletions(-)

diff --git a/hw/qdev.c b/hw/qdev.c
index af54467..f1e83a4 100644
--- a/hw/qdev.c
+++ b/hw/qdev.c
@@ -502,6 +502,10 @@ static int qdev_get_fw_dev_path_helper(DeviceState *dev, char *p, int size)
     if (dev && dev->parent_bus) {
         char *d;
         l = qdev_get_fw_dev_path_helper(dev->parent_bus->parent, p, size);
+        if (l >= size) {
+            return l;
+        }
+
         d = bus_get_fw_dev_path(dev->parent_bus, dev);
         if (d) {
             l += snprintf(p + l, size - l, "%s", d);
@@ -509,6 +513,10 @@ static int qdev_get_fw_dev_path_helper(DeviceState *dev, char *p, int size)
         } else {
             l += snprintf(p + l, size - l, "%s", object_get_typename(OBJECT(dev)));
         }
+
+        if (l >= size) {
+            return l;
+        }
     }
     l += snprintf(p + l , size - l, "/");
 
@@ -520,8 +528,12 @@ char* qdev_get_fw_dev_path(DeviceState *dev)
     char path[128];
     int l;
 
-    l = qdev_get_fw_dev_path_helper(dev, path, 128);
+    l = qdev_get_fw_dev_path_helper(dev, path, sizeof(path));
 
+    assert(l > 0);
+    if (l >= sizeof(path)) {
+        return NULL;
+    }
     path[l-1] = '\0';
 
     return strdup(path);
diff --git a/vl.c b/vl.c
index 8904db1..78dcc93 100644
--- a/vl.c
+++ b/vl.c
@@ -913,7 +913,12 @@ char *get_boot_devices_list(uint32_t *size)
 
         if (i->dev) {
             devpath = qdev_get_fw_dev_path(i->dev);
-            assert(devpath);
+            if (devpath == NULL) {
+                fprintf(stderr,
+                        "OpenFirmware Device Path too long (boot index %d)\n",
+                        i->bootindex);
+                exit(1);
+            }
         }
 
         if (i->suffix && devpath) {
-- 
1.7.1

Re: [Qemu-devel] [PATCH] check for available room when formatting OpenFirmware device path

[Peter Maydell]

On 23 July 2012 12:33, Laszlo Ersek <lersek@redhat.com> wrote:
>
> Signed-off-by: Laszlo Ersek <lersek@redhat.com>

I think it would be much nicer to just rewrite qdev_get_fw_dev_path
so we weren't trying to fill the path into a fixed string buffer
at all. Here is an entirely untested implementation:

char *qdev_get_fw_dev_path(DeviceState *dev)
{
    char *path;
    char **strarray;
    int depth = 0;
    DeviceState *d = dev;
    for (d = dev; d && d->parent_bus; d = d->parent_bus->parent) {
        depth++;
    }
    depth++;
    strarray = g_new(char*, depth);
    for (d = dev; d && d->parent_bus; d = d->parent_bus->parent) {
        depth--;
        strarray[depth] = bus_get_fw_dev_path(dev->parent_bus, dev);
        if (!strarray[depth]) {
            strarray[depth] = g_strdup(object_get_typename(OBJECT(dev)));
        }
    }
    strarray[0] = g_strdup("");
    path = g_strjoinv("/", strarray);
    g_strfreev(strarray);
    return path;
}

Bonus extra patch: check that all the implementations of get_fw_dev_path()
are returning a g_malloc'd string rather than a plain malloc'd one
(both this code and the current implementation assume so but at least
the scsi bus implementation doesn't use the glib functions.)

-- PMM

Re: [Qemu-devel] [PATCH] check for available room when formatting OpenFirmware device path

[Peter Maydell]

On 23 July 2012 13:34, Peter Maydell <peter.maydell@linaro.org> wrote:
> On 23 July 2012 12:33, Laszlo Ersek <lersek@redhat.com> wrote:
>>
>> Signed-off-by: Laszlo Ersek <lersek@redhat.com>
>
> I think it would be much nicer to just rewrite qdev_get_fw_dev_path
> so we weren't trying to fill the path into a fixed string buffer
> at all. Here is an entirely untested implementation:
>
> char *qdev_get_fw_dev_path(DeviceState *dev)
> {
>     char *path;
>     char **strarray;
>     int depth = 0;
>     DeviceState *d = dev;
>     for (d = dev; d && d->parent_bus; d = d->parent_bus->parent) {
>         depth++;
>     }
>     depth++;
>     strarray = g_new(char*, depth);
>     for (d = dev; d && d->parent_bus; d = d->parent_bus->parent) {
>         depth--;
>         strarray[depth] = bus_get_fw_dev_path(dev->parent_bus, dev);

"d" not "dev" here and in the line below, obviously. I said it was
untested :-)

>         if (!strarray[depth]) {
>             strarray[depth] = g_strdup(object_get_typename(OBJECT(dev)));
>         }
>     }
>     strarray[0] = g_strdup("");
>     path = g_strjoinv("/", strarray);
>     g_strfreev(strarray);
>     return path;
> }

-- PMM

Re: [Qemu-devel] [PATCH] check for available room when formatting OpenFirmware device path

[Markus Armbruster]

Laszlo Ersek <lersek@redhat.com> writes:

> Signed-off-by: Laszlo Ersek <lersek@redhat.com>
> ---
>  hw/qdev.c |   14 +++++++++++++-
>  vl.c      |    7 ++++++-
>  2 files changed, 19 insertions(+), 2 deletions(-)
>
> diff --git a/hw/qdev.c b/hw/qdev.c
> index af54467..f1e83a4 100644
> --- a/hw/qdev.c
> +++ b/hw/qdev.c
> @@ -502,6 +502,10 @@ static int qdev_get_fw_dev_path_helper(DeviceState *dev, char *p, int size)
>      if (dev && dev->parent_bus) {
>          char *d;
>          l = qdev_get_fw_dev_path_helper(dev->parent_bus->parent, p, size);
> +        if (l >= size) {
> +            return l;
> +        }
> +
>          d = bus_get_fw_dev_path(dev->parent_bus, dev);
>          if (d) {
>              l += snprintf(p + l, size - l, "%s", d);
> @@ -509,6 +513,10 @@ static int qdev_get_fw_dev_path_helper(DeviceState *dev, char *p, int size)
>          } else {
>              l += snprintf(p + l, size - l, "%s", object_get_typename(OBJECT(dev)));
>          }
> +
> +        if (l >= size) {
> +            return l;
> +        }
>      }
>      l += snprintf(p + l , size - l, "/");
>  

If the return value is less than the size argument, it's the length of
the string written into p[].  Else, it means p[] has insufficient
space.

> @@ -520,8 +528,12 @@ char* qdev_get_fw_dev_path(DeviceState *dev)
>      char path[128];
>      int l;
>  
> -    l = qdev_get_fw_dev_path_helper(dev, path, 128);
> +    l = qdev_get_fw_dev_path_helper(dev, path, sizeof(path));
>  
> +    assert(l > 0);
> +    if (l >= sizeof(path)) {
> +        return NULL;
> +    }

Failure mode could be avoided with the common technique: make
qdev_get_fw_dev_path_helper() return the true length.  If it fit into
path[], return strdup(path).  Else, allocate a suitable buffer and try
again.

What do you think?

>      path[l-1] = '\0';
>  
>      return strdup(path);
> diff --git a/vl.c b/vl.c
> index 8904db1..78dcc93 100644
> --- a/vl.c
> +++ b/vl.c
> @@ -913,7 +913,12 @@ char *get_boot_devices_list(uint32_t *size)
>  
>          if (i->dev) {
>              devpath = qdev_get_fw_dev_path(i->dev);
> -            assert(devpath);
> +            if (devpath == NULL) {
> +                fprintf(stderr,
> +                        "OpenFirmware Device Path too long (boot index %d)\n",
> +                        i->bootindex);
> +                exit(1);
> +            }
>          }
>  
>          if (i->suffix && devpath) {

Re: [Qemu-devel] [PATCH] check for available room when formatting OpenFirmware device path

[Laszlo Ersek]

On 07/23/12 14:46, Markus Armbruster wrote:
> Laszlo Ersek <lersek@redhat.com> writes:
> 
>> Signed-off-by: Laszlo Ersek <lersek@redhat.com>
>> ---
>>  hw/qdev.c |   14 +++++++++++++-
>>  vl.c      |    7 ++++++-
>>  2 files changed, 19 insertions(+), 2 deletions(-)
>>
>> diff --git a/hw/qdev.c b/hw/qdev.c
>> index af54467..f1e83a4 100644
>> --- a/hw/qdev.c
>> +++ b/hw/qdev.c
>> @@ -502,6 +502,10 @@ static int qdev_get_fw_dev_path_helper(DeviceState *dev, char *p, int size)
>>      if (dev && dev->parent_bus) {
>>          char *d;
>>          l = qdev_get_fw_dev_path_helper(dev->parent_bus->parent, p, size);
>> +        if (l >= size) {
>> +            return l;
>> +        }
>> +
>>          d = bus_get_fw_dev_path(dev->parent_bus, dev);
>>          if (d) {
>>              l += snprintf(p + l, size - l, "%s", d);
>> @@ -509,6 +513,10 @@ static int qdev_get_fw_dev_path_helper(DeviceState *dev, char *p, int size)
>>          } else {
>>              l += snprintf(p + l, size - l, "%s", object_get_typename(OBJECT(dev)));
>>          }
>> +
>> +        if (l >= size) {
>> +            return l;
>> +        }
>>      }
>>      l += snprintf(p + l , size - l, "/");
>>  
> 
> If the return value is less than the size argument, it's the length of
> the string written into p[].  Else, it means p[] has insufficient
> space.

Yes. (snprintf() returns the number of bytes it would store, excluding
the terminating NUL, had there been enough room.
<http://pubs.opengroup.org/onlinepubs/9699919799/functions/snprintf.html#tag_16_159_04>)

Did I make a mistake?

Supposing snprintf() encounters no error, it returns a positive value P
in the above.

P = snprintf(..., size - l0, ...)
l1 = l0 + P;

         l1 >= size
<->  l0 + P >= size
<->       P >= size - l0


The return value of qdev_get_fw_dev_path_helper() comes from another
invocation of itself, or from the trailing snprintf(), so it behaves
like snprintf.

Or what do you have in mind?


> 
>> @@ -520,8 +528,12 @@ char* qdev_get_fw_dev_path(DeviceState *dev)
>>      char path[128];
>>      int l;
>>  
>> -    l = qdev_get_fw_dev_path_helper(dev, path, 128);
>> +    l = qdev_get_fw_dev_path_helper(dev, path, sizeof(path));
>>  
>> +    assert(l > 0);
>> +    if (l >= sizeof(path)) {
>> +        return NULL;
>> +    }
> 
> Failure mode could be avoided with the common technique: make
> qdev_get_fw_dev_path_helper() return the true length.  If it fit into
> path[], return strdup(path).  Else, allocate a suitable buffer and try
> again.
> 
> What do you think?

You're right of course, but I didn't have (or wanted to spend) the time
to change the code that much (and to test it...), especially because it
would have been fixed already if it had caused actual problems. I think
the patch could work as a "last resort", small improvement, but I don't
mind if someone posts a better fix :)

Laszlo

Re: [Qemu-devel] [PATCH] check for available room when formatting OpenFirmware device path

[Markus Armbruster]

Laszlo Ersek <lersek@redhat.com> writes:

> On 07/23/12 14:46, Markus Armbruster wrote:
>> Laszlo Ersek <lersek@redhat.com> writes:
>> 
>>> Signed-off-by: Laszlo Ersek <lersek@redhat.com>
>>> ---
>>>  hw/qdev.c |   14 +++++++++++++-
>>>  vl.c      |    7 ++++++-
>>>  2 files changed, 19 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/hw/qdev.c b/hw/qdev.c
>>> index af54467..f1e83a4 100644
>>> --- a/hw/qdev.c
>>> +++ b/hw/qdev.c
>>> @@ -502,6 +502,10 @@ static int qdev_get_fw_dev_path_helper(DeviceState *dev, char *p, int size)
>>>      if (dev && dev->parent_bus) {
>>>          char *d;
>>>          l = qdev_get_fw_dev_path_helper(dev->parent_bus->parent, p, size);
>>> +        if (l >= size) {
>>> +            return l;
>>> +        }
>>> +
>>>          d = bus_get_fw_dev_path(dev->parent_bus, dev);
>>>          if (d) {
>>>              l += snprintf(p + l, size - l, "%s", d);
>>> @@ -509,6 +513,10 @@ static int qdev_get_fw_dev_path_helper(DeviceState *dev, char *p, int size)
>>>          } else {
>>>              l += snprintf(p + l, size - l, "%s", object_get_typename(OBJECT(dev)));
>>>          }
>>> +
>>> +        if (l >= size) {
>>> +            return l;
>>> +        }
>>>      }
>>>      l += snprintf(p + l , size - l, "/");
>>>  
>> 
>> If the return value is less than the size argument, it's the length of
>> the string written into p[].  Else, it means p[] has insufficient
>> space.
>
> Yes. (snprintf() returns the number of bytes it would store, excluding
> the terminating NUL, had there been enough room.
> <http://pubs.opengroup.org/onlinepubs/9699919799/functions/snprintf.html#tag_16_159_04>)
>
> Did I make a mistake?
>
> Supposing snprintf() encounters no error, it returns a positive value P
> in the above.
>
> P = snprintf(..., size - l0, ...)
> l1 = l0 + P;
>
>          l1 >= size
> <->  l0 + P >= size
> <->       P >= size - l0
>
>
> The return value of qdev_get_fw_dev_path_helper() comes from another
> invocation of itself, or from the trailing snprintf(), so it behaves
> like snprintf.
>
> Or what do you have in mind?

If I read your code correctly, qdev_get_fw_dev_path_helper() bails out
after the first snprintf() that goes beyond the buffer size.  When that
happens before the job's done, the return value is less than the length
of the full path.

 static int qdev_get_fw_dev_path_helper(DeviceState *dev, char *p, int size)
 {
     int l = 0;

     if (dev && dev->parent_bus) {
         char *d;
         l = qdev_get_fw_dev_path_helper(dev->parent_bus->parent, p, size);
+        if (l >= size) {
+            return l;                  <--- bail out early
+        }
+
         if (dev->parent_bus->info->get_fw_dev_path) {
             d = dev->parent_bus->info->get_fw_dev_path(dev);
             l += snprintf(p + l, size - l, "%s", d);
             qemu_free(d);
         } else {
             l += snprintf(p + l, size - l, "%s", dev->info->name);
         }
+
+        if (l >= size) {
+            return l;                  <--- bail out early
+        }
     }
     l += snprintf(p + l , size - l, "/");

     return l;
 }

>>> @@ -520,8 +528,12 @@ char* qdev_get_fw_dev_path(DeviceState *dev)
>>>      char path[128];
>>>      int l;
>>>  
>>> -    l = qdev_get_fw_dev_path_helper(dev, path, 128);
>>> +    l = qdev_get_fw_dev_path_helper(dev, path, sizeof(path));
>>>  
>>> +    assert(l > 0);
>>> +    if (l >= sizeof(path)) {
>>> +        return NULL;
>>> +    }
>> 
>> Failure mode could be avoided with the common technique: make
>> qdev_get_fw_dev_path_helper() return the true length.  If it fit into
>> path[], return strdup(path).  Else, allocate a suitable buffer and try
>> again.
>> 
>> What do you think?
>
> You're right of course, but I didn't have (or wanted to spend) the time
> to change the code that much (and to test it...), especially because it
> would have been fixed already if it had caused actual problems. I think
> the patch could work as a "last resort", small improvement, but I don't
> mind if someone posts a better fix :)

Only marginally harder than handling the failure :)

Re: [Qemu-devel] [PATCH] check for available room when formatting OpenFirmware device path

[Laszlo Ersek]

On 07/23/12 17:01, Markus Armbruster wrote:
> Laszlo Ersek <lersek@redhat.com> writes:
> 
>> On 07/23/12 14:46, Markus Armbruster wrote:
>>> Laszlo Ersek <lersek@redhat.com> writes:
>>>
>>>> Signed-off-by: Laszlo Ersek <lersek@redhat.com>
>>>> ---
>>>>  hw/qdev.c |   14 +++++++++++++-
>>>>  vl.c      |    7 ++++++-
>>>>  2 files changed, 19 insertions(+), 2 deletions(-)
>>>>
>>>> diff --git a/hw/qdev.c b/hw/qdev.c
>>>> index af54467..f1e83a4 100644
>>>> --- a/hw/qdev.c
>>>> +++ b/hw/qdev.c
>>>> @@ -502,6 +502,10 @@ static int qdev_get_fw_dev_path_helper(DeviceState *dev, char *p, int size)
>>>>      if (dev && dev->parent_bus) {
>>>>          char *d;
>>>>          l = qdev_get_fw_dev_path_helper(dev->parent_bus->parent, p, size);
>>>> +        if (l >= size) {
>>>> +            return l;
>>>> +        }
>>>> +
>>>>          d = bus_get_fw_dev_path(dev->parent_bus, dev);
>>>>          if (d) {
>>>>              l += snprintf(p + l, size - l, "%s", d);
>>>> @@ -509,6 +513,10 @@ static int qdev_get_fw_dev_path_helper(DeviceState *dev, char *p, int size)
>>>>          } else {
>>>>              l += snprintf(p + l, size - l, "%s", object_get_typename(OBJECT(dev)));
>>>>          }
>>>> +
>>>> +        if (l >= size) {
>>>> +            return l;
>>>> +        }
>>>>      }
>>>>      l += snprintf(p + l , size - l, "/");
>>>>  
>>>
>>> If the return value is less than the size argument, it's the length of
>>> the string written into p[].  Else, it means p[] has insufficient
>>> space.
>>
>> Yes. (snprintf() returns the number of bytes it would store, excluding
>> the terminating NUL, had there been enough room.
>> <http://pubs.opengroup.org/onlinepubs/9699919799/functions/snprintf.html#tag_16_159_04>)
>>
>> Did I make a mistake?
>>
>> Supposing snprintf() encounters no error, it returns a positive value P
>> in the above.
>>
>> P = snprintf(..., size - l0, ...)
>> l1 = l0 + P;
>>
>>          l1 >= size
>> <->  l0 + P >= size
>> <->       P >= size - l0
>>
>>
>> The return value of qdev_get_fw_dev_path_helper() comes from another
>> invocation of itself, or from the trailing snprintf(), so it behaves
>> like snprintf.
>>
>> Or what do you have in mind?
> 
> If I read your code correctly, qdev_get_fw_dev_path_helper() bails out
> after the first snprintf() that goes beyond the buffer size.  When that
> happens before the job's done, the return value is less than the length
> of the full path.

Yes, but still greater than what would fit in the buffer. Is it a problem?

... Oh, did you mean the first comment in connection with the second?
Ie. we should continue to format (just for length counting's sake) and
then retry? IOW the early return isn't problematic in itself, but it
doesn't support the reallocation?

Thanks,
Laszlo

Re: [Qemu-devel] [PATCH] check for available room when formatting OpenFirmware device path

[Markus Armbruster]

Laszlo Ersek <lersek@redhat.com> writes:

> On 07/23/12 17:01, Markus Armbruster wrote:
>> Laszlo Ersek <lersek@redhat.com> writes:
>> 
>>> On 07/23/12 14:46, Markus Armbruster wrote:
>>>> Laszlo Ersek <lersek@redhat.com> writes:
>>>>
>>>>> Signed-off-by: Laszlo Ersek <lersek@redhat.com>
>>>>> ---
>>>>>  hw/qdev.c |   14 +++++++++++++-
>>>>>  vl.c      |    7 ++++++-
>>>>>  2 files changed, 19 insertions(+), 2 deletions(-)
>>>>>
>>>>> diff --git a/hw/qdev.c b/hw/qdev.c
>>>>> index af54467..f1e83a4 100644
>>>>> --- a/hw/qdev.c
>>>>> +++ b/hw/qdev.c
>>>>> @@ -502,6 +502,10 @@ static int qdev_get_fw_dev_path_helper(DeviceState *dev, char *p, int size)
>>>>>      if (dev && dev->parent_bus) {
>>>>>          char *d;
>>>>>          l = qdev_get_fw_dev_path_helper(dev->parent_bus->parent, p, size);
>>>>> +        if (l >= size) {
>>>>> +            return l;
>>>>> +        }
>>>>> +
>>>>>          d = bus_get_fw_dev_path(dev->parent_bus, dev);
>>>>>          if (d) {
>>>>>              l += snprintf(p + l, size - l, "%s", d);
>>>>> @@ -509,6 +513,10 @@ static int qdev_get_fw_dev_path_helper(DeviceState *dev, char *p, int size)
>>>>>          } else {
>>>>>              l += snprintf(p + l, size - l, "%s", object_get_typename(OBJECT(dev)));
>>>>>          }
>>>>> +
>>>>> +        if (l >= size) {
>>>>> +            return l;
>>>>> +        }
>>>>>      }
>>>>>      l += snprintf(p + l , size - l, "/");
>>>>>  
>>>>
>>>> If the return value is less than the size argument, it's the length of
>>>> the string written into p[].  Else, it means p[] has insufficient
>>>> space.
>>>
>>> Yes. (snprintf() returns the number of bytes it would store, excluding
>>> the terminating NUL, had there been enough room.
>>> <http://pubs.opengroup.org/onlinepubs/9699919799/functions/snprintf.html#tag_16_159_04>)
>>>
>>> Did I make a mistake?
>>>
>>> Supposing snprintf() encounters no error, it returns a positive value P
>>> in the above.
>>>
>>> P = snprintf(..., size - l0, ...)
>>> l1 = l0 + P;
>>>
>>>          l1 >= size
>>> <->  l0 + P >= size
>>> <->       P >= size - l0
>>>
>>>
>>> The return value of qdev_get_fw_dev_path_helper() comes from another
>>> invocation of itself, or from the trailing snprintf(), so it behaves
>>> like snprintf.
>>>
>>> Or what do you have in mind?
>> 
>> If I read your code correctly, qdev_get_fw_dev_path_helper() bails out
>> after the first snprintf() that goes beyond the buffer size.  When that
>> happens before the job's done, the return value is less than the length
>> of the full path.
>
> Yes, but still greater than what would fit in the buffer. Is it a problem?
>
> ... Oh, did you mean the first comment in connection with the second?
> Ie. we should continue to format (just for length counting's sake) and
> then retry? IOW the early return isn't problematic in itself, but it
> doesn't support the reallocation?

Yes.

Your code lets the caller detect "need more space" reliably.  How much
more it can only guess, and need to be prepared for the retry to fail
again.  If we return the required size, like snprintf() does, the caller
knows, and the retry will succeed.

[Qemu-devel] [PATCH v2 0/2] assorted device path formatting cleanups

[Laszlo Ersek]

As long as "two" qualifies as "assorted".

v1->v2:
- abandon original "idea", allocate sufficient memory for OFW device path
  formatting [Markus]
- all bus formatters should rely on glib for dynamic allocation [Peter]

Tested with an OVMF debug patch that grabs and logs the "bootorder" fw_cfg
file:

  /pci@i0cf8/ide@1,1/drive@0/disk@0
  /pci@i0cf8/ide@1,1/drive@1/disk@0
  /pci@i0cf8/isa@1/fdc@03f0/floppy@0
  /pci@i0cf8/ethernet@3/ethernet-phy@0

Laszlo Ersek (2):
  accomodate OpenFirmware device paths in sufficient storage
  get_fw_dev_path() impls should allocate memory with glib functions

 hw/ide/qdev.c |    2 +-
 hw/isa-bus.c  |    2 +-
 hw/pci.c      |    2 +-
 hw/qdev.c     |   54 +++++++++++++++++++++++++++++++++++++++++++-----------
 hw/scsi-bus.c |    2 +-
 hw/sysbus.c   |    2 +-
 6 files changed, 48 insertions(+), 16 deletions(-)

[Qemu-devel] [PATCH v2 1/2] accomodate OpenFirmware device paths in sufficient storage

[Laszlo Ersek]

Signed-off-by: Laszlo Ersek <lersek@redhat.com>
---
 hw/qdev.c |   54 +++++++++++++++++++++++++++++++++++++++++++-----------
 1 files changed, 43 insertions(+), 11 deletions(-)

diff --git a/hw/qdev.c b/hw/qdev.c
index af54467..59cc0c2 100644
--- a/hw/qdev.c
+++ b/hw/qdev.c
@@ -25,6 +25,8 @@
    inherit from a particular bus (e.g. PCI or I2C) rather than
    this API directly.  */
 
+#include <stdarg.h>
+
 #include "net.h"
 #include "qdev.h"
 #include "sysemu.h"
@@ -495,36 +497,66 @@ static char *bus_get_fw_dev_path(BusState *bus, DeviceState *dev)
     return NULL;
 }
 
+static int snprintf_helper(char *p, int size, int len, const char *format, ...)
+{
+    char *target;
+    int free_sz;
+    va_list args;
+    int ret;
+
+    if (len < size) {
+        target = p + len;
+        free_sz = size - len;
+    }
+    else {
+        target = NULL;
+        free_sz = 0;
+    }
+
+    va_start(args, format);
+    ret = vsnprintf(target, free_sz, format, args);
+    va_end(args);
+
+    assert(ret >= 0);
+    return ret;
+}
+
+
 static int qdev_get_fw_dev_path_helper(DeviceState *dev, char *p, int size)
 {
-    int l = 0;
+    int len = 0;
 
     if (dev && dev->parent_bus) {
         char *d;
-        l = qdev_get_fw_dev_path_helper(dev->parent_bus->parent, p, size);
+        len = qdev_get_fw_dev_path_helper(dev->parent_bus->parent, p, size);
         d = bus_get_fw_dev_path(dev->parent_bus, dev);
         if (d) {
-            l += snprintf(p + l, size - l, "%s", d);
+            len += snprintf_helper(p, size, len, "%s", d);
             g_free(d);
         } else {
-            l += snprintf(p + l, size - l, "%s", object_get_typename(OBJECT(dev)));
+            len += snprintf_helper(p, size, len, "%s",
+                                   object_get_typename(OBJECT(dev)));
         }
     }
-    l += snprintf(p + l , size - l, "/");
+    len += snprintf_helper(p, size, len, "/");
 
-    return l;
+    return len;
 }
 
 char* qdev_get_fw_dev_path(DeviceState *dev)
 {
-    char path[128];
-    int l;
+    int len, len2;
+    char *path;
 
-    l = qdev_get_fw_dev_path_helper(dev, path, 128);
+    len = qdev_get_fw_dev_path_helper(dev, NULL, 0);
+    path = g_malloc(len + 1);
 
-    path[l-1] = '\0';
+    len2 = qdev_get_fw_dev_path_helper(dev, path, len + 1);
+    assert(len2 == len);
 
-    return strdup(path);
+    assert(len > 0);
+    path[len - 1] = '\0';
+    return path;
 }
 
 char *qdev_get_dev_path(DeviceState *dev)
-- 
1.7.1

[Qemu-devel] [PATCH v2 2/2] get_fw_dev_path() impls should allocate memory with glib functions

[Laszlo Ersek]

Signed-off-by: Laszlo Ersek <lersek@redhat.com>
---
 hw/ide/qdev.c |    2 +-
 hw/isa-bus.c  |    2 +-
 hw/pci.c      |    2 +-
 hw/scsi-bus.c |    2 +-
 hw/sysbus.c   |    2 +-
 5 files changed, 5 insertions(+), 5 deletions(-)

diff --git a/hw/ide/qdev.c b/hw/ide/qdev.c
index 22e58df..4f15070 100644
--- a/hw/ide/qdev.c
+++ b/hw/ide/qdev.c
@@ -60,7 +60,7 @@ static char *idebus_get_fw_dev_path(DeviceState *dev)
     snprintf(path, sizeof(path), "%s@%d", qdev_fw_name(dev),
              ((IDEBus*)dev->parent_bus)->bus_id);
 
-    return strdup(path);
+    return g_strdup(path);
 }
 
 static int ide_qdev_init(DeviceState *qdev)
diff --git a/hw/isa-bus.c b/hw/isa-bus.c
index f9b2373..47c93d3 100644
--- a/hw/isa-bus.c
+++ b/hw/isa-bus.c
@@ -236,7 +236,7 @@ static char *isabus_get_fw_dev_path(DeviceState *dev)
         snprintf(path + off, sizeof(path) - off, "@%04x", d->ioport_id);
     }
 
-    return strdup(path);
+    return g_strdup(path);
 }
 
 MemoryRegion *isa_address_space(ISADevice *dev)
diff --git a/hw/pci.c b/hw/pci.c
index 99a4304..ef8996d 100644
--- a/hw/pci.c
+++ b/hw/pci.c
@@ -1920,7 +1920,7 @@ static char *pcibus_get_fw_dev_path(DeviceState *dev)
                    PCI_SLOT(d->devfn));
     if (PCI_FUNC(d->devfn))
         snprintf(path + off, sizeof(path) + off, ",%x", PCI_FUNC(d->devfn));
-    return strdup(path);
+    return g_strdup(path);
 }
 
 static char *pcibus_get_dev_path(DeviceState *dev)
diff --git a/hw/scsi-bus.c b/hw/scsi-bus.c
index dc74063..4544d0b 100644
--- a/hw/scsi-bus.c
+++ b/hw/scsi-bus.c
@@ -1547,7 +1547,7 @@ static char *scsibus_get_fw_dev_path(DeviceState *dev)
     snprintf(path, sizeof(path), "channel@%x/%s@%x,%x", d->channel,
              qdev_fw_name(dev), d->id, d->lun);
 
-    return strdup(path);
+    return g_strdup(path);
 }
 
 SCSIDevice *scsi_device_find(SCSIBus *bus, int channel, int id, int lun)
diff --git a/hw/sysbus.c b/hw/sysbus.c
index 9d8b1ea..c173840 100644
--- a/hw/sysbus.c
+++ b/hw/sysbus.c
@@ -211,7 +211,7 @@ static char *sysbus_get_fw_dev_path(DeviceState *dev)
         snprintf(path + off, sizeof(path) - off, "@i%04x", s->pio[0]);
     }
 
-    return strdup(path);
+    return g_strdup(path);
 }
 
 void sysbus_add_memory(SysBusDevice *dev, target_phys_addr_t addr,
-- 
1.7.1

Re: [Qemu-devel] [PATCH v2 1/2] accomodate OpenFirmware device paths in sufficient storage

[Peter Maydell]

On 25 July 2012 01:19, Laszlo Ersek <lersek@redhat.com> wrote:
>
> Signed-off-by: Laszlo Ersek <lersek@redhat.com>
> ---
>  hw/qdev.c |   54 +++++++++++++++++++++++++++++++++++++++++++-----------
>  1 files changed, 43 insertions(+), 11 deletions(-)

Yuck. Just redo qdev_get_fw_dev_path along the lines I
suggested please. This is way too complicated.

-- PMM

> diff --git a/hw/qdev.c b/hw/qdev.c
> index af54467..59cc0c2 100644
> --- a/hw/qdev.c
> +++ b/hw/qdev.c
> @@ -25,6 +25,8 @@
>     inherit from a particular bus (e.g. PCI or I2C) rather than
>     this API directly.  */
>
> +#include <stdarg.h>
> +
>  #include "net.h"
>  #include "qdev.h"
>  #include "sysemu.h"
> @@ -495,36 +497,66 @@ static char *bus_get_fw_dev_path(BusState *bus, DeviceState *dev)
>      return NULL;
>  }
>
> +static int snprintf_helper(char *p, int size, int len, const char *format, ...)
> +{
> +    char *target;
> +    int free_sz;
> +    va_list args;
> +    int ret;
> +
> +    if (len < size) {
> +        target = p + len;
> +        free_sz = size - len;
> +    }
> +    else {
> +        target = NULL;
> +        free_sz = 0;
> +    }
> +
> +    va_start(args, format);
> +    ret = vsnprintf(target, free_sz, format, args);
> +    va_end(args);
> +
> +    assert(ret >= 0);
> +    return ret;
> +}
> +
> +
>  static int qdev_get_fw_dev_path_helper(DeviceState *dev, char *p, int size)
>  {
> -    int l = 0;
> +    int len = 0;
>
>      if (dev && dev->parent_bus) {
>          char *d;
> -        l = qdev_get_fw_dev_path_helper(dev->parent_bus->parent, p, size);
> +        len = qdev_get_fw_dev_path_helper(dev->parent_bus->parent, p, size);
>          d = bus_get_fw_dev_path(dev->parent_bus, dev);
>          if (d) {
> -            l += snprintf(p + l, size - l, "%s", d);
> +            len += snprintf_helper(p, size, len, "%s", d);
>              g_free(d);
>          } else {
> -            l += snprintf(p + l, size - l, "%s", object_get_typename(OBJECT(dev)));
> +            len += snprintf_helper(p, size, len, "%s",
> +                                   object_get_typename(OBJECT(dev)));
>          }
>      }
> -    l += snprintf(p + l , size - l, "/");
> +    len += snprintf_helper(p, size, len, "/");
>
> -    return l;
> +    return len;
>  }
>
>  char* qdev_get_fw_dev_path(DeviceState *dev)
>  {
> -    char path[128];
> -    int l;
> +    int len, len2;
> +    char *path;
>
> -    l = qdev_get_fw_dev_path_helper(dev, path, 128);
> +    len = qdev_get_fw_dev_path_helper(dev, NULL, 0);
> +    path = g_malloc(len + 1);
>
> -    path[l-1] = '\0';
> +    len2 = qdev_get_fw_dev_path_helper(dev, path, len + 1);
> +    assert(len2 == len);
>
> -    return strdup(path);
> +    assert(len > 0);
> +    path[len - 1] = '\0';
> +    return path;
>  }
>
>  char *qdev_get_dev_path(DeviceState *dev)
> --
> 1.7.1
>
>

Re: [Qemu-devel] [PATCH v2 1/2] accomodate OpenFirmware device paths in sufficient storage

[Markus Armbruster]

Reviewed-by: Markus Armbruster <armbru@redhat.com>