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

[Markus Armbruster]

Paolo Bonzini <address@hidden> writes:

> On 05/11/2014 12:11, Max Reitz wrote:
>> 
>> 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
>> (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.
>> 
>> 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.
>
> I think side effect are a different matter than misuse of QEMU.

Yes.

error_setg_errno() had an unclear contract regarding errno.  Some
callers unjustifiedly assumed that it preserves errno.  We clearly
needed to clarify the contract.  Both "preserves" and "doesn't preserve"
make sense.  We picked the one that saves us fixing up callers.

error_setg_errno()'s contract regarding os_error isn't spelled out, but
the general errno contract applies by association: needs to be
non-negative.

> There are "only" 157 calls to error_setg_errno; 67 use "errno" as the
> argument, and 4 use an explicit errno value (one of them is the wrong
> -EBUSY).  The other 86 seem correct and should not be hard to audit.

Negative value is probably just a sign error, but it *could* be a logic
error.  I'd rather not sweep the latter under the carpet.

> Let's instead add an assertion check to error_setg_errno.

Yes, please.