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

[Philippe Mathieu-Daudé]

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. 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.

Thanks,

Phil.