Re: [Qemu-devel] [PATCH v2 1/7] error: De-duplicate code creating Error objects

[Markus Armbruster]

Eric Blake <address@hidden> writes:

> On 07/23/2015 08:01 AM, Markus Armbruster wrote:
>> Duplicated when commit 680d16d added error_set_errno(), and again when
>> commit 20840d4 added error_set_win32().
>> 
>> Make the original copy in error_set() reusable by factoring out
>> error_setv(), then rewrite error_set_errno() and error_set_win32() on
>> top of it.
>> 
>> Signed-off-by: Markus Armbruster <address@hidden>
>> Reviewed-by: Eric Blake <address@hidden>
>> ---
>>  util/error.c | 70 
>> ++++++++++++++++++++++--------------------------------------
>>  1 file changed, 26 insertions(+), 44 deletions(-)
>> 
>
>>      if (os_errno != 0) {
>> -        err->msg = g_strdup_printf("%s: %s", msg1, strerror(os_errno));
>> -        g_free(msg1);
>> -    } else {
>> -        err->msg = msg1;
>> +        msg = (*errp)->msg;
>> +        (*errp)->msg = g_strdup_printf("%s: %s", msg, strerror(os_errno));
>
> Unrelated comment, so doesn't change R-b:
>
> strerror() is not required to be thread-safe, and can return NULL on
> error.  Do we care?

Quoting strerror(3):

       POSIX.1-2001 permits strerror() to set errno if the call
       encounters an error, but does not specify what value should be
       returned as the function result in the event of an error.  On
       some systems, strerror() returns NULL if the error number is
       unknown.  On other systems, strerror() returns a string something
       like "Error nnn occurred" and sets errno to EINVAL if the error
       number is unknown.  C99 and POSIX.1-2008 require the return value
       to be non-NULL.

We already rely on strerror() behaving nicely in many, many places.

Let's not start worrying about rotten, NULL-returning implementations
until we run into one.

I don't want to hand-wave thread safety worries away similarly.
However, this patch doesn't add strerror() uses.  If we want to discuss
the existing potential problems, I feel we should start a new thread.