| Message ID | 1415174992-13246-1-git-send-email-syeon.hwang@samsung.com |
|---|---|
| State | New |
| Headers | show |
On 2014-11-05 at 09:09, SeokYeon Hwang wrote: > Negative type of errno like -ERRNO is used a lot by developers. Therefore, error_set_errno() is modified to deal with a negative type of os_error. > (Negative type is used at pcie_cap_slot_hotplug_common() in hw/pci/pcie.c) > > Signed-off-by: SeokYeon Hwang <syeon.hwang@samsung.com> > --- > util/error.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/util/error.c b/util/error.c > index 2ace0d8..5db00c9 100644 > --- a/util/error.c > +++ b/util/error.c > @@ -68,7 +68,7 @@ void error_set_errno(Error **errp, int os_errno, ErrorClass err_class, > va_start(ap, fmt); > msg1 = g_strdup_vprintf(fmt, ap); > if (os_errno != 0) { > - err->msg = g_strdup_printf("%s: %s", msg1, strerror(os_errno)); > + err->msg = g_strdup_printf("%s: %s", msg1, strerror(abs(os_errno))); > g_free(msg1); > } else { > err->msg = msg1; This is utterly broken and we should fix all callers instead. ...But I like it. Reviewed-by: Max Reitz <mreitz@redhat.com>
On 05/11/2014 09:12, Max Reitz wrote: > On 2014-11-05 at 09:09, SeokYeon Hwang wrote: >> Negative type of errno like -ERRNO is used a lot by developers. >> Therefore, error_set_errno() is modified to deal with a negative type >> of os_error. >> (Negative type is used at pcie_cap_slot_hotplug_common() in >> hw/pci/pcie.c) >> >> Signed-off-by: SeokYeon Hwang <syeon.hwang@samsung.com> >> --- >> util/error.c | 2 +- >> 1 file changed, 1 insertion(+), 1 deletion(-) >> >> diff --git a/util/error.c b/util/error.c >> index 2ace0d8..5db00c9 100644 >> --- a/util/error.c >> +++ b/util/error.c >> @@ -68,7 +68,7 @@ void error_set_errno(Error **errp, int os_errno, >> ErrorClass err_class, >> va_start(ap, fmt); >> msg1 = g_strdup_vprintf(fmt, ap); >> if (os_errno != 0) { >> - err->msg = g_strdup_printf("%s: %s", msg1, strerror(os_errno)); >> + err->msg = g_strdup_printf("%s: %s", msg1, >> strerror(abs(os_errno))); >> g_free(msg1); >> } else { >> err->msg = msg1; > > This is utterly broken and we should fix all callers instead. > > ...But I like it. I don't, we really should fix the callers. Paolo > Reviewed-by: Max Reitz <mreitz@redhat.com>
On 2014-11-05 at 11:57, Paolo Bonzini wrote: > On 05/11/2014 09:12, Max Reitz wrote: >> On 2014-11-05 at 09:09, SeokYeon Hwang wrote: >>> Negative type of errno like -ERRNO is used a lot by developers. >>> Therefore, error_set_errno() is modified to deal with a negative type >>> of os_error. >>> (Negative type is used at pcie_cap_slot_hotplug_common() in >>> hw/pci/pcie.c) >>> >>> Signed-off-by: SeokYeon Hwang <syeon.hwang@samsung.com> >>> --- >>> util/error.c | 2 +- >>> 1 file changed, 1 insertion(+), 1 deletion(-) >>> >>> diff --git a/util/error.c b/util/error.c >>> index 2ace0d8..5db00c9 100644 >>> --- a/util/error.c >>> +++ b/util/error.c >>> @@ -68,7 +68,7 @@ void error_set_errno(Error **errp, int os_errno, >>> ErrorClass err_class, >>> va_start(ap, fmt); >>> msg1 = g_strdup_vprintf(fmt, ap); >>> if (os_errno != 0) { >>> - err->msg = g_strdup_printf("%s: %s", msg1, strerror(os_errno)); >>> + err->msg = g_strdup_printf("%s: %s", msg1, >>> strerror(abs(os_errno))); >>> g_free(msg1); >>> } else { >>> err->msg = msg1; >> This is utterly broken and we should fix all callers instead. >> >> ...But I like it. > I don't, we really should fix the callers. 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. Max > Paolo > >> Reviewed-by: Max Reitz <mreitz@redhat.com>
On 11/05/2014 12:11 PM, Max Reitz wrote: >>>> + err->msg = g_strdup_printf("%s: %s", msg1, >>>> strerror(abs(os_errno))); >> I don't, we really should fix the callers. > > 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 POSIX requires all defined errno values to be positive; negative errno values are unambiguous as values that will cause strerror() to have to generate a message about an unknown value. > (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. This patch is a bandaid; it makes it harder to find callers that need to be fixed. I'd almost argue the exact opposite - add an assert(os_errno > 0). Then we'd loudly break on broken callers, making them easier to find. > > 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. If we're willing to accept the convenience so that callers can be lazy, then I like this patch. If we want to fix bugs in the callers, then this patch makes it harder to find those bugs. I'm actually 60:40 in favor of this patch (I think the convenience outweighs an audit of fixing all callers); but if we do that, then we might also want to intentionally switch existing callers to pass negative values rather than declaring that passing a negative value is a bug.
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
> -----Original Message----- > From: Paolo Bonzini [mailto:paolo.bonzini@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: armbru@redhat.com; paolo.bonzini@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.
Paolo Bonzini <bonzini@gnu.org> 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.
> -----Original Message----- > From: SeokYeon Hwang [mailto:syeon.hwang@samsung.com] > Sent: Wednesday, November 05, 2014 10:13 PM > To: 'Paolo Bonzini'; 'Max Reitz'; 'qemu-devel@nongnu.org' > Cc: 'armbru@redhat.com'; 'paolo.bonzini@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.bonzini@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: armbru@redhat.com; paolo.bonzini@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?? Thanks.
On Wed, Nov 05, 2014 at 12:19:34PM +0100, Eric Blake wrote: > On 11/05/2014 12:11 PM, Max Reitz wrote: > > >>>> + err->msg = g_strdup_printf("%s: %s", msg1, > >>>> strerror(abs(os_errno))); > > >> I don't, we really should fix the callers. > > > > 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 > > POSIX requires all defined errno values to be positive; negative errno > values are unambiguous as values that will cause strerror() to have to > generate a message about an unknown value. > > > (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. > > This patch is a bandaid; it makes it harder to find callers that need to > be fixed. I'd almost argue the exact opposite - add an assert(os_errno > > 0). Then we'd loudly break on broken callers, making them easier to find. Agree! use an assert to teach the caller ;) > > 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. > > If we're willing to accept the convenience so that callers can be lazy, > then I like this patch. If we want to fix bugs in the callers, then > this patch makes it harder to find those bugs. > > I'm actually 60:40 in favor of this patch (I think the convenience > outweighs an audit of fixing all callers); but if we do that, then we > might also want to intentionally switch existing callers to pass > negative values rather than declaring that passing a negative value is a > bug.
SeokYeon Hwang <syeon.hwang@samsung.com> writes: >> -----Original Message----- >> From: SeokYeon Hwang [mailto:syeon.hwang@samsung.com] >> Sent: Wednesday, November 05, 2014 10:13 PM >> To: 'Paolo Bonzini'; 'Max Reitz'; 'qemu-devel@nongnu.org' >> Cc: 'armbru@redhat.com'; 'paolo.bonzini@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.bonzini@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: armbru@redhat.com; paolo.bonzini@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.
> -----Original Message----- > From: Markus Armbruster [mailto:armbru@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.bonzini@gmail.com > Subject: Re: [Qemu-devel] [PATCH] error: fixed error_set_errno() to deal > with a negative type of os_error. > > SeokYeon Hwang <syeon.hwang@samsung.com> writes: > > >> -----Original Message----- > >> From: SeokYeon Hwang [mailto:syeon.hwang@samsung.com] > >> Sent: Wednesday, November 05, 2014 10:13 PM > >> To: 'Paolo Bonzini'; 'Max Reitz'; 'qemu-devel@nongnu.org' > >> Cc: 'armbru@redhat.com'; 'paolo.bonzini@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.bonzini@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: armbru@redhat.com; paolo.bonzini@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.
diff --git a/util/error.c b/util/error.c index 2ace0d8..5db00c9 100644 --- a/util/error.c +++ b/util/error.c @@ -68,7 +68,7 @@ void error_set_errno(Error **errp, int os_errno, ErrorClass err_class, va_start(ap, fmt); msg1 = g_strdup_vprintf(fmt, ap); if (os_errno != 0) { - err->msg = g_strdup_printf("%s: %s", msg1, strerror(os_errno)); + err->msg = g_strdup_printf("%s: %s", msg1, strerror(abs(os_errno))); g_free(msg1); } else { err->msg = msg1;
Negative type of errno like -ERRNO is used a lot by developers. Therefore, error_set_errno() is modified to deal with a negative type of os_error. (Negative type is used at pcie_cap_slot_hotplug_common() in hw/pci/pcie.c) Signed-off-by: SeokYeon Hwang <syeon.hwang@samsung.com> --- util/error.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)