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