| Message ID | ffa2cc4d0706fa522a5e581af14dadb03aec323a.1496387804.git.maozy.fnst@cn.fujitsu.com |
|---|---|
| State | New |
| Headers | show |
On Fri, Jun 02, 2017 at 03:54:37PM +0800, Mao Zhongyi wrote: > On success, pci_add_capability2() returns a positive value. On > failure, it sets an error and return a negative value. > > pci_add_capability() laboriously checks this behavior. No other > caller does. Drop the checks from pci_add_capability(). > > Cc: mst@redhat.com > Cc: marcel@redhat.com > Cc: armbru@redhat.com > Signed-off-by: Mao Zhongyi <maozy.fnst@cn.fujitsu.com> > Reviewed-by: Marcel Apfelbaum <marcel@redhat.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 98ccc27..53566b8 100644 > --- a/hw/pci/pci.c > +++ b/hw/pci/pci.c > @@ -2270,12 +2270,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; > } I don't see why this is a good idea. You drop a bunch of asserts, so naturally code is slightly tighter. We could gain the same by building with NDEBUG but we don't, we rather have more safety. > -- > 2.9.3 > > >
"Michael S. Tsirkin" <mst@redhat.com> writes: > On Fri, Jun 02, 2017 at 03:54:37PM +0800, Mao Zhongyi wrote: >> On success, pci_add_capability2() returns a positive value. On >> failure, it sets an error and return a negative value. >> >> pci_add_capability() laboriously checks this behavior. No other >> caller does. Drop the checks from pci_add_capability(). >> >> Cc: mst@redhat.com >> Cc: marcel@redhat.com >> Cc: armbru@redhat.com >> Signed-off-by: Mao Zhongyi <maozy.fnst@cn.fujitsu.com> >> Reviewed-by: Marcel Apfelbaum <marcel@redhat.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 98ccc27..53566b8 100644 >> --- a/hw/pci/pci.c >> +++ b/hw/pci/pci.c >> @@ -2270,12 +2270,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; >> } > > > I don't see why this is a good idea. You drop a bunch of > asserts, so naturally code is slightly tighter. We could gain > the same by building with NDEBUG but we don't, we rather > have more safety. It's a good idea because it's what we do basically everywhere when a function sets an error and returns a distinct error value.
On Tue, Jun 06, 2017 at 06:14:02PM +0200, Markus Armbruster wrote: > "Michael S. Tsirkin" <mst@redhat.com> writes: > > > On Fri, Jun 02, 2017 at 03:54:37PM +0800, Mao Zhongyi wrote: > >> On success, pci_add_capability2() returns a positive value. On > >> failure, it sets an error and return a negative value. > >> > >> pci_add_capability() laboriously checks this behavior. No other > >> caller does. Drop the checks from pci_add_capability(). > >> > >> Cc: mst@redhat.com > >> Cc: marcel@redhat.com > >> Cc: armbru@redhat.com > >> Signed-off-by: Mao Zhongyi <maozy.fnst@cn.fujitsu.com> > >> Reviewed-by: Marcel Apfelbaum <marcel@redhat.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 98ccc27..53566b8 100644 > >> --- a/hw/pci/pci.c > >> +++ b/hw/pci/pci.c > >> @@ -2270,12 +2270,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; > >> } > > > > > > I don't see why this is a good idea. You drop a bunch of > > asserts, so naturally code is slightly tighter. We could gain > > the same by building with NDEBUG but we don't, we rather > > have more safety. > > It's a good idea because it's what we do basically everywhere when a > function sets an error and returns a distinct error value. We typically just do if (local_err) { error_report_err(local_err); ... }
"Michael S. Tsirkin" <mst@redhat.com> writes: > On Tue, Jun 06, 2017 at 06:14:02PM +0200, Markus Armbruster wrote: >> "Michael S. Tsirkin" <mst@redhat.com> writes: >> >> > On Fri, Jun 02, 2017 at 03:54:37PM +0800, Mao Zhongyi wrote: >> >> On success, pci_add_capability2() returns a positive value. On >> >> failure, it sets an error and return a negative value. >> >> >> >> pci_add_capability() laboriously checks this behavior. No other >> >> caller does. Drop the checks from pci_add_capability(). >> >> >> >> Cc: mst@redhat.com >> >> Cc: marcel@redhat.com >> >> Cc: armbru@redhat.com >> >> Signed-off-by: Mao Zhongyi <maozy.fnst@cn.fujitsu.com> >> >> Reviewed-by: Marcel Apfelbaum <marcel@redhat.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 98ccc27..53566b8 100644 >> >> --- a/hw/pci/pci.c >> >> +++ b/hw/pci/pci.c >> >> @@ -2270,12 +2270,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; >> >> } >> > >> > >> > I don't see why this is a good idea. You drop a bunch of >> > asserts, so naturally code is slightly tighter. We could gain >> > the same by building with NDEBUG but we don't, we rather >> > have more safety. >> >> It's a good idea because it's what we do basically everywhere when a >> function sets an error and returns a distinct error value. > > We typically just do > func(arg1, arg2, &local_err); > if (local_err) { > error_report_err(local_err); > ... > } That one's fine. Equally fine: ret = func(arg1, arg2, &local_err); if (ret < 0) { error_report_err(local_err); ... } When func(...) is short, then if (func(arg1, arg2, &local_err) < 0) { error_report_err(local_err); ... } is also fine. Whether you check the Error * variable or a distinct error return value is purely a matter of taste (I prefer return value when possible). Keeping matters of taste locally consistent can make sense. What matters is keeping the error checking short and to the point. Asserting that the callee returns the distinct error value if and only if it sets an error is unusual precisely because it's not worth the distraction. Mao Zhongyi's patch cleans it up here.
diff --git a/hw/pci/pci.c b/hw/pci/pci.c index 98ccc27..53566b8 100644 --- a/hw/pci/pci.c +++ b/hw/pci/pci.c @@ -2270,12 +2270,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; }