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
