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

[SeokYeon Hwang]

> -----Original Message-----
> From: Markus Armbruster [mailto:address@hidden
> Sent: Thursday, November 06, 2014 6:25 PM
> To: SeokYeon Hwang
> Cc: 'Paolo Bonzini'; 'Max Reitz'; address@hidden; Peter Maydell;
> address@hidden
> Subject: Re: [Qemu-devel] [PATCH] error: fixed error_set_errno() to deal
> with a negative type of os_error.
> 
> 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.

I just posted patch v2.
Thank you for your advice.