Re: [Qemu-devel] [PATCH] error: fixed error_set_errno() to deal with a negative type of os_error.

[Eric Blake <eblake@redhat.com>]

[-- Attachment #1: Type: text/plain, Size: 1803 bytes --]

On 11/05/2014 12:11 PM, Max Reitz wrote:

>>>> +        err->msg = g_strdup_printf("%s: %s", msg1,
>>>> strerror(abs(os_errno)));

>> I don't, we really should fix the callers.
> 
> Of course I understand, but this patch doesn't make matters worse, as
> long as there are not systems which have negative values for errno

POSIX requires all defined errno values to be positive; negative errno
values are unambiguous as values that will cause strerror() to have to
generate a message about an unknown value.

> (which I think we generally assume not to exist throughout qemu). That's
> why I'm fine with it. We should fix the callers but I don't see why we
> shouldn't apply this patch as well.

This patch is a bandaid; it makes it harder to find callers that need to
be fixed.  I'd almost argue the exact opposite - add an assert(os_errno
> 0).  Then we'd loudly break on broken callers, making them easier to find.

> 
> A similar issue already came up and led to commit b276d2499, where
> callers of error_setg_errno() assumed that it would not clobber errno,
> so we fixed some of the callers but also applied that commit which just
> saves errno because there's no reason not to.

If we're willing to accept the convenience so that callers can be lazy,
then I like this patch.  If we want to fix bugs in the callers, then
this patch makes it harder to find those bugs.

I'm actually 60:40 in favor of this patch (I think the convenience
outweighs an audit of fixing all callers); but if we do that, then we
might also want to intentionally switch existing callers to pass
negative values rather than declaring that passing a negative value is a
bug.

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 539 bytes --]