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

[Mao Zhongyi]

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?


> 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?

> >      }
> >
> >      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.

Mao