qemu-devel
[Top][All Lists]
Advanced

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

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


From: Mao Zhongyi
Subject: Re: [Qemu-devel] [PATCH v4 1/3] net/rocker: Remove the dead error handling
Date: Mon, 22 May 2017 11:47:24 +0800
User-agent: Mozilla/5.0 (X11; Linux x86_64; rv:45.0) Gecko/20100101 Thunderbird/45.8.0

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 <address@hidden> 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: address@hidden
Cc: address@hidden
Cc: address@hidden
Cc: address@hidden
Signed-off-by: Mao Zhongyi <address@hidden>
---
 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

























reply via email to

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