Re: [PATCH-for-5.2 1/5] hw/core/qdev-properties: Simplify get_reserved_region()

[Markus Armbruster]

Philippe Mathieu-Daudé <philmd@redhat.com> writes:

> On 7/16/20 10:29 AM, Markus Armbruster wrote:
>> Philippe Mathieu-Daudé <philmd@redhat.com> writes:
>> 
>>> Use the safer g_strdup_printf() over snprintf() + abort().
>>>
>>> Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
>>> ---
>>>  hw/core/qdev-properties.c | 9 +++------
>>>  1 file changed, 3 insertions(+), 6 deletions(-)
>>>
>>> diff --git a/hw/core/qdev-properties.c b/hw/core/qdev-properties.c
>>> index 098298c78e..d5f5aa150b 100644
>>> --- a/hw/core/qdev-properties.c
>>> +++ b/hw/core/qdev-properties.c
>>> @@ -581,13 +581,10 @@ static void get_reserved_region(Object *obj, Visitor 
>>> *v, const char *name,
>>>      DeviceState *dev = DEVICE(obj);
>>>      Property *prop = opaque;
>>>      ReservedRegion *rr = qdev_get_prop_ptr(dev, prop);
>>> -    char buffer[64];
>>> -    char *p = buffer;
>>> -    int rc;
>>> +    g_autofree char *p;
>>>  
>>> -    rc = snprintf(buffer, sizeof(buffer), "0x%"PRIx64":0x%"PRIx64":%u",
>>> -                  rr->low, rr->high, rr->type);
>>> -    assert(rc < sizeof(buffer));
>>> +    p = g_strdup_printf("0x%"PRIx64":0x%"PRIx64":%u",
>>> +                        rr->low, rr->high, rr->type);
>>>  
>>>      visit_type_str(v, name, &p, errp);
>>>  }
>> 
>> I don't buy "safer" (the old code is already safe).
>
> I'm suspicious when I find an assert/abort in a code reachable by
> management interface, as IIUC we don't want to crash the process.
> I agree this shouldn't happen and if it happens we are screwed
> anyway.
>
>> I could buy
>> "simpler".
>> 
>> It's also less efficient, but that shouldn't matter in a property
>> getter.
>
> If we want more efficient code, we should replace all the
> g_strdup_printf() calls by snprintf() + assert() in the places
> where we don't expect failure. This seems counterproductive from
> a maintenance PoV.

Mind, I'm not opposed to your patch.  I just find the commit message's
rationale misleading.  Your patch doesn't make the code safer (it
already is).  It does make it simpler.

>                    At some point we should make a decision and
> not allow more than 3 similar APIs at a time. We have been
> recommended to use GLib instead of snprintf() because it is "safer".
> Can we be consistent with recommendations? Else we should stop
> recommending to use GLib and friends.

We recommend g_strdup_printf() over snprintf() because it's easier to
use correctly, and usually easier to read.

Incorrect use of snprintf() is commonly unsafe.  Correct use of
snprintf() is safe, but commonly less readable than the equivalent
g_strdup_printf().  snprintf()'s efficiency advantage rarely matters.