Re: [PATCH] Do not use %m in common code to print error messages

[Thomas Huth <thuth@redhat.com>]

On 18/10/2019 12.57, Daniel P. Berrangé wrote:
> On Fri, Oct 18, 2019 at 12:44:38PM +0200, Thomas Huth wrote:
>> The %m format specifier is an extension from glibc - and when compiling
>> QEMU for NetBSD, the compiler correctly complains, e.g.:
>>
>> /home/qemu/qemu-test.ELjfrQ/src/util/main-loop.c: In function 'sigfd_handler':
>> /home/qemu/qemu-test.ELjfrQ/src/util/main-loop.c:64:13: warning: %m is only
>>  allowed in syslog(3) like functions [-Wformat=]
>>              printf("read from sigfd returned %zd: %m\n", len);
>>              ^
>> Let's use g_strerror() here instead, which is an easy-to-use wrapper
>> around the thread-safe strerror_r() function.
>>
>> While we're at it, also convert the "printf()" in main-loop.c into
>> the preferred "error_report()".
>>
>> Signed-off-by: Thomas Huth <thuth@redhat.com>
>> ---
>>  hw/misc/tmp421.c | 8 ++++++--
>>  util/main-loop.c | 4 +++-
>>  util/systemd.c   | 5 +++--
>>  3 files changed, 12 insertions(+), 5 deletions(-)
>>
>> diff --git a/hw/misc/tmp421.c b/hw/misc/tmp421.c
>> index 9f044705fa..f23c46a40a 100644
>> --- a/hw/misc/tmp421.c
>> +++ b/hw/misc/tmp421.c
>> @@ -120,7 +120,9 @@ static void tmp421_get_temperature(Object *obj, Visitor *v, const char *name,
>>      int tempid;
>>  
>>      if (sscanf(name, "temperature%d", &tempid) != 1) {
>> -        error_setg(errp, "error reading %s: %m", name);
>> +        const char *errmsg = g_strerror(errno);
>> +        error_setg(errp, "error reading %s: %s", name, errmsg);
>> +        g_free((gpointer)errmsg);
> 
> Kaboom crash. This is trying to free a const string that is the caller
> doesn't own. It remains under ownership of g_strerror forever.

Well, if you look at the implementation of g_strerror(), you can see
that glib returns allocated memory here (with g_strdup() for example).
So if we don't free, this will leak memory over time.

But you're right, the "const" and documentation of the function indicate
that the caller should rather not free the memory. What a bummer, sounds
like g_strerror() is pretty much useless unless you immediately exit()
afterwards (and you care about not leaking memory).

I guess we have to implement our own wrapper, qemu_strerror() instead?

 Thomas