qemu-devel
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [Qemu-devel] [PATCH v2 1/6] pci: Clean up error checking in pci_add_


From: Markus Armbruster
Subject: Re: [Qemu-devel] [PATCH v2 1/6] pci: Clean up error checking in pci_add_capability()
Date: Tue, 06 Jun 2017 20:53:31 +0200
User-agent: Gnus/5.13 (Gnus v5.13) Emacs/25.2 (gnu/linux)

"Michael S. Tsirkin" <address@hidden> writes:

> On Tue, Jun 06, 2017 at 06:14:02PM +0200, Markus Armbruster wrote:
>> "Michael S. Tsirkin" <address@hidden> 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: address@hidden
>> >> Cc: address@hidden
>> >> Cc: address@hidden
>> >> Signed-off-by: Mao Zhongyi <address@hidden>
>> >> Reviewed-by: Marcel Apfelbaum <address@hidden>
>> >> ---
>> >>  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.



reply via email to

[Prev in Thread] Current Thread [Next in Thread]