qemu-devel
[Top][All Lists]
Advanced

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

Re: [Qemu-devel] [PATCH v3] net/rocker: Convert to realize


From: Markus Armbruster
Subject: Re: [Qemu-devel] [PATCH v3] net/rocker: Convert to realize
Date: Wed, 17 May 2017 09:04:54 +0200
User-agent: Gnus/5.13 (Gnus v5.13) Emacs/25.2 (gnu/linux)

Mao Zhongyi <address@hidden> writes:

> Hi, Markus
>
> On 05/16/2017 11:29 PM, Markus Armbruster wrote:
>> Mao Zhongyi <address@hidden> writes:
>>
>>> The rocker device still implements the old PCIDeviceClass .init()
>>> instead of the new .realize(). All devices need to be converted to
>>> .realize().
>>
>> Thanks for chipping in!
>>
>>> .init() reports errors with fprintf() and return 0 on success, negative
>>> number on failure. Meanwhile, when -device rocker fails, it first report
>>> a specific error, then a generic one, like this:
>>>
>>>     $ x86_64-softmmu/qemu-system-x86_64 -device rocker,name=qemu-rocker
>>>     rocker: name too long; please shorten to at most 9 chars
>>>     qemu-system-x86_64: -device rocker,name=qemu-rocker: Device 
>>> initialization failed
>>>
>>> Now, convert it to .realize() that passes errors to its callers via its
>>> errp argument. Also avoid the superfluous second error message.
>>
>> Recommend to show the error message after your patch here:
>>
>>       qemu-system-x86_64: -device rocker,name=qemu-rocker: rocker: name too 
>> long; please shorten to at most 9 chars
>
> Thanks, I think I got it.
>
>>
>> Not least because that makes it blatantly obvious that keeping the
>> "rocker: " is not a good idea :)
>
> Actually, I was always curious about why there are 2 "rocker" strings
> in the report, it's superfluous. But in order to keep a consistent log
> format, so inherited the original style.
>
> Will remove it in the next version.
>
>>
>>> Cc: address@hidden
>>> Cc: address@hidden
>>> Cc: address@hidden
>>> Signed-off-by: Mao Zhongyi <address@hidden>
>>> ---
>>>  hw/net/rocker/rocker.c | 35 +++++++++++++++++------------------
>>>  1 file changed, 17 insertions(+), 18 deletions(-)
>>>
>>> diff --git a/hw/net/rocker/rocker.c b/hw/net/rocker/rocker.c
>>> index 6e70fdd..c446cda 100644
>>> --- a/hw/net/rocker/rocker.c
>>> +++ b/hw/net/rocker/rocker.c
>>> @@ -1252,20 +1252,18 @@ rollback:
>>>      return err;
>>>  }
>>>
>>> -static int rocker_msix_init(Rocker *r)
>>> +static int rocker_msix_init(Rocker *r, Error **errp)
>>>  {
>>>      PCIDevice *dev = PCI_DEVICE(r);
>>>      int err;
>>> -    Error *local_err = NULL;
>>>
>>>      err = msix_init(dev, ROCKER_MSIX_VEC_COUNT(r->fp_ports),
>>>                      &r->msix_bar,
>>>                      ROCKER_PCI_MSIX_BAR_IDX, ROCKER_PCI_MSIX_TABLE_OFFSET,
>>>                      &r->msix_bar,
>>>                      ROCKER_PCI_MSIX_BAR_IDX, ROCKER_PCI_MSIX_PBA_OFFSET,
>>> -                    0, &local_err);
>>> +                    0, errp);
>>>      if (err) {
>>> -        error_report_err(local_err);
>>>          return err;
>>>      }
>>>
>>> @@ -1301,7 +1299,7 @@ static World *rocker_world_type_by_name(Rocker *r, 
>>> const char *name)
>>>      return NULL;
>>>  }
>>>
>>> -static int pci_rocker_init(PCIDevice *dev)
>>> +static void pci_rocker_realize(PCIDevice *dev, Error **errp)
>>>  {
>>>      Rocker *r = to_rocker(dev);
>>>      const MACAddr zero = { .a = { 0, 0, 0, 0, 0, 0 } };
>>> @@ -1315,7 +1313,7 @@ static int pci_rocker_init(PCIDevice *dev)
>>>
>>>      for (i = 0; i < ROCKER_WORLD_TYPE_MAX; i++) {
>>>          if (!r->worlds[i]) {
>>> -            err = -ENOMEM;
>>> +            error_setg(errp, "rocker: memory allocation for worlds 
>>> failed");
>>
>> r->worlds[i] is null when of_dpa_world_alloc() returns null.  It's a
>> wrapper around world_alloc(), which returns null only when g_malloc()
>> does.  It doesn't.  Please remove the dead error handling.  Ideally in a
>> separate cleanup patch before this one, to facilitate review.
>>
>
> Thanks very much for your detailed explanation.
>
> After reading g_malloc0(), I am aware of this: g_malloc0(size_t size)
> returns null only when size is 0. But it is a wrapper around
> g_malloc0_n(1, size) that ignore the fact that g_malloc0() of 0 bytes
> returns null. So it doesn't return null. Am I right?

Correct, it can't return null here.

Aside: even when it does return null for zero size, that null is *not*
an error!

>> Recommend to drop the "rocker: " prefix.  Same for all the other error
>> messages.
>>
>
> Thanks, will dorp it entirely.
>
>>>              goto err_world_alloc;
>>>          }
>>>      }
>>> @@ -1326,10 +1324,9 @@ static int pci_rocker_init(PCIDevice *dev)
>>>
>>>      r->world_dflt = rocker_world_type_by_name(r, r->world_name);
>>>      if (!r->world_dflt) {
>>> -        fprintf(stderr,
>>> -                "rocker: requested world \"%s\" does not exist\n",
>>> +        error_setg(errp,
>>> +                "rocker: invalid argument, requested world %s does not 
>>> exist",
>>>                  r->world_name);
>>> -        err = -EINVAL;
>>>          goto err_world_type_by_name;
>>>      }
>>>
>>> @@ -1349,7 +1346,7 @@ static int pci_rocker_init(PCIDevice *dev)
>>>
>>>      /* MSI-X init */
>>>
>>> -    err = rocker_msix_init(r);
>>> +    err = rocker_msix_init(r, errp);
>>>      if (err) {
>>>          goto err_msix_init;
>>>      }
>>> @@ -1361,7 +1358,7 @@ static int pci_rocker_init(PCIDevice *dev)
>>>      }
>>>
>>>      if (rocker_find(r->name)) {
>>> -        err = -EEXIST;
>>> +        error_setg(errp, "rocker: %s already exists", r->name);
>>>          goto err_duplicate;
>>>      }
>>>
>>> @@ -1375,10 +1372,10 @@ static int pci_rocker_init(PCIDevice *dev)
>>>  #define ROCKER_IFNAMSIZ 16
>>>  #define MAX_ROCKER_NAME_LEN  (ROCKER_IFNAMSIZ - 1 - 3 - 3)
>>>      if (strlen(r->name) > MAX_ROCKER_NAME_LEN) {
>>> -        fprintf(stderr,
>>> -                "rocker: name too long; please shorten to at most %d 
>>> chars\n",
>>> +        error_setg(errp,
>>> +                "rocker: name too long; please shorten to at most %d 
>>> chars",
>>>                  MAX_ROCKER_NAME_LEN);
>>> -        return -EINVAL;
>>> +        goto err_name_too_long;
>>
>> Is this a bug fix?
>
> Before the patch, it will return a negative value when the name more
> than 9 chars. But it doesn't free the memory of world and msix that
> has allocated previously.
>
> After the patch, I think the cleanup is more entire. doesn't it?

Sounds like you're plugging a memory leak.  I recommend to plug it in a
separate patch before this one, because that way your bug fix is
properly visible in the commit log.

>>>      }
>>>
>>>      if (memcmp(&r->fp_start_macaddr, &zero, sizeof(zero)) == 0) {
>>> @@ -1397,6 +1394,7 @@ static int pci_rocker_init(PCIDevice *dev)
>>>
>>>      r->rings = g_new(DescRing *, rocker_pci_ring_count(r));
>>>      if (!r->rings) {
>>> +        error_setg(errp, "rocker: memory allocation for rings failed");
>>>          goto err_rings_alloc;
>>>      }
>>
>> g_new() can't fail.  Please remove the dead error handling.
>
> Thanks, will drop it.
>
>>
>>>
>>> @@ -1410,11 +1408,11 @@ static int pci_rocker_init(PCIDevice *dev)
>>>       * .....
>>>       */
>>>
>>> -    err = -ENOMEM;
>>>      for (i = 0; i < rocker_pci_ring_count(r); i++) {
>>>          DescRing *ring = desc_ring_alloc(r, i);
>>>
>>>          if (!ring) {
>>> +            error_setg(errp, "rocker: memory allocation for ring failed");
>>>              goto err_ring_alloc;
>>>          }
>>>
>>
>> desc_ring_alloc() returns null only when g_new0() does.  It doesn't.
>> Please remove the dead error handling here and in desc_ring_alloc().
>>
>
> I got it, will remove it in the next version.
> Thanks
>
>>> @@ -1438,6 +1436,7 @@ static int pci_rocker_init(PCIDevice *dev)
>>>                            i, &r->fp_ports_peers[i]);
>>>
>>>          if (!port) {
>>> +            error_setg(errp, "rocker: memory allocation for port failed");
>>>              goto err_port_alloc;
>>>          }
>>>
>>
>> Likewise for fp_port_alloc().
>>
>> I recommend you search all of rocker/ for similarly dead error checking
>> after g_malloc() & friends.
>
> I'll search and fix similar error checking.
>
>>
>>> @@ -1447,7 +1446,7 @@ static int pci_rocker_init(PCIDevice *dev)
> [...]
>
> Thanks for your quick review:), will do the fix right away.

Looking forward to v2 :)



reply via email to

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