[Qemu-devel] [PATCH v4 0/3] Convert to realize and fix error handling

[Qemu-devel] [PATCH v4 0/3] Convert to realize and fix error handling

[Mao Zhongyi]

v4:
* Patch 1 is following Markus's suggestion that remove the dead error
  handling.
* Patch 2 is separate from patch 1 to plug the memory leak in the v3.
* Patch 3 is based on the patch 1 in the v3. Meanwhile, dorp the superfluous
  prefix "rocker:" and adjust the commit message.

v3:
* Following Jason's suggstion that add suitable error message to 
  each error site.
* Modified the commit message to make it easier to read.

v2:
* Following Philippe's suggestion that shorten the patch subject 
  "hw/net/rocker/rocker" to "net/rocker". 
* Use a consistent log format to report error message.
* Add a specific goto label "err_name_too_long" to make a correct
  cleanup.

Cc: jasowang@redhat.com
Cc: jiri@resnulli.us
Cc: f4bug@amsat.org
Cc: armbru@redhat.com
Mao Zhongyi (3):
  net/rocker: Remove the dead error handling
  net/rocker: Plug memory leak in pci_rocker_init()
  net/rocker: Convert to realize()

 hw/net/rocker/rocker.c      | 61 ++++++++++-----------------------------------
 hw/net/rocker/rocker_desc.c |  3 ---
 hw/net/rocker/rocker_fp.c   |  4 ---
 3 files changed, 13 insertions(+), 55 deletions(-)

-- 
2.9.3

[Qemu-devel] [PATCH v4 1/3] net/rocker: Remove the dead error handling

[Mao Zhongyi]

The function of_dpa_world_alloc is a wrapper around world_alloc(), which
returns null only when g_malloc0(size_t size) does. But g_malloc0() also
is a wrapper around g_malloc0_n(1, size) that ignore the fact that
g_malloc0() of 0 bytes, it doesn't returns null. So the error handling is
dead. Similar like desc_ring_alloc(), fp_port_alloc() etc. Remove these
entirely.

Cc: jasowang@redhat.com
Cc: jiri@resnulli.us
Cc: f4bug@amsat.org
Cc: armbru@redhat.com
Signed-off-by: Mao Zhongyi <maozy.fnst@cn.fujitsu.com>
---
 hw/net/rocker/rocker.c      | 32 --------------------------------
 hw/net/rocker/rocker_desc.c |  3 ---
 hw/net/rocker/rocker_fp.c   |  4 ----
 3 files changed, 39 deletions(-)

diff --git a/hw/net/rocker/rocker.c b/hw/net/rocker/rocker.c
index 6e70fdd..b2b6dc7 100644
--- a/hw/net/rocker/rocker.c
+++ b/hw/net/rocker/rocker.c
@@ -1313,13 +1313,6 @@ static int pci_rocker_init(PCIDevice *dev)
 
     r->worlds[ROCKER_WORLD_TYPE_OF_DPA] = of_dpa_world_alloc(r);
 
-    for (i = 0; i < ROCKER_WORLD_TYPE_MAX; i++) {
-        if (!r->worlds[i]) {
-            err = -ENOMEM;
-            goto err_world_alloc;
-        }
-    }
-
     if (!r->world_name) {
         r->world_name = g_strdup(world_name(r->worlds[ROCKER_WORLD_TYPE_OF_DPA]));
     }
@@ -1396,9 +1389,6 @@ static int pci_rocker_init(PCIDevice *dev)
     }
 
     r->rings = g_new(DescRing *, rocker_pci_ring_count(r));
-    if (!r->rings) {
-        goto err_rings_alloc;
-    }
 
     /* Rings are ordered like this:
      * - command ring
@@ -1410,14 +1400,9 @@ 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) {
-            goto err_ring_alloc;
-        }
-
         if (i == ROCKER_RING_CMD) {
             desc_ring_set_consume(ring, cmd_consume, ROCKER_MSIX_VEC_CMD);
         } else if (i == ROCKER_RING_EVENT) {
@@ -1437,10 +1422,6 @@ static int pci_rocker_init(PCIDevice *dev)
             fp_port_alloc(r, r->name, &r->fp_start_macaddr,
                           i, &r->fp_ports_peers[i]);
 
-        if (!port) {
-            goto err_port_alloc;
-        }
-
         r->fp_port[i] = port;
         fp_port_set_world(port, r->world_dflt);
     }
@@ -1449,25 +1430,12 @@ static int pci_rocker_init(PCIDevice *dev)
 
     return 0;
 
-err_port_alloc:
-    for (--i; i >= 0; i--) {
-        FpPort *port = r->fp_port[i];
-        fp_port_free(port);
-    }
-    i = rocker_pci_ring_count(r);
-err_ring_alloc:
-    for (--i; i >= 0; i--) {
-        desc_ring_free(r->rings[i]);
-    }
-    g_free(r->rings);
-err_rings_alloc:
 err_duplicate:
     rocker_msix_uninit(r);
 err_msix_init:
     object_unparent(OBJECT(&r->msix_bar));
     object_unparent(OBJECT(&r->mmio));
 err_world_type_by_name:
-err_world_alloc:
     for (i = 0; i < ROCKER_WORLD_TYPE_MAX; i++) {
         if (r->worlds[i]) {
             world_free(r->worlds[i]);
diff --git a/hw/net/rocker/rocker_desc.c b/hw/net/rocker/rocker_desc.c
index ac02797..bd7e364 100644
--- a/hw/net/rocker/rocker_desc.c
+++ b/hw/net/rocker/rocker_desc.c
@@ -347,9 +347,6 @@ DescRing *desc_ring_alloc(Rocker *r, int index)
     DescRing *ring;
 
     ring = g_new0(DescRing, 1);
-    if (!ring) {
-        return NULL;
-    }
 
     ring->r = r;
     ring->index = index;
diff --git a/hw/net/rocker/rocker_fp.c b/hw/net/rocker/rocker_fp.c
index 1305ac3..4b3c984 100644
--- a/hw/net/rocker/rocker_fp.c
+++ b/hw/net/rocker/rocker_fp.c
@@ -226,10 +226,6 @@ FpPort *fp_port_alloc(Rocker *r, char *sw_name,
 {
     FpPort *port = g_new0(FpPort, 1);
 
-    if (!port) {
-        return NULL;
-    }
-
     port->r = r;
     port->index = index;
     port->pport = index + 1;
-- 
2.9.3

[Qemu-devel] [PATCH v4 2/3] net/rocker: Plug memory leak in pci_rocker_init()

[Mao Zhongyi]

pci_rocker_init() leaks a World when the name more than 9 chars,
then return a negative value directly, doesn't make a correct
cleanup. So add a new goto label to fix it.

Cc: jasowang@redhat.com
Cc: jiri@resnulli.us
Cc: f4bug@amsat.org
Cc: armbru@redhat.com
Signed-off-by: Mao Zhongyi <maozy.fnst@cn.fujitsu.com>
---
 hw/net/rocker/rocker.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/hw/net/rocker/rocker.c b/hw/net/rocker/rocker.c
index b2b6dc7..a382a6f 100644
--- a/hw/net/rocker/rocker.c
+++ b/hw/net/rocker/rocker.c
@@ -1371,7 +1371,8 @@ static int pci_rocker_init(PCIDevice *dev)
         fprintf(stderr,
                 "rocker: name too long; please shorten to at most %d chars\n",
                 MAX_ROCKER_NAME_LEN);
-        return -EINVAL;
+        err = -EINVAL;
+        goto err_name_too_long;
     }
 
     if (memcmp(&r->fp_start_macaddr, &zero, sizeof(zero)) == 0) {
@@ -1430,6 +1431,7 @@ static int pci_rocker_init(PCIDevice *dev)
 
     return 0;
 
+err_name_too_long:
 err_duplicate:
     rocker_msix_uninit(r);
 err_msix_init:
-- 
2.9.3

[Qemu-devel] [PATCH v4 3/3] net/rocker: Convert to realize()

[Mao Zhongyi]

The rocker device still implements the old PCIDeviceClass .init()
instead of the new .realize(). All devices need to be converted to
.realize().

.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. After
the patch, effect like this:

    $ x86_64-softmmu/qemu-system-x86_64 -device rocker,name=qemu-rocker
    qemu-system-x86_64: -device rocker,name=qemu-rocker: name too long; please shorten to at most 9 chars

Cc: jasowang@redhat.com
Cc: jiri@resnulli.us
Cc: f4bug@amsat.org
Cc: armbru@redhat.com
Signed-off-by: Mao Zhongyi <maozy.fnst@cn.fujitsu.com>
---
 hw/net/rocker/rocker.c | 27 +++++++++++----------------
 1 file changed, 11 insertions(+), 16 deletions(-)

diff --git a/hw/net/rocker/rocker.c b/hw/net/rocker/rocker.c
index a382a6f..b9a77f1 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 } };
@@ -1319,10 +1317,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,
+                "invalid argument requested world %s does not exist",
                 r->world_name);
-        err = -EINVAL;
         goto err_world_type_by_name;
     }
 
@@ -1342,7 +1339,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;
     }
@@ -1354,7 +1351,7 @@ static int pci_rocker_init(PCIDevice *dev)
     }
 
     if (rocker_find(r->name)) {
-        err = -EEXIST;
+        error_setg(errp, "%s already exists", r->name);
         goto err_duplicate;
     }
 
@@ -1368,10 +1365,9 @@ 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,
+                "name too long; please shorten to at most %d chars",
                 MAX_ROCKER_NAME_LEN);
-        err = -EINVAL;
         goto err_name_too_long;
     }
 
@@ -1429,7 +1425,7 @@ static int pci_rocker_init(PCIDevice *dev)
 
     QLIST_INSERT_HEAD(&rockers, r, next);
 
-    return 0;
+    return;
 
 err_name_too_long:
 err_duplicate:
@@ -1443,7 +1439,6 @@ err_world_type_by_name:
             world_free(r->worlds[i]);
         }
     }
-    return err;
 }
 
 static void pci_rocker_uninit(PCIDevice *dev)
@@ -1528,7 +1523,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;
-- 
2.9.3

Re: [Qemu-devel] [PATCH v4 2/3] net/rocker: Plug memory leak in pci_rocker_init()

[Philippe Mathieu-Daudé]

Hi Mao,

On 05/17/2017 08:12 AM, Mao Zhongyi wrote:
> pci_rocker_init() leaks a World when the name more than 9 chars,
> then return a negative value directly, doesn't make a correct
> cleanup. So add a new goto label to fix it.
>
> Cc: jasowang@redhat.com
> Cc: jiri@resnulli.us
> Cc: f4bug@amsat.org
> Cc: armbru@redhat.com
> Signed-off-by: Mao Zhongyi <maozy.fnst@cn.fujitsu.com>
> ---
>  hw/net/rocker/rocker.c | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
>
> diff --git a/hw/net/rocker/rocker.c b/hw/net/rocker/rocker.c
> index b2b6dc7..a382a6f 100644
> --- a/hw/net/rocker/rocker.c
> +++ b/hw/net/rocker/rocker.c
> @@ -1371,7 +1371,8 @@ static int pci_rocker_init(PCIDevice *dev)
>          fprintf(stderr,

While here can you update fprintf -> qemu_log_mask?

>                  "rocker: name too long; please shorten to at most %d chars\n",
>                  MAX_ROCKER_NAME_LEN);
> -        return -EINVAL;
> +        err = -EINVAL;
> +        goto err_name_too_long;
>      }
>
>      if (memcmp(&r->fp_start_macaddr, &zero, sizeof(zero)) == 0) {
> @@ -1430,6 +1431,7 @@ static int pci_rocker_init(PCIDevice *dev)
>
>      return 0;
>
> +err_name_too_long:
>  err_duplicate:
>      rocker_msix_uninit(r);
>  err_msix_init:
>

Re: [Qemu-devel] [PATCH v4 2/3] net/rocker: Plug memory leak in pci_rocker_init()

[Mao Zhongyi]

Hi, Philippe
Thanks for your quick review:)

On 05/18/2017 01:22 AM, Philippe Mathieu-Daudé wrote:
> Hi Mao,
>
> On 05/17/2017 08:12 AM, Mao Zhongyi wrote:
>> pci_rocker_init() leaks a World when the name more than 9 chars,
>> then return a negative value directly, doesn't make a correct
>> cleanup. So add a new goto label to fix it.
>>
>> Cc: jasowang@redhat.com
>> Cc: jiri@resnulli.us
>> Cc: f4bug@amsat.org
>> Cc: armbru@redhat.com
>> Signed-off-by: Mao Zhongyi <maozy.fnst@cn.fujitsu.com>
>> ---
>>  hw/net/rocker/rocker.c | 4 +++-
>>  1 file changed, 3 insertions(+), 1 deletion(-)
>>
>> diff --git a/hw/net/rocker/rocker.c b/hw/net/rocker/rocker.c
>> index b2b6dc7..a382a6f 100644
>> --- a/hw/net/rocker/rocker.c
>> +++ b/hw/net/rocker/rocker.c
>> @@ -1371,7 +1371,8 @@ static int pci_rocker_init(PCIDevice *dev)
>>          fprintf(stderr,
>
> While here can you update fprintf -> qemu_log_mask?

In the patch 3, fprintf() has been updated to err_setg() which output error message
for more user-friendly. I'm not very clear why convert it to qemu_log_mask(). Could
you give me more hints about this?

Meanwhile, updating to qemu_log_mask() results in a segmentation fault. Because the
World memory was freed for 2 times. The first time is when the name more than 9 chars,
It was freed by the goto label "err_name_too_long" in the pci_rocker_realize(), the
second is freed by qemu_system_reset() which called in the main().

So I think convert it to Error maybe a little better. What do you think?

Thanks
Mao

>
>>                  "rocker: name too long; please shorten to at most %d chars\n",
>>                  MAX_ROCKER_NAME_LEN);
>> -        return -EINVAL;
>> +        err = -EINVAL;
>> +        goto err_name_too_long;
>>      }
>>
>>      if (memcmp(&r->fp_start_macaddr, &zero, sizeof(zero)) == 0) {
>> @@ -1430,6 +1431,7 @@ static int pci_rocker_init(PCIDevice *dev)
>>
>>      return 0;
>>
>> +err_name_too_long:
>>  err_duplicate:
>>      rocker_msix_uninit(r);
>>  err_msix_init:
>>
>
>
>

Re: [Qemu-devel] [PATCH v4 1/3] net/rocker: Remove the dead error handling

[Markus Armbruster]

Mao Zhongyi <maozy.fnst@cn.fujitsu.com> writes:

> The function of_dpa_world_alloc is a wrapper around world_alloc(), which
> returns null only when g_malloc0(size_t size) does. But g_malloc0() also
> is a wrapper around g_malloc0_n(1, size) that ignore the fact that
> g_malloc0() of 0 bytes, it doesn't returns null. So the error handling is
> dead. Similar like desc_ring_alloc(), fp_port_alloc() etc. Remove these
> entirely.
>
> Cc: jasowang@redhat.com
> Cc: jiri@resnulli.us
> Cc: f4bug@amsat.org
> Cc: armbru@redhat.com
> Signed-off-by: Mao Zhongyi <maozy.fnst@cn.fujitsu.com>
> ---
>  hw/net/rocker/rocker.c      | 32 --------------------------------
>  hw/net/rocker/rocker_desc.c |  3 ---
>  hw/net/rocker/rocker_fp.c   |  4 ----
>  3 files changed, 39 deletions(-)
>
> diff --git a/hw/net/rocker/rocker.c b/hw/net/rocker/rocker.c
> index 6e70fdd..b2b6dc7 100644
> --- a/hw/net/rocker/rocker.c
> +++ b/hw/net/rocker/rocker.c
> @@ -1313,13 +1313,6 @@ static int pci_rocker_init(PCIDevice *dev)
>  
>      r->worlds[ROCKER_WORLD_TYPE_OF_DPA] = of_dpa_world_alloc(r);
>  
> -    for (i = 0; i < ROCKER_WORLD_TYPE_MAX; i++) {
> -        if (!r->worlds[i]) {
> -            err = -ENOMEM;
> -            goto err_world_alloc;
> -        }
> -    }
> -
>      if (!r->world_name) {
>          r->world_name = g_strdup(world_name(r->worlds[ROCKER_WORLD_TYPE_OF_DPA]));
>      }

Please also simplify world_alloc()

   World *world_alloc(Rocker *r, size_t sizeof_private,
                      enum rocker_world_type type, WorldOps *ops)
   {
       World *w = g_malloc0(sizeof(World) + sizeof_private);

  -    if (w) {
           w->r = r;
           w->type = type;
           w->ops = ops;
           if (w->ops->init) {
               w->ops->init(w);
           }
       }

  -    return w;
   }

and reindent.

> @@ -1396,9 +1389,6 @@ static int pci_rocker_init(PCIDevice *dev)
>      }
>  
>      r->rings = g_new(DescRing *, rocker_pci_ring_count(r));
> -    if (!r->rings) {
> -        goto err_rings_alloc;
> -    }
>  
>      /* Rings are ordered like this:
>       * - command ring
> @@ -1410,14 +1400,9 @@ 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) {
> -            goto err_ring_alloc;
> -        }
> -
>          if (i == ROCKER_RING_CMD) {
>              desc_ring_set_consume(ring, cmd_consume, ROCKER_MSIX_VEC_CMD);
>          } else if (i == ROCKER_RING_EVENT) {
> @@ -1437,10 +1422,6 @@ static int pci_rocker_init(PCIDevice *dev)
>              fp_port_alloc(r, r->name, &r->fp_start_macaddr,
>                            i, &r->fp_ports_peers[i]);
>  
> -        if (!port) {
> -            goto err_port_alloc;
> -        }
> -
>          r->fp_port[i] = port;
>          fp_port_set_world(port, r->world_dflt);
>      }
> @@ -1449,25 +1430,12 @@ static int pci_rocker_init(PCIDevice *dev)
>  
>      return 0;
>  
> -err_port_alloc:
> -    for (--i; i >= 0; i--) {
> -        FpPort *port = r->fp_port[i];
> -        fp_port_free(port);
> -    }
> -    i = rocker_pci_ring_count(r);
> -err_ring_alloc:
> -    for (--i; i >= 0; i--) {
> -        desc_ring_free(r->rings[i]);
> -    }
> -    g_free(r->rings);
> -err_rings_alloc:
>  err_duplicate:
>      rocker_msix_uninit(r);
>  err_msix_init:
>      object_unparent(OBJECT(&r->msix_bar));
>      object_unparent(OBJECT(&r->mmio));
>  err_world_type_by_name:
> -err_world_alloc:
>      for (i = 0; i < ROCKER_WORLD_TYPE_MAX; i++) {
>          if (r->worlds[i]) {
>              world_free(r->worlds[i]);
> diff --git a/hw/net/rocker/rocker_desc.c b/hw/net/rocker/rocker_desc.c
> index ac02797..bd7e364 100644
> --- a/hw/net/rocker/rocker_desc.c
> +++ b/hw/net/rocker/rocker_desc.c
> @@ -347,9 +347,6 @@ DescRing *desc_ring_alloc(Rocker *r, int index)
>      DescRing *ring;
>  
>      ring = g_new0(DescRing, 1);
> -    if (!ring) {
> -        return NULL;
> -    }
>  
>      ring->r = r;
>      ring->index = index;
> diff --git a/hw/net/rocker/rocker_fp.c b/hw/net/rocker/rocker_fp.c
> index 1305ac3..4b3c984 100644
> --- a/hw/net/rocker/rocker_fp.c
> +++ b/hw/net/rocker/rocker_fp.c
> @@ -226,10 +226,6 @@ FpPort *fp_port_alloc(Rocker *r, char *sw_name,
>  {
>      FpPort *port = g_new0(FpPort, 1);
>  
> -    if (!port) {
> -        return NULL;
> -    }
> -
>      port->r = r;
>      port->index = index;
>      port->pport = index + 1;

There's more!

In tx_consume():

           }
           iov[iovcnt].iov_len = frag_len;
           iov[iovcnt].iov_base = g_malloc(frag_len);
  -        if (!iov[iovcnt].iov_base) {
  -            err = -ROCKER_ENOMEM;
  -            goto err_no_mem;
  -        }

           if (pci_dma_read(dev, frag_addr, iov[iovcnt].iov_base,

In rx_produce(): 

       data = g_malloc(data_size);
  -    if (!data) {
  -        err = -ROCKER_ENOMEM;
  -        goto out;
  -    }
       iov_to_buf(iov, iovcnt, 0, data, data_size);
       pci_dma_write(dev, frag_addr, data, data_size);

In rocker_test_dma_ctrl():

       int i;

       buf = g_malloc(r->test_dma_size);
   -
   -   if (!buf) {
   -       DPRINTF("test dma buffer alloc failed");
   -       return;
   -   }

       switch (val) {

This is just the result of a quick grep for '\<g_' immediately followed
by dead error handling in rocker.c.  Please do the same for the rest of
the rocker sources, and additionally search for dead error handling
separated from the allocation by function returns, like the one you
fixed in pci_rocker_init().

Re: [Qemu-devel] [PATCH v4 2/3] net/rocker: Plug memory leak in pci_rocker_init()

[Markus Armbruster]

Mao Zhongyi <maozy.fnst@cn.fujitsu.com> writes:

> pci_rocker_init() leaks a World when the name more than 9 chars,
> then return a negative value directly, doesn't make a correct
> cleanup. So add a new goto label to fix it.
>
> Cc: jasowang@redhat.com
> Cc: jiri@resnulli.us
> Cc: f4bug@amsat.org
> Cc: armbru@redhat.com
> Signed-off-by: Mao Zhongyi <maozy.fnst@cn.fujitsu.com>
> ---
>  hw/net/rocker/rocker.c | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
>
> diff --git a/hw/net/rocker/rocker.c b/hw/net/rocker/rocker.c
> index b2b6dc7..a382a6f 100644
> --- a/hw/net/rocker/rocker.c
> +++ b/hw/net/rocker/rocker.c
> @@ -1371,7 +1371,8 @@ static int pci_rocker_init(PCIDevice *dev)
>          fprintf(stderr,
>                  "rocker: name too long; please shorten to at most %d chars\n",
>                  MAX_ROCKER_NAME_LEN);
> -        return -EINVAL;
> +        err = -EINVAL;
> +        goto err_name_too_long;
>      }
>  
>      if (memcmp(&r->fp_start_macaddr, &zero, sizeof(zero)) == 0) {
> @@ -1430,6 +1431,7 @@ static int pci_rocker_init(PCIDevice *dev)
>  
>      return 0;
>  
> +err_name_too_long:
>  err_duplicate:
>      rocker_msix_uninit(r);
>  err_msix_init:

Reviewed-by: Markus Armbruster <armbru@redhat.com>

Re: [Qemu-devel] [PATCH v4 3/3] net/rocker: Convert to realize()

[Markus Armbruster]

Mao Zhongyi <maozy.fnst@cn.fujitsu.com> writes:

> The rocker device still implements the old PCIDeviceClass .init()
> instead of the new .realize(). All devices need to be converted to
> .realize().
>
> .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. After
> the patch, effect like this:
>
>     $ x86_64-softmmu/qemu-system-x86_64 -device rocker,name=qemu-rocker
>     qemu-system-x86_64: -device rocker,name=qemu-rocker: name too long; please shorten to at most 9 chars
>
> Cc: jasowang@redhat.com
> Cc: jiri@resnulli.us
> Cc: f4bug@amsat.org
> Cc: armbru@redhat.com
> Signed-off-by: Mao Zhongyi <maozy.fnst@cn.fujitsu.com>

The conversion to realize() looks good to me, therefore
Reviewed-by: Markus Armbruster <armbru@redhat.com>

However, I spotted a few issues not related to this patch.

1. Unusual macro names

    #define ROCKER "rocker"

    #define to_rocker(obj) \
        OBJECT_CHECK(Rocker, (obj), ROCKER)

   Please clean up to

    #define TYPE_ROCKER "rocker"

    #define ROCKER(obj) \
        OBJECT_CHECK(Rocker, (obj), TYPE_ROCKER)

   in a separate patch.

2. Memory leaks on error and unplug

   Explained inline below.

> ---
>  hw/net/rocker/rocker.c | 27 +++++++++++----------------
>  1 file changed, 11 insertions(+), 16 deletions(-)
>
> diff --git a/hw/net/rocker/rocker.c b/hw/net/rocker/rocker.c
> index a382a6f..b9a77f1 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 } };
> @@ -1319,10 +1317,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,
> +                "invalid argument requested world %s does not exist",
>                  r->world_name);
> -        err = -EINVAL;
>          goto err_world_type_by_name;
>      }
>  
> @@ -1342,7 +1339,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;
>      }
> @@ -1354,7 +1351,7 @@ static int pci_rocker_init(PCIDevice *dev)
>      }
>  
>      if (rocker_find(r->name)) {
> -        err = -EEXIST;
> +        error_setg(errp, "%s already exists", r->name);
>          goto err_duplicate;
>      }
>  
> @@ -1368,10 +1365,9 @@ 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,
> +                "name too long; please shorten to at most %d chars",
>                  MAX_ROCKER_NAME_LEN);
> -        err = -EINVAL;
>          goto err_name_too_long;
>      }
>  
> @@ -1429,7 +1425,7 @@ static int pci_rocker_init(PCIDevice *dev)
>  
>      QLIST_INSERT_HEAD(&rockers, r, next);
>  
> -    return 0;
> +    return;
>  
>  err_name_too_long:
>  err_duplicate:
       rocker_msix_uninit(r);
   err_msix_init:
       object_unparent(OBJECT(&r->msix_bar));
       object_unparent(OBJECT(&r->mmio));
   err_world_type_by_name:
       for (i = 0; i < ROCKER_WORLD_TYPE_MAX; i++) {
           if (r->worlds[i]) {
> @@ -1443,7 +1439,6 @@ err_world_type_by_name:
>              world_free(r->worlds[i]);
>          }
>      }
> -    return err;
>  }
>  

Does this leak r->world_name and r->name?

If yes, can you delay their allocation until after the last thing that
can fail?  That way, you don't have to free them on failure.  Simplifies
the error paths.  Keeping them as simple as practical is desireable,
because they're hard to test.

Simlarly, if you can delay allocation of r->worlds[], you can remove its
cleanup here.  Same for the other things you clean up here; I trust you
get the idea.

If substantial cleanup work remains, you could try to make
pci_rocker_uninit() usable here: ensure each of its steps does nothing
when the corresponding step in pci_rocker_realize() hasn't been
executed.  For a simple g_free() that's automatic, because g_free(P)
does nothing when P is null.  More complicated steps you might need to
wrap in a suitable conditional.

>  static void pci_rocker_uninit(PCIDevice *dev)
   {
       Rocker *r = to_rocker(dev);
       int i;

       QLIST_REMOVE(r, next);

       for (i = 0; i < r->fp_ports; i++) {
           FpPort *port = r->fp_port[i];

           fp_port_free(port);
           r->fp_port[i] = NULL;
       }

       for (i = 0; i < rocker_pci_ring_count(r); i++) {
           if (r->rings[i]) {
               desc_ring_free(r->rings[i]);
           }
       }
       g_free(r->rings);

       rocker_msix_uninit(r);
       object_unparent(OBJECT(&r->msix_bar));
       object_unparent(OBJECT(&r->mmio));

       for (i = 0; i < ROCKER_WORLD_TYPE_MAX; i++) {
           if (r->worlds[i]) {
               world_free(r->worlds[i]);
           }
       }
       g_free(r->fp_ports_peers);
   }

Does this leak r->world_name and r->name?

Suggest to run a hot unplug under valgrind to check for leaks.  Best run
it before fixing the leaks you can see to make sure you're using it
correctly.

Fixes could be squashed into PATCH 2.  If you don't want to do that, a
separate patch would also be okay.

> @@ -1528,7 +1523,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;

Re: [Qemu-devel] [PATCH v4 1/3] net/rocker: Remove the dead error handling

[Mao Zhongyi]

Hi, Markus
Thanks for review and sorry for replying late, I was on the weekend.

On 05/19/2017 02:24 PM, Markus Armbruster wrote:
> Mao Zhongyi <maozy.fnst@cn.fujitsu.com> writes:
>
>> The function of_dpa_world_alloc is a wrapper around world_alloc(), which
>> returns null only when g_malloc0(size_t size) does. But g_malloc0() also
>> is a wrapper around g_malloc0_n(1, size) that ignore the fact that
>> g_malloc0() of 0 bytes, it doesn't returns null. So the error handling is
>> dead. Similar like desc_ring_alloc(), fp_port_alloc() etc. Remove these
>> entirely.
>>
>> Cc: jasowang@redhat.com
>> Cc: jiri@resnulli.us
>> Cc: f4bug@amsat.org
>> Cc: armbru@redhat.com
>> Signed-off-by: Mao Zhongyi <maozy.fnst@cn.fujitsu.com>
>> ---
>>  hw/net/rocker/rocker.c      | 32 --------------------------------
>>  hw/net/rocker/rocker_desc.c |  3 ---
>>  hw/net/rocker/rocker_fp.c   |  4 ----
>>  3 files changed, 39 deletions(-)
>>
>> diff --git a/hw/net/rocker/rocker.c b/hw/net/rocker/rocker.c
>> index 6e70fdd..b2b6dc7 100644
>> --- a/hw/net/rocker/rocker.c
>> +++ b/hw/net/rocker/rocker.c
>> @@ -1313,13 +1313,6 @@ static int pci_rocker_init(PCIDevice *dev)
>>
>>      r->worlds[ROCKER_WORLD_TYPE_OF_DPA] = of_dpa_world_alloc(r);
>>
>> -    for (i = 0; i < ROCKER_WORLD_TYPE_MAX; i++) {
>> -        if (!r->worlds[i]) {
>> -            err = -ENOMEM;
>> -            goto err_world_alloc;
>> -        }
>> -    }
>> -
>>      if (!r->world_name) {
>>          r->world_name = g_strdup(world_name(r->worlds[ROCKER_WORLD_TYPE_OF_DPA]));
>>      }
>
> Please also simplify world_alloc()
>
>    World *world_alloc(Rocker *r, size_t sizeof_private,
>                       enum rocker_world_type type, WorldOps *ops)
>    {
>        World *w = g_malloc0(sizeof(World) + sizeof_private);
>
>   -    if (w) {
>            w->r = r;
>            w->type = type;
>            w->ops = ops;
>            if (w->ops->init) {
>                w->ops->init(w);
>            }
>        }
>
>   -    return w;
>    }

OK, I will fix it and search for the similar error handling from others allocation
functions.

>
> and reindent.
>
>> @@ -1396,9 +1389,6 @@ static int pci_rocker_init(PCIDevice *dev)
>>      }
>>
>>      r->rings = g_new(DescRing *, rocker_pci_ring_count(r));
>> -    if (!r->rings) {
>> -        goto err_rings_alloc;
>> -    }
>>
>>      /* Rings are ordered like this:
>>       * - command ring
>> @@ -1410,14 +1400,9 @@ 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) {
>> -            goto err_ring_alloc;
>> -        }
>> -
>>          if (i == ROCKER_RING_CMD) {
>>              desc_ring_set_consume(ring, cmd_consume, ROCKER_MSIX_VEC_CMD);
>>          } else if (i == ROCKER_RING_EVENT) {
>> @@ -1437,10 +1422,6 @@ static int pci_rocker_init(PCIDevice *dev)
>>              fp_port_alloc(r, r->name, &r->fp_start_macaddr,
>>                            i, &r->fp_ports_peers[i]);
>>
>> -        if (!port) {
>> -            goto err_port_alloc;
>> -        }
>> -
>>          r->fp_port[i] = port;
>>          fp_port_set_world(port, r->world_dflt);
>>      }
>> @@ -1449,25 +1430,12 @@ static int pci_rocker_init(PCIDevice *dev)
>>
>>      return 0;
>>
>> -err_port_alloc:
>> -    for (--i; i >= 0; i--) {
>> -        FpPort *port = r->fp_port[i];
>> -        fp_port_free(port);
>> -    }
>> -    i = rocker_pci_ring_count(r);
>> -err_ring_alloc:
>> -    for (--i; i >= 0; i--) {
>> -        desc_ring_free(r->rings[i]);
>> -    }
>> -    g_free(r->rings);
>> -err_rings_alloc:
>>  err_duplicate:
>>      rocker_msix_uninit(r);
>>  err_msix_init:
>>      object_unparent(OBJECT(&r->msix_bar));
>>      object_unparent(OBJECT(&r->mmio));
>>  err_world_type_by_name:
>> -err_world_alloc:
>>      for (i = 0; i < ROCKER_WORLD_TYPE_MAX; i++) {
>>          if (r->worlds[i]) {
>>              world_free(r->worlds[i]);
>> diff --git a/hw/net/rocker/rocker_desc.c b/hw/net/rocker/rocker_desc.c
>> index ac02797..bd7e364 100644
>> --- a/hw/net/rocker/rocker_desc.c
>> +++ b/hw/net/rocker/rocker_desc.c
>> @@ -347,9 +347,6 @@ DescRing *desc_ring_alloc(Rocker *r, int index)
>>      DescRing *ring;
>>
>>      ring = g_new0(DescRing, 1);
>> -    if (!ring) {
>> -        return NULL;
>> -    }
>>
>>      ring->r = r;
>>      ring->index = index;
>> diff --git a/hw/net/rocker/rocker_fp.c b/hw/net/rocker/rocker_fp.c
>> index 1305ac3..4b3c984 100644
>> --- a/hw/net/rocker/rocker_fp.c
>> +++ b/hw/net/rocker/rocker_fp.c
>> @@ -226,10 +226,6 @@ FpPort *fp_port_alloc(Rocker *r, char *sw_name,
>>  {
>>      FpPort *port = g_new0(FpPort, 1);
>>
>> -    if (!port) {
>> -        return NULL;
>> -    }
>> -
>>      port->r = r;
>>      port->index = index;
>>      port->pport = index + 1;
>
> There's more!
>
> In tx_consume():
>
>            }
>            iov[iovcnt].iov_len = frag_len;
>            iov[iovcnt].iov_base = g_malloc(frag_len);
>   -        if (!iov[iovcnt].iov_base) {
>   -            err = -ROCKER_ENOMEM;
>   -            goto err_no_mem;
>   -        }
>
>            if (pci_dma_read(dev, frag_addr, iov[iovcnt].iov_base,
>
> In rx_produce():
>
>        data = g_malloc(data_size);
>   -    if (!data) {
>   -        err = -ROCKER_ENOMEM;
>   -        goto out;
>   -    }
>        iov_to_buf(iov, iovcnt, 0, data, data_size);
>        pci_dma_write(dev, frag_addr, data, data_size);
>
> In rocker_test_dma_ctrl():
>
>        int i;
>
>        buf = g_malloc(r->test_dma_size);
>    -
>    -   if (!buf) {
>    -       DPRINTF("test dma buffer alloc failed");
>    -       return;
>    -   }
>
>        switch (val) {
>
> This is just the result of a quick grep for '\<g_' immediately followed
> by dead error handling in rocker.c.  Please do the same for the rest of
> the rocker sources, and additionally search for dead error handling
> separated from the allocation by function returns, like the one you
> fixed in pci_rocker_init().

After reading your detailed explanation, I think I will try to do these:
1. Searching for all the dead error handling in the rocker_*.c.
2. All dead error handling from allocation functions will be as a separated
    patch. Similarly, from function returns as another patch. Is that right?

Thanks
Mao

Re: [Qemu-devel] [PATCH v4 3/3] net/rocker: Convert to realize()

[Mao Zhongyi]

Hi, Markus

On 05/19/2017 03:20 PM, Markus Armbruster wrote:
> Mao Zhongyi <maozy.fnst@cn.fujitsu.com> writes:
>
>> The rocker device still implements the old PCIDeviceClass .init()
>> instead of the new .realize(). All devices need to be converted to
>> .realize().
>>
>> .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. After
>> the patch, effect like this:
>>
>>     $ x86_64-softmmu/qemu-system-x86_64 -device rocker,name=qemu-rocker
>>     qemu-system-x86_64: -device rocker,name=qemu-rocker: name too long; please shorten to at most 9 chars
>>
>> Cc: jasowang@redhat.com
>> Cc: jiri@resnulli.us
>> Cc: f4bug@amsat.org
>> Cc: armbru@redhat.com
>> Signed-off-by: Mao Zhongyi <maozy.fnst@cn.fujitsu.com>
>
> The conversion to realize() looks good to me, therefore
> Reviewed-by: Markus Armbruster <armbru@redhat.com>
>
> However, I spotted a few issues not related to this patch.
>
> 1. Unusual macro names
>
>     #define ROCKER "rocker"
>
>     #define to_rocker(obj) \
>         OBJECT_CHECK(Rocker, (obj), ROCKER)
>
>    Please clean up to
>
>     #define TYPE_ROCKER "rocker"
>
>     #define ROCKER(obj) \
>         OBJECT_CHECK(Rocker, (obj), TYPE_ROCKER)
>
>    in a separate patch.

Thanks, will fix it in the next version.

>
> 2. Memory leaks on error and unplug
>
>    Explained inline below.
>
>> ---
>>  hw/net/rocker/rocker.c | 27 +++++++++++----------------
>>  1 file changed, 11 insertions(+), 16 deletions(-)
>>
>> diff --git a/hw/net/rocker/rocker.c b/hw/net/rocker/rocker.c
>> index a382a6f..b9a77f1 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 } };
>> @@ -1319,10 +1317,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,
>> +                "invalid argument requested world %s does not exist",
>>                  r->world_name);
>> -        err = -EINVAL;
>>          goto err_world_type_by_name;
>>      }
>>
>> @@ -1342,7 +1339,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;
>>      }
>> @@ -1354,7 +1351,7 @@ static int pci_rocker_init(PCIDevice *dev)
>>      }
>>
>>      if (rocker_find(r->name)) {
>> -        err = -EEXIST;
>> +        error_setg(errp, "%s already exists", r->name);
>>          goto err_duplicate;
>>      }
>>
>> @@ -1368,10 +1365,9 @@ 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,
>> +                "name too long; please shorten to at most %d chars",
>>                  MAX_ROCKER_NAME_LEN);
>> -        err = -EINVAL;
>>          goto err_name_too_long;
>>      }
>>
>> @@ -1429,7 +1425,7 @@ static int pci_rocker_init(PCIDevice *dev)
>>
>>      QLIST_INSERT_HEAD(&rockers, r, next);
>>
>> -    return 0;
>> +    return;
>>
>>  err_name_too_long:
>>  err_duplicate:
>        rocker_msix_uninit(r);
>    err_msix_init:
>        object_unparent(OBJECT(&r->msix_bar));
>        object_unparent(OBJECT(&r->mmio));
>    err_world_type_by_name:
>        for (i = 0; i < ROCKER_WORLD_TYPE_MAX; i++) {
>            if (r->worlds[i]) {
>> @@ -1443,7 +1439,6 @@ err_world_type_by_name:
>>              world_free(r->worlds[i]);
>>          }
>>      }
>> -    return err;
>>  }
>>
>
> Does this leak r->world_name and r->name?

I think it was leaked neither r->world_name nor r->name, but msix and
worlds related.

>
> If yes, can you delay their allocation until after the last thing that
> can fail?  That way, you don't have to free them on failure.  Simplifies
> the error paths.  Keeping them as simple as practical is desireable,
> because they're hard to test.

If delay allocation of r->worlds[] until after the last, when r->name and
r->world_name failed, passing the error message to errp is a good idea. I
think that's what 'error paths' means, then it is really not need to free
the memory in the goto label. Because the r->worlds[] has not yet been
allocated. Is that right? If yes, it's really a perfect solution.

But, this ignores the fact that r->name and r->world_name are depend on
r->worlds, so r->worlds's allocation must be before them. in this case,
the previous solution will be lose its meaning.

>
> Simlarly, if you can delay allocation of r->worlds[], you can remove its
> cleanup here.  Same for the other things you clean up here; I trust you
> get the idea.
>
> If substantial cleanup work remains, you could try to make
> pci_rocker_uninit() usable here: ensure each of its steps does nothing
> when the corresponding step in pci_rocker_realize() hasn't been
> executed.  For a simple g_free() that's automatic, because g_free(P)
> does nothing when P is null.  More complicated steps you might need to
> wrap in a suitable conditional.
>
>>  static void pci_rocker_uninit(PCIDevice *dev)
>    {
>        Rocker *r = to_rocker(dev);
>        int i;
>
>        QLIST_REMOVE(r, next);
>
>        for (i = 0; i < r->fp_ports; i++) {
>            FpPort *port = r->fp_port[i];
>
>            fp_port_free(port);
>            r->fp_port[i] = NULL;
>        }
>
>        for (i = 0; i < rocker_pci_ring_count(r); i++) {
>            if (r->rings[i]) {
>                desc_ring_free(r->rings[i]);
>            }
>        }
>        g_free(r->rings);
>
>        rocker_msix_uninit(r);
>        object_unparent(OBJECT(&r->msix_bar));
>        object_unparent(OBJECT(&r->mmio));
>
>        for (i = 0; i < ROCKER_WORLD_TYPE_MAX; i++) {
>            if (r->worlds[i]) {
>                world_free(r->worlds[i]);
>            }
>        }
>        g_free(r->fp_ports_peers);
>    }
>
> Does this leak r->world_name and r->name?
>
> Suggest to run a hot unplug under valgrind to check for leaks.  Best run
> it before fixing the leaks you can see to make sure you're using it
> correctly.
>

In order to make sure the plug patch is completely correct, will before the
patch to use valgrind tools to check it.

Thanks very much for your kind suggestion.
Mao

> Fixes could be squashed into PATCH 2.  If you don't want to do that, a
> separate patch would also be okay.
>
>> @@ -1528,7 +1523,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;
>

Re: [Qemu-devel] [PATCH v4 1/3] net/rocker: Remove the dead error handling

[Markus Armbruster]

Mao Zhongyi <maozy.fnst@cn.fujitsu.com> writes:

> Hi, Markus
> Thanks for review and sorry for replying late, I was on the weekend.

No need to apologize for the weekend.  I hope you enjoyed it :)

> On 05/19/2017 02:24 PM, Markus Armbruster wrote:
>> Mao Zhongyi <maozy.fnst@cn.fujitsu.com> writes:
>>
>>> The function of_dpa_world_alloc is a wrapper around world_alloc(), which
>>> returns null only when g_malloc0(size_t size) does. But g_malloc0() also
>>> is a wrapper around g_malloc0_n(1, size) that ignore the fact that
>>> g_malloc0() of 0 bytes, it doesn't returns null. So the error handling is
>>> dead. Similar like desc_ring_alloc(), fp_port_alloc() etc. Remove these
>>> entirely.
>>>
>>> Cc: jasowang@redhat.com
>>> Cc: jiri@resnulli.us
>>> Cc: f4bug@amsat.org
>>> Cc: armbru@redhat.com
>>> Signed-off-by: Mao Zhongyi <maozy.fnst@cn.fujitsu.com>
>>> ---
>>>  hw/net/rocker/rocker.c      | 32 --------------------------------
>>>  hw/net/rocker/rocker_desc.c |  3 ---
>>>  hw/net/rocker/rocker_fp.c   |  4 ----
>>>  3 files changed, 39 deletions(-)
>>>
>>> diff --git a/hw/net/rocker/rocker.c b/hw/net/rocker/rocker.c
>>> index 6e70fdd..b2b6dc7 100644
>>> --- a/hw/net/rocker/rocker.c
>>> +++ b/hw/net/rocker/rocker.c
>>> @@ -1313,13 +1313,6 @@ static int pci_rocker_init(PCIDevice *dev)
>>>
>>>      r->worlds[ROCKER_WORLD_TYPE_OF_DPA] = of_dpa_world_alloc(r);
>>>
>>> -    for (i = 0; i < ROCKER_WORLD_TYPE_MAX; i++) {
>>> -        if (!r->worlds[i]) {
>>> -            err = -ENOMEM;
>>> -            goto err_world_alloc;
>>> -        }
>>> -    }
>>> -
>>>      if (!r->world_name) {
>>>          r->world_name = g_strdup(world_name(r->worlds[ROCKER_WORLD_TYPE_OF_DPA]));
>>>      }
>>
>> Please also simplify world_alloc()
>>
>>    World *world_alloc(Rocker *r, size_t sizeof_private,
>>                       enum rocker_world_type type, WorldOps *ops)
>>    {
>>        World *w = g_malloc0(sizeof(World) + sizeof_private);
>>
>>   -    if (w) {
>>            w->r = r;
>>            w->type = type;
>>            w->ops = ops;
>>            if (w->ops->init) {
>>                w->ops->init(w);
>>            }
>>        }
>>
>>   -    return w;
>>    }
>
> OK, I will fix it and search for the similar error handling from others allocation
> functions.
>
>>
>> and reindent.
>>
>>> @@ -1396,9 +1389,6 @@ static int pci_rocker_init(PCIDevice *dev)
>>>      }
>>>
>>>      r->rings = g_new(DescRing *, rocker_pci_ring_count(r));
>>> -    if (!r->rings) {
>>> -        goto err_rings_alloc;
>>> -    }
>>>
>>>      /* Rings are ordered like this:
>>>       * - command ring
>>> @@ -1410,14 +1400,9 @@ 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) {
>>> -            goto err_ring_alloc;
>>> -        }
>>> -
>>>          if (i == ROCKER_RING_CMD) {
>>>              desc_ring_set_consume(ring, cmd_consume, ROCKER_MSIX_VEC_CMD);
>>>          } else if (i == ROCKER_RING_EVENT) {
>>> @@ -1437,10 +1422,6 @@ static int pci_rocker_init(PCIDevice *dev)
>>>              fp_port_alloc(r, r->name, &r->fp_start_macaddr,
>>>                            i, &r->fp_ports_peers[i]);
>>>
>>> -        if (!port) {
>>> -            goto err_port_alloc;
>>> -        }
>>> -
>>>          r->fp_port[i] = port;
>>>          fp_port_set_world(port, r->world_dflt);
>>>      }
>>> @@ -1449,25 +1430,12 @@ static int pci_rocker_init(PCIDevice *dev)
>>>
>>>      return 0;
>>>
>>> -err_port_alloc:
>>> -    for (--i; i >= 0; i--) {
>>> -        FpPort *port = r->fp_port[i];
>>> -        fp_port_free(port);
>>> -    }
>>> -    i = rocker_pci_ring_count(r);
>>> -err_ring_alloc:
>>> -    for (--i; i >= 0; i--) {
>>> -        desc_ring_free(r->rings[i]);
>>> -    }
>>> -    g_free(r->rings);
>>> -err_rings_alloc:
>>>  err_duplicate:
>>>      rocker_msix_uninit(r);
>>>  err_msix_init:
>>>      object_unparent(OBJECT(&r->msix_bar));
>>>      object_unparent(OBJECT(&r->mmio));
>>>  err_world_type_by_name:
>>> -err_world_alloc:
>>>      for (i = 0; i < ROCKER_WORLD_TYPE_MAX; i++) {
>>>          if (r->worlds[i]) {
>>>              world_free(r->worlds[i]);
>>> diff --git a/hw/net/rocker/rocker_desc.c b/hw/net/rocker/rocker_desc.c
>>> index ac02797..bd7e364 100644
>>> --- a/hw/net/rocker/rocker_desc.c
>>> +++ b/hw/net/rocker/rocker_desc.c
>>> @@ -347,9 +347,6 @@ DescRing *desc_ring_alloc(Rocker *r, int index)
>>>      DescRing *ring;
>>>
>>>      ring = g_new0(DescRing, 1);
>>> -    if (!ring) {
>>> -        return NULL;
>>> -    }
>>>
>>>      ring->r = r;
>>>      ring->index = index;
>>> diff --git a/hw/net/rocker/rocker_fp.c b/hw/net/rocker/rocker_fp.c
>>> index 1305ac3..4b3c984 100644
>>> --- a/hw/net/rocker/rocker_fp.c
>>> +++ b/hw/net/rocker/rocker_fp.c
>>> @@ -226,10 +226,6 @@ FpPort *fp_port_alloc(Rocker *r, char *sw_name,
>>>  {
>>>      FpPort *port = g_new0(FpPort, 1);
>>>
>>> -    if (!port) {
>>> -        return NULL;
>>> -    }
>>> -
>>>      port->r = r;
>>>      port->index = index;
>>>      port->pport = index + 1;
>>
>> There's more!
>>
>> In tx_consume():
>>
>>            }
>>            iov[iovcnt].iov_len = frag_len;
>>            iov[iovcnt].iov_base = g_malloc(frag_len);
>>   -        if (!iov[iovcnt].iov_base) {
>>   -            err = -ROCKER_ENOMEM;
>>   -            goto err_no_mem;
>>   -        }
>>
>>            if (pci_dma_read(dev, frag_addr, iov[iovcnt].iov_base,
>>
>> In rx_produce():
>>
>>        data = g_malloc(data_size);
>>   -    if (!data) {
>>   -        err = -ROCKER_ENOMEM;
>>   -        goto out;
>>   -    }
>>        iov_to_buf(iov, iovcnt, 0, data, data_size);
>>        pci_dma_write(dev, frag_addr, data, data_size);
>>
>> In rocker_test_dma_ctrl():
>>
>>        int i;
>>
>>        buf = g_malloc(r->test_dma_size);
>>    -
>>    -   if (!buf) {
>>    -       DPRINTF("test dma buffer alloc failed");
>>    -       return;
>>    -   }
>>
>>        switch (val) {
>>
>> This is just the result of a quick grep for '\<g_' immediately followed
>> by dead error handling in rocker.c.  Please do the same for the rest of
>> the rocker sources, and additionally search for dead error handling
>> separated from the allocation by function returns, like the one you
>> fixed in pci_rocker_init().
>
> After reading your detailed explanation, I think I will try to do these:
> 1. Searching for all the dead error handling in the rocker_*.c.
> 2. All dead error handling from allocation functions will be as a separated
>    patch. Similarly, from function returns as another patch. Is that right?

A single patch to drop all dead error handling could be okay.  Hard to
say before you see it.  Merging patches is less work than splitting
them, so you might want to develop separate patches, then squash them
together to see whether the result is easy enough to describe and
understand.

General advice: when you notice that writing a commit message is hard,
stop and think whether it would be easier if the patch was split.

Re: [Qemu-devel] [PATCH v4 3/3] net/rocker: Convert to realize()

[Markus Armbruster]

Mao Zhongyi <maozy.fnst@cn.fujitsu.com> writes:

> Hi, Markus
>
> On 05/19/2017 03:20 PM, Markus Armbruster wrote:
>> Mao Zhongyi <maozy.fnst@cn.fujitsu.com> writes:
>>
>>> The rocker device still implements the old PCIDeviceClass .init()
>>> instead of the new .realize(). All devices need to be converted to
>>> .realize().
>>>
>>> .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. After
>>> the patch, effect like this:
>>>
>>>     $ x86_64-softmmu/qemu-system-x86_64 -device rocker,name=qemu-rocker
>>>     qemu-system-x86_64: -device rocker,name=qemu-rocker: name too long; please shorten to at most 9 chars
>>>
>>> Cc: jasowang@redhat.com
>>> Cc: jiri@resnulli.us
>>> Cc: f4bug@amsat.org
>>> Cc: armbru@redhat.com
>>> Signed-off-by: Mao Zhongyi <maozy.fnst@cn.fujitsu.com>
>>
>> The conversion to realize() looks good to me, therefore
>> Reviewed-by: Markus Armbruster <armbru@redhat.com>
>>
>> However, I spotted a few issues not related to this patch.
>>
>> 1. Unusual macro names
>>
>>     #define ROCKER "rocker"
>>
>>     #define to_rocker(obj) \
>>         OBJECT_CHECK(Rocker, (obj), ROCKER)
>>
>>    Please clean up to
>>
>>     #define TYPE_ROCKER "rocker"
>>
>>     #define ROCKER(obj) \
>>         OBJECT_CHECK(Rocker, (obj), TYPE_ROCKER)
>>
>>    in a separate patch.
>
> Thanks, will fix it in the next version.
>
>>
>> 2. Memory leaks on error and unplug
>>
>>    Explained inline below.
>>
>>> ---
>>>  hw/net/rocker/rocker.c | 27 +++++++++++----------------
>>>  1 file changed, 11 insertions(+), 16 deletions(-)
>>>
>>> diff --git a/hw/net/rocker/rocker.c b/hw/net/rocker/rocker.c
>>> index a382a6f..b9a77f1 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 } };
>>> @@ -1319,10 +1317,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,
>>> +                "invalid argument requested world %s does not exist",
>>>                  r->world_name);
>>> -        err = -EINVAL;
>>>          goto err_world_type_by_name;
>>>      }
>>>
>>> @@ -1342,7 +1339,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;
>>>      }
>>> @@ -1354,7 +1351,7 @@ static int pci_rocker_init(PCIDevice *dev)
>>>      }
>>>
>>>      if (rocker_find(r->name)) {
>>> -        err = -EEXIST;
>>> +        error_setg(errp, "%s already exists", r->name);
>>>          goto err_duplicate;
>>>      }
>>>
>>> @@ -1368,10 +1365,9 @@ 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,
>>> +                "name too long; please shorten to at most %d chars",
>>>                  MAX_ROCKER_NAME_LEN);
>>> -        err = -EINVAL;
>>>          goto err_name_too_long;
>>>      }
>>>
>>> @@ -1429,7 +1425,7 @@ static int pci_rocker_init(PCIDevice *dev)
>>>
>>>      QLIST_INSERT_HEAD(&rockers, r, next);
>>>
>>> -    return 0;
>>> +    return;
>>>
>>>  err_name_too_long:
>>>  err_duplicate:
>>        rocker_msix_uninit(r);
>>    err_msix_init:
>>        object_unparent(OBJECT(&r->msix_bar));
>>        object_unparent(OBJECT(&r->mmio));
>>    err_world_type_by_name:
>>        for (i = 0; i < ROCKER_WORLD_TYPE_MAX; i++) {
>>            if (r->worlds[i]) {
>>> @@ -1443,7 +1439,6 @@ err_world_type_by_name:
>>>              world_free(r->worlds[i]);
>>>          }
>>>      }
>>> -    return err;
>>>  }
>>>
>>
>> Does this leak r->world_name and r->name?
>
> I think it was leaked neither r->world_name nor r->name, but msix and
> worlds related.

You're right: the two are properties, so the property machinery should
free them.

>> If yes, can you delay their allocation until after the last thing that
>> can fail?  That way, you don't have to free them on failure.  Simplifies
>> the error paths.  Keeping them as simple as practical is desireable,
>> because they're hard to test.
>
> If delay allocation of r->worlds[] until after the last, when r->name and
> r->world_name failed, passing the error message to errp is a good idea. I
> think that's what 'error paths' means, then it is really not need to free
> the memory in the goto label. Because the r->worlds[] has not yet been
> allocated. Is that right? If yes, it's really a perfect solution.
>
> But, this ignores the fact that r->name and r->world_name are depend on
> r->worlds, so r->worlds's allocation must be before them. in this case,
> the previous solution will be lose its meaning.

Delaying allocation until after all error checks is not always
practical.  You decide.

[...]

Re: [Qemu-devel] [PATCH v4 1/3] net/rocker: Remove the dead error handling

[Mao Zhongyi]

On 05/22/2017 02:35 PM, Markus Armbruster wrote:
> Mao Zhongyi <maozy.fnst@cn.fujitsu.com> writes:
>
>> Hi, Markus
>> Thanks for review and sorry for replying late, I was on the weekend.
>
> No need to apologize for the weekend.  I hope you enjoyed it :)
>
>> On 05/19/2017 02:24 PM, Markus Armbruster wrote:
>>> Mao Zhongyi <maozy.fnst@cn.fujitsu.com> writes:
>>>
>>>> The function of_dpa_world_alloc is a wrapper around world_alloc(), which
>>>> returns null only when g_malloc0(size_t size) does. But g_malloc0() also
>>>> is a wrapper around g_malloc0_n(1, size) that ignore the fact that
>>>> g_malloc0() of 0 bytes, it doesn't returns null. So the error handling is
>>>> dead. Similar like desc_ring_alloc(), fp_port_alloc() etc. Remove these
>>>> entirely.
>>>>
>>>> Cc: jasowang@redhat.com
>>>> Cc: jiri@resnulli.us
>>>> Cc: f4bug@amsat.org
>>>> Cc: armbru@redhat.com
>>>> Signed-off-by: Mao Zhongyi <maozy.fnst@cn.fujitsu.com>
>>>> ---
>>>>  hw/net/rocker/rocker.c      | 32 --------------------------------
>>>>  hw/net/rocker/rocker_desc.c |  3 ---
>>>>  hw/net/rocker/rocker_fp.c   |  4 ----
>>>>  3 files changed, 39 deletions(-)
>>>>
>>>> diff --git a/hw/net/rocker/rocker.c b/hw/net/rocker/rocker.c
>>>> index 6e70fdd..b2b6dc7 100644
>>>> --- a/hw/net/rocker/rocker.c
>>>> +++ b/hw/net/rocker/rocker.c
>>>> @@ -1313,13 +1313,6 @@ static int pci_rocker_init(PCIDevice *dev)
>>>>
>>>>      r->worlds[ROCKER_WORLD_TYPE_OF_DPA] = of_dpa_world_alloc(r);
>>>>
>>>> -    for (i = 0; i < ROCKER_WORLD_TYPE_MAX; i++) {
>>>> -        if (!r->worlds[i]) {
>>>> -            err = -ENOMEM;
>>>> -            goto err_world_alloc;
>>>> -        }
>>>> -    }
>>>> -
>>>>      if (!r->world_name) {
>>>>          r->world_name = g_strdup(world_name(r->worlds[ROCKER_WORLD_TYPE_OF_DPA]));
>>>>      }
>>>
>>> Please also simplify world_alloc()
>>>
>>>    World *world_alloc(Rocker *r, size_t sizeof_private,
>>>                       enum rocker_world_type type, WorldOps *ops)
>>>    {
>>>        World *w = g_malloc0(sizeof(World) + sizeof_private);
>>>
>>>   -    if (w) {
>>>            w->r = r;
>>>            w->type = type;
>>>            w->ops = ops;
>>>            if (w->ops->init) {
>>>                w->ops->init(w);
>>>            }
>>>        }
>>>
>>>   -    return w;
>>>    }
>>
>> OK, I will fix it and search for the similar error handling from others allocation
>> functions.
>>
>>>
>>> and reindent.
>>>
>>>> @@ -1396,9 +1389,6 @@ static int pci_rocker_init(PCIDevice *dev)
>>>>      }
>>>>
>>>>      r->rings = g_new(DescRing *, rocker_pci_ring_count(r));
>>>> -    if (!r->rings) {
>>>> -        goto err_rings_alloc;
>>>> -    }
>>>>
>>>>      /* Rings are ordered like this:
>>>>       * - command ring
>>>> @@ -1410,14 +1400,9 @@ 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) {
>>>> -            goto err_ring_alloc;
>>>> -        }
>>>> -
>>>>          if (i == ROCKER_RING_CMD) {
>>>>              desc_ring_set_consume(ring, cmd_consume, ROCKER_MSIX_VEC_CMD);
>>>>          } else if (i == ROCKER_RING_EVENT) {
>>>> @@ -1437,10 +1422,6 @@ static int pci_rocker_init(PCIDevice *dev)
>>>>              fp_port_alloc(r, r->name, &r->fp_start_macaddr,
>>>>                            i, &r->fp_ports_peers[i]);
>>>>
>>>> -        if (!port) {
>>>> -            goto err_port_alloc;
>>>> -        }
>>>> -
>>>>          r->fp_port[i] = port;
>>>>          fp_port_set_world(port, r->world_dflt);
>>>>      }
>>>> @@ -1449,25 +1430,12 @@ static int pci_rocker_init(PCIDevice *dev)
>>>>
>>>>      return 0;
>>>>
>>>> -err_port_alloc:
>>>> -    for (--i; i >= 0; i--) {
>>>> -        FpPort *port = r->fp_port[i];
>>>> -        fp_port_free(port);
>>>> -    }
>>>> -    i = rocker_pci_ring_count(r);
>>>> -err_ring_alloc:
>>>> -    for (--i; i >= 0; i--) {
>>>> -        desc_ring_free(r->rings[i]);
>>>> -    }
>>>> -    g_free(r->rings);
>>>> -err_rings_alloc:
>>>>  err_duplicate:
>>>>      rocker_msix_uninit(r);
>>>>  err_msix_init:
>>>>      object_unparent(OBJECT(&r->msix_bar));
>>>>      object_unparent(OBJECT(&r->mmio));
>>>>  err_world_type_by_name:
>>>> -err_world_alloc:
>>>>      for (i = 0; i < ROCKER_WORLD_TYPE_MAX; i++) {
>>>>          if (r->worlds[i]) {
>>>>              world_free(r->worlds[i]);
>>>> diff --git a/hw/net/rocker/rocker_desc.c b/hw/net/rocker/rocker_desc.c
>>>> index ac02797..bd7e364 100644
>>>> --- a/hw/net/rocker/rocker_desc.c
>>>> +++ b/hw/net/rocker/rocker_desc.c
>>>> @@ -347,9 +347,6 @@ DescRing *desc_ring_alloc(Rocker *r, int index)
>>>>      DescRing *ring;
>>>>
>>>>      ring = g_new0(DescRing, 1);
>>>> -    if (!ring) {
>>>> -        return NULL;
>>>> -    }
>>>>
>>>>      ring->r = r;
>>>>      ring->index = index;
>>>> diff --git a/hw/net/rocker/rocker_fp.c b/hw/net/rocker/rocker_fp.c
>>>> index 1305ac3..4b3c984 100644
>>>> --- a/hw/net/rocker/rocker_fp.c
>>>> +++ b/hw/net/rocker/rocker_fp.c
>>>> @@ -226,10 +226,6 @@ FpPort *fp_port_alloc(Rocker *r, char *sw_name,
>>>>  {
>>>>      FpPort *port = g_new0(FpPort, 1);
>>>>
>>>> -    if (!port) {
>>>> -        return NULL;
>>>> -    }
>>>> -
>>>>      port->r = r;
>>>>      port->index = index;
>>>>      port->pport = index + 1;
>>>
>>> There's more!
>>>
>>> In tx_consume():
>>>
>>>            }
>>>            iov[iovcnt].iov_len = frag_len;
>>>            iov[iovcnt].iov_base = g_malloc(frag_len);
>>>   -        if (!iov[iovcnt].iov_base) {
>>>   -            err = -ROCKER_ENOMEM;
>>>   -            goto err_no_mem;
>>>   -        }
>>>
>>>            if (pci_dma_read(dev, frag_addr, iov[iovcnt].iov_base,
>>>
>>> In rx_produce():
>>>
>>>        data = g_malloc(data_size);
>>>   -    if (!data) {
>>>   -        err = -ROCKER_ENOMEM;
>>>   -        goto out;
>>>   -    }
>>>        iov_to_buf(iov, iovcnt, 0, data, data_size);
>>>        pci_dma_write(dev, frag_addr, data, data_size);
>>>
>>> In rocker_test_dma_ctrl():
>>>
>>>        int i;
>>>
>>>        buf = g_malloc(r->test_dma_size);
>>>    -
>>>    -   if (!buf) {
>>>    -       DPRINTF("test dma buffer alloc failed");
>>>    -       return;
>>>    -   }
>>>
>>>        switch (val) {
>>>
>>> This is just the result of a quick grep for '\<g_' immediately followed
>>> by dead error handling in rocker.c.  Please do the same for the rest of
>>> the rocker sources, and additionally search for dead error handling
>>> separated from the allocation by function returns, like the one you
>>> fixed in pci_rocker_init().
>>
>> After reading your detailed explanation, I think I will try to do these:
>> 1. Searching for all the dead error handling in the rocker_*.c.
>> 2. All dead error handling from allocation functions will be as a separated
>>    patch. Similarly, from function returns as another patch. Is that right?
>
> A single patch to drop all dead error handling could be okay.  Hard to
> say before you see it.  Merging patches is less work than splitting
> them, so you might want to develop separate patches, then squash them
> together to see whether the result is easy enough to describe and
> understand.
>
> General advice: when you notice that writing a commit message is hard,
> stop and think whether it would be easier if the patch was split.
>

Hi, Markus

Benefited a lot from your pertinent advice. I think I got it.

Thanks:)
Mao