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

[Amos Kong]

On Wed, Nov 05, 2014 at 12:19:34PM +0100, Eric Blake wrote:
> 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.

Agree! use an assert to teach the caller ;)

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

-- 
                        Amos.

signature.asc
Description: Digital signature