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

[Markus Armbruster]

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

Not least because that makes it blatantly obvious that keeping the
"rocker: " is not a good idea :)

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

Recommend to drop the "rocker: " prefix.  Same for all the other error
messages.

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

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

>  
> @@ -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().

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

> @@ -1447,7 +1446,7 @@ static int pci_rocker_init(PCIDevice *dev)
>  
>      QLIST_INSERT_HEAD(&rockers, r, next);
>  
> -    return 0;
> +    return;
>  
>  err_port_alloc:
>      for (--i; i >= 0; i--) {
> @@ -1461,6 +1460,7 @@ err_ring_alloc:
>      }
>      g_free(r->rings);
>  err_rings_alloc:
> +err_name_too_long:
>  err_duplicate:
>      rocker_msix_uninit(r);
>  err_msix_init:
> @@ -1473,7 +1473,6 @@ err_world_alloc:
>              world_free(r->worlds[i]);
>          }
>      }
> -    return err;
>  }
>  
>  static void pci_rocker_uninit(PCIDevice *dev)
> @@ -1558,7 +1557,7 @@ static void rocker_class_init(ObjectClass *klass, void 
> *data)
>      DeviceClass *dc = DEVICE_CLASS(klass);
>      PCIDeviceClass *k = PCI_DEVICE_CLASS(klass);
>  
> -    k->init = pci_rocker_init;
> +    k->realize = pci_rocker_realize;
>      k->exit = pci_rocker_uninit;
>      k->vendor_id = PCI_VENDOR_ID_REDHAT;
>      k->device_id = PCI_DEVICE_ID_REDHAT_ROCKER;