> -----Original Message----- > From: Markus Armbruster [mailto:arm...@redhat.com] > Sent: Thursday, November 06, 2014 6:25 PM > To: SeokYeon Hwang > Cc: 'Paolo Bonzini'; 'Max Reitz'; qemu-devel@nongnu.org; Peter Maydell; > paolo.bonz...@gmail.com > Subject: Re: [Qemu-devel] [PATCH] error: fixed error_set_errno() to deal > with a negative type of os_error. > > 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.
I just posted patch v2. Thank you for your advice.
