* [Qemu-devel] [PATCH] pci: Fix unreasonable return value check @ 2017-05-31 7:04 Mao Zhongyi 2017-05-31 11:07 ` Markus Armbruster 0 siblings, 1 reply; 4+ messages in thread From: Mao Zhongyi @ 2017-05-31 7:04 UTC (permalink / raw) To: qemu-devel; +Cc: mst, marcel, armbru The return value of pci_add_capability2() is only 2 cases, positive on success, nagetive on failure and set error message to Error. In other worlds, If Error is filled, the return value must be nagetive. There is no case where errp is set but the return value is a positive. But pci_add_capability() does. So the return value check is illogical. Meanwhile, all other callers of pci_add_capability2() do the same check as this patch. So fix it. Signed-off-by: Mao Zhongyi <maozy.fnst@cn.fujitsu.com> --- hw/pci/pci.c | 6 +----- 1 file changed, 1 insertion(+), 5 deletions(-) diff --git a/hw/pci/pci.c b/hw/pci/pci.c index 259483b..1faf060 100644 --- a/hw/pci/pci.c +++ b/hw/pci/pci.c @@ -2269,12 +2269,8 @@ int pci_add_capability(PCIDevice *pdev, uint8_t cap_id, Error *local_err = NULL; ret = pci_add_capability2(pdev, cap_id, offset, size, &local_err); - if (local_err) { - assert(ret < 0); + if (ret < 0) { error_report_err(local_err); - } else { - /* success implies a positive offset in config space */ - assert(ret > 0); } return ret; } -- 2.9.3 ^ permalink raw reply related [flat|nested] 4+ messages in thread
* Re: [Qemu-devel] [PATCH] pci: Fix unreasonable return value check 2017-05-31 7:04 [Qemu-devel] [PATCH] pci: Fix unreasonable return value check Mao Zhongyi @ 2017-05-31 11:07 ` Markus Armbruster 2017-06-01 2:51 ` Mao Zhongyi 0 siblings, 1 reply; 4+ messages in thread From: Markus Armbruster @ 2017-05-31 11:07 UTC (permalink / raw) To: Mao Zhongyi; +Cc: qemu-devel, marcel, mst This is cleanup, not a bug fix, so: pci: Clean up error checking in pci_add_capability() Mao Zhongyi <maozy.fnst@cn.fujitsu.com> writes: > The return value of pci_add_capability2() is only 2 cases, positive > on success, nagetive on failure and set error message to Error. In negative > other worlds, If Error is filled, the return value must be nagetive. words, if > There is no case where errp is set but the return value is a positive. > But pci_add_capability() does. So the return value check is illogical. pci_add_capability2() could use a function comment explaining its return value. Not this patch's job. > Meanwhile, all other callers of pci_add_capability2() do the same > check as this patch. So fix it. Suggest: pci: Clean up error checking in pci_add_capability() On success, pci_add_capability2() returns a positive value. On failure, it sets an error and returns a negative value. pci_add_capability() laboriously checks this behavior. No other caller does. Drop the checks from pci_add_capability(). > Signed-off-by: Mao Zhongyi <maozy.fnst@cn.fujitsu.com> > --- > hw/pci/pci.c | 6 +----- > 1 file changed, 1 insertion(+), 5 deletions(-) > > diff --git a/hw/pci/pci.c b/hw/pci/pci.c > index 259483b..1faf060 100644 > --- a/hw/pci/pci.c > +++ b/hw/pci/pci.c > @@ -2269,12 +2269,8 @@ int pci_add_capability(PCIDevice *pdev, uint8_t cap_id, > Error *local_err = NULL; > > ret = pci_add_capability2(pdev, cap_id, offset, size, &local_err); > - if (local_err) { > - assert(ret < 0); > + if (ret < 0) { > error_report_err(local_err); > - } else { > - /* success implies a positive offset in config space */ > - assert(ret > 0); > } > return ret; > } Many functions return distinct error values in addition to setting an error. We usually check one of the two, and assume the other is sane. This is one of the few places where we assert it is. Not wrong, just cumbersome. I'd prefer to drop the assertions, i.e. take this patch. But it's up to the PCI maintainers. ^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [Qemu-devel] [PATCH] pci: Fix unreasonable return value check 2017-05-31 11:07 ` Markus Armbruster @ 2017-06-01 2:51 ` Mao Zhongyi 2017-06-01 7:43 ` Marcel Apfelbaum 0 siblings, 1 reply; 4+ messages in thread From: Mao Zhongyi @ 2017-06-01 2:51 UTC (permalink / raw) To: Markus Armbruster; +Cc: qemu-devel, marcel, mst Hi, Markus On 05/31/2017 07:07 PM, Markus Armbruster wrote: > This is cleanup, not a bug fix, so: > > pci: Clean up error checking in pci_add_capability() > > Mao Zhongyi <maozy.fnst@cn.fujitsu.com> writes: > >> The return value of pci_add_capability2() is only 2 cases, positive >> on success, nagetive on failure and set error message to Error. In > > negative > >> other worlds, If Error is filled, the return value must be nagetive. > > words, if > >> There is no case where errp is set but the return value is a positive. >> But pci_add_capability() does. So the return value check is illogical. > > pci_add_capability2() could use a function comment explaining its return > value. Not this patch's job. > Thanks, will make a separated patch to explain it. >> Meanwhile, all other callers of pci_add_capability2() do the same >> check as this patch. So fix it. > > Suggest: > > pci: Clean up error checking in pci_add_capability() > > On success, pci_add_capability2() returns a positive value. On > failure, it sets an error and returns a negative value. > > pci_add_capability() laboriously checks this behavior. No other > caller does. Drop the checks from pci_add_capability(). Thanks for your perfect suggestion. > >> Signed-off-by: Mao Zhongyi <maozy.fnst@cn.fujitsu.com> >> --- >> hw/pci/pci.c | 6 +----- >> 1 file changed, 1 insertion(+), 5 deletions(-) >> >> diff --git a/hw/pci/pci.c b/hw/pci/pci.c >> index 259483b..1faf060 100644 >> --- a/hw/pci/pci.c >> +++ b/hw/pci/pci.c >> @@ -2269,12 +2269,8 @@ int pci_add_capability(PCIDevice *pdev, uint8_t cap_id, >> Error *local_err = NULL; >> >> ret = pci_add_capability2(pdev, cap_id, offset, size, &local_err); >> - if (local_err) { >> - assert(ret < 0); >> + if (ret < 0) { >> error_report_err(local_err); >> - } else { >> - /* success implies a positive offset in config space */ >> - assert(ret > 0); >> } >> return ret; >> } > > Many functions return distinct error values in addition to setting an > error. We usually check one of the two, and assume the other is sane. > This is one of the few places where we assert it is. Not wrong, just > cumbersome. I'd prefer to drop the assertions, i.e. take this patch. > But it's up to the PCI maintainers. Yes, I also think it really is not necessary. Keeping code as simple as practical is desirable. So drop the assertions. Of course, I will listen to the views of Marcel and Michael. Thanks Mao ^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [Qemu-devel] [PATCH] pci: Fix unreasonable return value check 2017-06-01 2:51 ` Mao Zhongyi @ 2017-06-01 7:43 ` Marcel Apfelbaum 0 siblings, 0 replies; 4+ messages in thread From: Marcel Apfelbaum @ 2017-06-01 7:43 UTC (permalink / raw) To: Mao Zhongyi, Markus Armbruster; +Cc: qemu-devel, mst On 01/06/2017 5:51, Mao Zhongyi wrote: > Hi, Markus > > On 05/31/2017 07:07 PM, Markus Armbruster wrote: >> This is cleanup, not a bug fix, so: >> >> pci: Clean up error checking in pci_add_capability() >> >> Mao Zhongyi <maozy.fnst@cn.fujitsu.com> writes: >> >>> The return value of pci_add_capability2() is only 2 cases, positive >>> on success, nagetive on failure and set error message to Error. In >> >> negative >> >>> other worlds, If Error is filled, the return value must be nagetive. >> >> words, if >> >>> There is no case where errp is set but the return value is a positive. >>> But pci_add_capability() does. So the return value check is illogical. >> >> pci_add_capability2() could use a function comment explaining its return >> value. Not this patch's job. >> > > Thanks, will make a separated patch to explain it. > >>> Meanwhile, all other callers of pci_add_capability2() do the same >>> check as this patch. So fix it. >> >> Suggest: >> >> pci: Clean up error checking in pci_add_capability() >> >> On success, pci_add_capability2() returns a positive value. On >> failure, it sets an error and returns a negative value. >> >> pci_add_capability() laboriously checks this behavior. No other >> caller does. Drop the checks from pci_add_capability(). > > Thanks for your perfect suggestion. > >> >>> Signed-off-by: Mao Zhongyi <maozy.fnst@cn.fujitsu.com> >>> --- >>> hw/pci/pci.c | 6 +----- >>> 1 file changed, 1 insertion(+), 5 deletions(-) >>> >>> diff --git a/hw/pci/pci.c b/hw/pci/pci.c >>> index 259483b..1faf060 100644 >>> --- a/hw/pci/pci.c >>> +++ b/hw/pci/pci.c >>> @@ -2269,12 +2269,8 @@ int pci_add_capability(PCIDevice *pdev, >>> uint8_t cap_id, >>> Error *local_err = NULL; >>> >>> ret = pci_add_capability2(pdev, cap_id, offset, size, &local_err); >>> - if (local_err) { >>> - assert(ret < 0); >>> + if (ret < 0) { >>> error_report_err(local_err); >>> - } else { >>> - /* success implies a positive offset in config space */ >>> - assert(ret > 0); >>> } >>> return ret; >>> } >> >> Many functions return distinct error values in addition to setting an >> error. We usually check one of the two, and assume the other is sane. >> This is one of the few places where we assert it is. Not wrong, just >> cumbersome. I'd prefer to drop the assertions, i.e. take this patch. >> But it's up to the PCI maintainers. > > Yes, I also think it really is not necessary. Keeping code as simple as > practical is desirable. So drop the assertions. Of course, I will listen > to the views of Marcel and Michael. > I have nothing against it. Thanks for the patch! With Markus comments: Reviewed-by: Marcel Apfelbaum <marcel@redhat.com> Thanks, Marcel > Thanks > Mao > > > ^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2017-06-01 7:44 UTC | newest] Thread overview: 4+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2017-05-31 7:04 [Qemu-devel] [PATCH] pci: Fix unreasonable return value check Mao Zhongyi 2017-05-31 11:07 ` Markus Armbruster 2017-06-01 2:51 ` Mao Zhongyi 2017-06-01 7:43 ` Marcel Apfelbaum
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).