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

[Markus Armbruster]

Laszlo Ersek <address@hidden> writes:

> On 07/23/12 17:01, Markus Armbruster wrote:
>> Laszlo Ersek <address@hidden> writes:
>> 
>>> On 07/23/12 14:46, Markus Armbruster wrote:
>>>> Laszlo Ersek <address@hidden> writes:
>>>>
>>>>> Signed-off-by: Laszlo Ersek <address@hidden>
>>>>> ---
>>>>>  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.