[Qemu-devel] [PATCH 0/3] trivial patches

[Qemu-devel] [PATCH 0/3] trivial patches

[arei.gonglei]

From: Gonglei <arei.gonglei@huawei.com>

These trivial patches are collected by me
during qemu 2.3 hard-freeze time, which spoted by coverity.

Please ack or review if possible, thanks.

Gonglei (3):
  target-mips: fix memory leak
  xhci: remove unused code
  vhost-user: remove superfluous '\n' around error_report()

 hw/mips/mips_fulong2e.c |  1 +
 hw/mips/mips_malta.c    |  1 +
 hw/mips/mips_r4k.c      |  1 +
 hw/usb/hcd-xhci.c       |  1 -
 hw/virtio/vhost-user.c  | 22 +++++++++++-----------
 5 files changed, 14 insertions(+), 12 deletions(-)

-- 
1.7.12.4

[Qemu-devel] [PATCH 1/3] target-mips: fix memory leak

[arei.gonglei]

From: Gonglei <arei.gonglei@huawei.com>

Coveristy reports that variable prom_buf/params_buf going
out of scope leaks the storage it points to.

Cc: Aurelien Jarno <aurelien@aurel32.net>
Cc: Leon Alrae <leon.alrae@imgtec.com>
Signed-off-by: Gonglei <arei.gonglei@huawei.com>
---
 hw/mips/mips_fulong2e.c | 1 +
 hw/mips/mips_malta.c    | 1 +
 hw/mips/mips_r4k.c      | 1 +
 3 files changed, 3 insertions(+)

diff --git a/hw/mips/mips_fulong2e.c b/hw/mips/mips_fulong2e.c
index 4aae64a..dea941a 100644
--- a/hw/mips/mips_fulong2e.c
+++ b/hw/mips/mips_fulong2e.c
@@ -168,6 +168,7 @@ static int64_t load_kernel (CPUMIPSState *env)
     rom_add_blob_fixed("prom", prom_buf, prom_size,
                        cpu_mips_kseg0_to_phys(NULL, ENVP_ADDR));
 
+    g_free(prom_buf);
     return kernel_entry;
 }
 
diff --git a/hw/mips/mips_malta.c b/hw/mips/mips_malta.c
index b0fa71a..482250d 100644
--- a/hw/mips/mips_malta.c
+++ b/hw/mips/mips_malta.c
@@ -861,6 +861,7 @@ static int64_t load_kernel (void)
     rom_add_blob_fixed("prom", prom_buf, prom_size,
                        cpu_mips_kseg0_to_phys(NULL, ENVP_ADDR));
 
+    g_free(prom_buf);
     return kernel_entry;
 }
 
diff --git a/hw/mips/mips_r4k.c b/hw/mips/mips_r4k.c
index 66e2a58..f4dcacd 100644
--- a/hw/mips/mips_r4k.c
+++ b/hw/mips/mips_r4k.c
@@ -139,6 +139,7 @@ static int64_t load_kernel(void)
     rom_add_blob_fixed("params", params_buf, params_size,
                        (16 << 20) - 264);
 
+    g_free(params_buf);
     return entry;
 }
 
-- 
1.7.12.4

[Qemu-devel] [PATCH 2/3] xhci: remove unused code

[arei.gonglei]

From: Gonglei <arei.gonglei@huawei.com>

Value from xfer->packet.ep is assigned to ep here, but that
stored value is not used before it is overwritten. Remove it.

Cc: Gerd Hoffmann <kraxel@redhat.com>
Signed-off-by: Gonglei <arei.gonglei@huawei.com>
---
 hw/usb/hcd-xhci.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/hw/usb/hcd-xhci.c b/hw/usb/hcd-xhci.c
index ba15ae0..99f11fc 100644
--- a/hw/usb/hcd-xhci.c
+++ b/hw/usb/hcd-xhci.c
@@ -2204,7 +2204,6 @@ static void xhci_kick_ep(XHCIState *xhci, unsigned int slotid,
         if (epid == 1) {
             if (xhci_fire_ctl_transfer(xhci, xfer) >= 0) {
                 epctx->next_xfer = (epctx->next_xfer + 1) % TD_QUEUE;
-                ep = xfer->packet.ep;
             } else {
                 DPRINTF("xhci: error firing CTL transfer\n");
             }
-- 
1.7.12.4

[Qemu-devel] [PATCH 3/3] vhost-user: remove superfluous '\n' around error_report()

[arei.gonglei]

From: Gonglei <arei.gonglei@huawei.com>

Signed-off-by: Gonglei <arei.gonglei@huawei.com>
---
 hw/virtio/vhost-user.c | 22 +++++++++++-----------
 1 file changed, 11 insertions(+), 11 deletions(-)

diff --git a/hw/virtio/vhost-user.c b/hw/virtio/vhost-user.c
index aefe0bb..e7ab829 100644
--- a/hw/virtio/vhost-user.c
+++ b/hw/virtio/vhost-user.c
@@ -128,7 +128,7 @@ static int vhost_user_read(struct vhost_dev *dev, VhostUserMsg *msg)
 
     r = qemu_chr_fe_read_all(chr, p, size);
     if (r != size) {
-        error_report("Failed to read msg header. Read %d instead of %d.\n", r,
+        error_report("Failed to read msg header. Read %d instead of %d.", r,
                 size);
         goto fail;
     }
@@ -136,7 +136,7 @@ static int vhost_user_read(struct vhost_dev *dev, VhostUserMsg *msg)
     /* validate received flags */
     if (msg->flags != (VHOST_USER_REPLY_MASK | VHOST_USER_VERSION)) {
         error_report("Failed to read msg header."
-                " Flags 0x%x instead of 0x%x.\n", msg->flags,
+                " Flags 0x%x instead of 0x%x.", msg->flags,
                 VHOST_USER_REPLY_MASK | VHOST_USER_VERSION);
         goto fail;
     }
@@ -144,7 +144,7 @@ static int vhost_user_read(struct vhost_dev *dev, VhostUserMsg *msg)
     /* validate message size is sane */
     if (msg->size > VHOST_USER_PAYLOAD_SIZE) {
         error_report("Failed to read msg header."
-                " Size %d exceeds the maximum %zu.\n", msg->size,
+                " Size %d exceeds the maximum %zu.", msg->size,
                 VHOST_USER_PAYLOAD_SIZE);
         goto fail;
     }
@@ -155,7 +155,7 @@ static int vhost_user_read(struct vhost_dev *dev, VhostUserMsg *msg)
         r = qemu_chr_fe_read_all(chr, p, size);
         if (r != size) {
             error_report("Failed to read msg payload."
-                         " Read %d instead of %d.\n", r, msg->size);
+                         " Read %d instead of %d.", r, msg->size);
             goto fail;
         }
     }
@@ -235,8 +235,8 @@ static int vhost_user_call(struct vhost_dev *dev, unsigned long int request,
         msg.memory.nregions = fd_num;
 
         if (!fd_num) {
-            error_report("Failed initializing vhost-user memory map\n"
-                    "consider using -object memory-backend-file share=on\n");
+            error_report("Failed initializing vhost-user memory map, "
+                    "consider using -object memory-backend-file share=on");
             return -1;
         }
 
@@ -280,7 +280,7 @@ static int vhost_user_call(struct vhost_dev *dev, unsigned long int request,
         }
         break;
     default:
-        error_report("vhost-user trying to send unhandled ioctl\n");
+        error_report("vhost-user trying to send unhandled ioctl");
         return -1;
         break;
     }
@@ -296,27 +296,27 @@ static int vhost_user_call(struct vhost_dev *dev, unsigned long int request,
 
         if (msg_request != msg.request) {
             error_report("Received unexpected msg type."
-                    " Expected %d received %d\n", msg_request, msg.request);
+                    " Expected %d received %d", msg_request, msg.request);
             return -1;
         }
 
         switch (msg_request) {
         case VHOST_USER_GET_FEATURES:
             if (msg.size != sizeof(m.u64)) {
-                error_report("Received bad msg size.\n");
+                error_report("Received bad msg size.");
                 return -1;
             }
             *((__u64 *) arg) = msg.u64;
             break;
         case VHOST_USER_GET_VRING_BASE:
             if (msg.size != sizeof(m.state)) {
-                error_report("Received bad msg size.\n");
+                error_report("Received bad msg size.");
                 return -1;
             }
             memcpy(arg, &msg.state, sizeof(struct vhost_vring_state));
             break;
         default:
-            error_report("Received unexpected msg type.\n");
+            error_report("Received unexpected msg type.");
             return -1;
             break;
         }
-- 
1.7.12.4

Re: [Qemu-devel] [PATCH 1/3] target-mips: fix memory leak

[Leon Alrae]

On 28/04/2015 10:11, arei.gonglei@huawei.com wrote:
> From: Gonglei <arei.gonglei@huawei.com>
> 
> Coveristy reports that variable prom_buf/params_buf going
> out of scope leaks the storage it points to.
> 
> Cc: Aurelien Jarno <aurelien@aurel32.net>
> Cc: Leon Alrae <leon.alrae@imgtec.com>
> Signed-off-by: Gonglei <arei.gonglei@huawei.com>
> ---
>  hw/mips/mips_fulong2e.c | 1 +
>  hw/mips/mips_malta.c    | 1 +
>  hw/mips/mips_r4k.c      | 1 +
>  3 files changed, 3 insertions(+)

Reviewed-by: Leon Alrae <leon.alrae@imgtec.com>

Thanks,
Leon

Re: [Qemu-devel] [PATCH 3/3] vhost-user: remove superfluous '\n' around error_report()

[Andreas Färber]

Am 28.04.2015 um 11:11 schrieb arei.gonglei@huawei.com:
> From: Gonglei <arei.gonglei@huawei.com>
> 
> Signed-off-by: Gonglei <arei.gonglei@huawei.com>
> ---
>  hw/virtio/vhost-user.c | 22 +++++++++++-----------
>  1 file changed, 11 insertions(+), 11 deletions(-)
> 
> diff --git a/hw/virtio/vhost-user.c b/hw/virtio/vhost-user.c
> index aefe0bb..e7ab829 100644
> --- a/hw/virtio/vhost-user.c
> +++ b/hw/virtio/vhost-user.c
> @@ -128,7 +128,7 @@ static int vhost_user_read(struct vhost_dev *dev, VhostUserMsg *msg)
>  
>      r = qemu_chr_fe_read_all(chr, p, size);
>      if (r != size) {
> -        error_report("Failed to read msg header. Read %d instead of %d.\n", r,
> +        error_report("Failed to read msg header. Read %d instead of %d.", r,
>                  size);
>          goto fail;
>      }
> @@ -136,7 +136,7 @@ static int vhost_user_read(struct vhost_dev *dev, VhostUserMsg *msg)
>      /* validate received flags */
>      if (msg->flags != (VHOST_USER_REPLY_MASK | VHOST_USER_VERSION)) {
>          error_report("Failed to read msg header."
> -                " Flags 0x%x instead of 0x%x.\n", msg->flags,
> +                " Flags 0x%x instead of 0x%x.", msg->flags,
>                  VHOST_USER_REPLY_MASK | VHOST_USER_VERSION);
>          goto fail;
>      }
> @@ -144,7 +144,7 @@ static int vhost_user_read(struct vhost_dev *dev, VhostUserMsg *msg)
>      /* validate message size is sane */
>      if (msg->size > VHOST_USER_PAYLOAD_SIZE) {
>          error_report("Failed to read msg header."
> -                " Size %d exceeds the maximum %zu.\n", msg->size,
> +                " Size %d exceeds the maximum %zu.", msg->size,
>                  VHOST_USER_PAYLOAD_SIZE);
>          goto fail;
>      }
> @@ -155,7 +155,7 @@ static int vhost_user_read(struct vhost_dev *dev, VhostUserMsg *msg)
>          r = qemu_chr_fe_read_all(chr, p, size);
>          if (r != size) {
>              error_report("Failed to read msg payload."
> -                         " Read %d instead of %d.\n", r, msg->size);
> +                         " Read %d instead of %d.", r, msg->size);
>              goto fail;
>          }
>      }
> @@ -235,8 +235,8 @@ static int vhost_user_call(struct vhost_dev *dev, unsigned long int request,
>          msg.memory.nregions = fd_num;
>  
>          if (!fd_num) {
> -            error_report("Failed initializing vhost-user memory map\n"
> -                    "consider using -object memory-backend-file share=on\n");
> +            error_report("Failed initializing vhost-user memory map, "
> +                    "consider using -object memory-backend-file share=on");

This one is not just dropping a trailing \n, but looks good to me,

Reviewed-by: Andreas Färber <afaerber@suse.de>

Thanks,
Andreas

>              return -1;
>          }
>  
> @@ -280,7 +280,7 @@ static int vhost_user_call(struct vhost_dev *dev, unsigned long int request,
>          }
>          break;
>      default:
> -        error_report("vhost-user trying to send unhandled ioctl\n");
> +        error_report("vhost-user trying to send unhandled ioctl");
>          return -1;
>          break;
>      }
> @@ -296,27 +296,27 @@ static int vhost_user_call(struct vhost_dev *dev, unsigned long int request,
>  
>          if (msg_request != msg.request) {
>              error_report("Received unexpected msg type."
> -                    " Expected %d received %d\n", msg_request, msg.request);
> +                    " Expected %d received %d", msg_request, msg.request);
>              return -1;
>          }
>  
>          switch (msg_request) {
>          case VHOST_USER_GET_FEATURES:
>              if (msg.size != sizeof(m.u64)) {
> -                error_report("Received bad msg size.\n");
> +                error_report("Received bad msg size.");
>                  return -1;
>              }
>              *((__u64 *) arg) = msg.u64;
>              break;
>          case VHOST_USER_GET_VRING_BASE:
>              if (msg.size != sizeof(m.state)) {
> -                error_report("Received bad msg size.\n");
> +                error_report("Received bad msg size.");
>                  return -1;
>              }
>              memcpy(arg, &msg.state, sizeof(struct vhost_vring_state));
>              break;
>          default:
> -            error_report("Received unexpected msg type.\n");
> +            error_report("Received unexpected msg type.");
>              return -1;
>              break;
>          }
> 


-- 
SUSE Linux GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Felix Imendörffer, Jane Smithard, Jennifer Guild, Dilip Upmanyu,
Graham Norton; HRB 21284 (AG Nürnberg)

Re: [Qemu-devel] [PATCH 2/3] xhci: remove unused code

[Michael Tokarev]

28.04.2015 12:11, arei.gonglei@huawei.com wrote:
> From: Gonglei <arei.gonglei@huawei.com>
> 
> Value from xfer->packet.ep is assigned to ep here, but that
> stored value is not used before it is overwritten. Remove it.
> 
> Cc: Gerd Hoffmann <kraxel@redhat.com>
> Signed-off-by: Gonglei <arei.gonglei@huawei.com>
> ---
>  hw/usb/hcd-xhci.c | 1 -
>  1 file changed, 1 deletion(-)
> 
> diff --git a/hw/usb/hcd-xhci.c b/hw/usb/hcd-xhci.c
> index ba15ae0..99f11fc 100644
> --- a/hw/usb/hcd-xhci.c
> +++ b/hw/usb/hcd-xhci.c
> @@ -2204,7 +2204,6 @@ static void xhci_kick_ep(XHCIState *xhci, unsigned int slotid,
>          if (epid == 1) {
>              if (xhci_fire_ctl_transfer(xhci, xfer) >= 0) {
>                  epctx->next_xfer = (epctx->next_xfer + 1) % TD_QUEUE;
> -                ep = xfer->packet.ep;
>              } else {
>                  DPRINTF("xhci: error firing CTL transfer\n");
>              }

This one is somewhat fun.  ep variable is not used in whole this
function until the very end, with the code:


    ep = xhci_epid_to_usbep(xhci, slotid, epid);
    if (ep) {
        usb_device_flush_ep_queue(ep->dev, ep);
    }

There are only 2 assignments to it here, it is the NULL
initializer and this place which is being removed by this
patch (which is obviously unused).

So, I think if we were to drop this assignment, we should
remove the initializer too.  But before doing this, I think
we should try to remember _why_ this assignment is here in
the first place. The code looks like after the loop, this
ep variable was supposed to be used for something.  Or is
it just a leftover from 518ad5f2a075 (Cc'ing the author)?

I haven't looked at all this in more details without good
knowlege of the protocol and background.

Thanks,

/mjt

Re: [Qemu-devel] [PATCH 2/3] xhci: remove unused code

[Gonglei]

On 2015/4/29 14:48, Michael Tokarev wrote:
> 28.04.2015 12:11, arei.gonglei@huawei.com wrote:
>> From: Gonglei <arei.gonglei@huawei.com>
>>
>> Value from xfer->packet.ep is assigned to ep here, but that
>> stored value is not used before it is overwritten. Remove it.
>>
>> Cc: Gerd Hoffmann <kraxel@redhat.com>
>> Signed-off-by: Gonglei <arei.gonglei@huawei.com>
>> ---
>>  hw/usb/hcd-xhci.c | 1 -
>>  1 file changed, 1 deletion(-)
>>
>> diff --git a/hw/usb/hcd-xhci.c b/hw/usb/hcd-xhci.c
>> index ba15ae0..99f11fc 100644
>> --- a/hw/usb/hcd-xhci.c
>> +++ b/hw/usb/hcd-xhci.c
>> @@ -2204,7 +2204,6 @@ static void xhci_kick_ep(XHCIState *xhci, unsigned int slotid,
>>          if (epid == 1) {
>>              if (xhci_fire_ctl_transfer(xhci, xfer) >= 0) {
>>                  epctx->next_xfer = (epctx->next_xfer + 1) % TD_QUEUE;
>> -                ep = xfer->packet.ep;
>>              } else {
>>                  DPRINTF("xhci: error firing CTL transfer\n");
>>              }
> 
> This one is somewhat fun.  ep variable is not used in whole this
> function until the very end, with the code:
> 
> 
>     ep = xhci_epid_to_usbep(xhci, slotid, epid);
>     if (ep) {
>         usb_device_flush_ep_queue(ep->dev, ep);
>     }
> 
> There are only 2 assignments to it here, it is the NULL
> initializer and this place which is being removed by this
> patch (which is obviously unused).
> 
> So, I think if we were to drop this assignment, we should
> remove the initializer too.  But before doing this, I think
> we should try to remember _why_ this assignment is here in
> the first place. The code looks like after the loop, this
> ep variable was supposed to be used for something.  Or is
> it just a leftover from 518ad5f2a075 (Cc'ing the author)?
> 
I can't agree with you more. :)

Regards,
-Gonglei

> I haven't looked at all this in more details without good
> knowlege of the protocol and background.
> 
> Thanks,
> 
> /mjt
> 

Re: [Qemu-devel] [PATCH 2/3] xhci: remove unused code

[Gerd Hoffmann]

  Hi,


> > So, I think if we were to drop this assignment, we should
> > remove the initializer too.  But before doing this, I think
> > we should try to remember _why_ this assignment is here in
> > the first place. The code looks like after the loop, this
> > ep variable was supposed to be used for something.  Or is
> > it just a leftover from 518ad5f2a075 (Cc'ing the author)?
> > 
> I can't agree with you more. :)

Yes, certainly looks like a 518ad5f2a075 leftover.
Should be ok to cleanup.

cheers,
  Gerd

Re: [Qemu-devel] [PATCH 2/3] xhci: remove unused code

[Michael Tokarev]

Applied to -trivial, thanks!

/mjt