SeokYeon Hwang <syeon.hw...@samsung.com> writes: >> -----Original Message----- >> From: SeokYeon Hwang [mailto:syeon.hw...@samsung.com] >> Sent: Wednesday, November 05, 2014 10:13 PM >> To: 'Paolo Bonzini'; 'Max Reitz'; 'qemu-devel@nongnu.org' >> Cc: 'arm...@redhat.com'; 'paolo.bonz...@gmail.com' >> Subject: RE: [PATCH] error: fixed error_set_errno() to deal with a >> negative type of os_error. >> >> > -----Original Message----- >> > From: Paolo Bonzini [mailto:paolo.bonz...@gmail.com] On Behalf Of >> > Paolo Bonzini >> > Sent: Wednesday, November 05, 2014 9:45 PM >> > To: Max Reitz; SeokYeon Hwang; qemu-devel@nongnu.org >> > Cc: arm...@redhat.com; paolo.bonz...@gmail.com >> > 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.
