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

[Markus Armbruster]

SeokYeon Hwang <address@hidden> writes:

>> -----Original Message-----
>> From: SeokYeon Hwang [mailto:address@hidden
>> Sent: Wednesday, November 05, 2014 10:13 PM
>> To: 'Paolo Bonzini'; 'Max Reitz'; 'address@hidden'
>> Cc: 'address@hidden'; 'address@hidden'
>> Subject: RE: [PATCH] error: fixed error_set_errno() to deal with a
>> negative type of os_error.
>> 
>> > -----Original Message-----
>> > From: Paolo Bonzini [mailto:address@hidden On Behalf Of
>> > Paolo Bonzini
>> > Sent: Wednesday, November 05, 2014 9:45 PM
>> > To: Max Reitz; SeokYeon Hwang; address@hidden
>> > Cc: address@hidden; address@hidden
>> > Subject: Re: [PATCH] error: fixed error_set_errno() to deal with a
>> > negative type of os_error.
>> >
>> >
>> >
>> > 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.
>> >
>> > 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.
>> >
>> > Let's instead add an assertion check to error_setg_errno.
>> >
>> > Paolo
>> 
>> I have expected to come out several opinions about this patch.
>> 
>> The use of negative errno on "strerror()" was obviously wrong. But that
>> does not mean it is wrong to use the negative errno on
> "error_set_errno()".
>> The reason that I chose this one among the solutions is to change function
>> specification. I think it seems good to us to respect the tradition of the
>> developers that use negative errno.
>> 
>> But if error_set_errno() has strict specification - so, we must not change
>> it's spec - I agree with Paolo's opinion.
>
> I think we have 2 options.
>
> 1. "error_set_errno()" is just utility for developer's convenience.
> Why can't we supply more convenience to developer ??
> -> My first opinion.
>
> 2. It is not just utility function for convenience or we cannot change
> its spec because it is well-known function.
> -> If this is right, I'm ready to post 2nd patch that applied Paolo's
> opinion.
>
> What do you think about it??

3. Passing a negative value to an errno parameter is wrong.  It's
probably a harmless sign error, but it *could* be a logic error.  We
should not sweep programming errors under the rug.

Please assert(os_error >= 0).  Help with auditing callers is welcome.