Re: [Qemu-devel] [PATCH] usb-storage: fix possible memory leak and missing error message

[Gonglei (Arei)]

> >> > ---
> >> >  hw/usb/dev-storage.c | 11 ++++++-----
> >> >  1 file changed, 6 insertions(+), 5 deletions(-)
> >> >
> >> > diff --git a/hw/usb/dev-storage.c b/hw/usb/dev-storage.c
> >> > index 5c51ac0..cabd053 100644
> >> > --- a/hw/usb/dev-storage.c
> >> > +++ b/hw/usb/dev-storage.c
> >> > @@ -599,7 +599,6 @@ static int usb_msd_initfn_storage(USBDevice
> *dev)
> >> >  {
> >> >      MSDState *s = DO_UPCAST(MSDState, dev, dev);
> >> >      BlockDriverState *bs = s->conf.bs;
> >> > -    SCSIDevice *scsi_dev;
> >> >      Error *err = NULL;
> >> >
> >> >      if (!bs) {
> >> > @@ -625,10 +624,12 @@ static int usb_msd_initfn_storage(USBDevice
> *dev)
> >> >      usb_desc_init(dev);
> >> >      scsi_bus_new(&s->bus, sizeof(s->bus), DEVICE(dev),
> >> >                   &usb_msd_scsi_info_storage, NULL);
> >> > -    scsi_dev = scsi_bus_legacy_add_drive(&s->bus, bs,
> 0, !!s->removable,
> >> > -                                         s->conf.bootindex,
> dev->serial,
> >> > -                                         &err);
> >> > -    if (!scsi_dev) {
> >> > +    scsi_bus_legacy_add_drive(&s->bus, bs, 0, !!s->removable,
> >> > +                              s->conf.bootindex, dev->serial,
> >> > +                              &err);
> >> > +    if (err) {
> >> > +        error_report("%s", error_get_pretty(err));
> >> > +        error_free(err);
> >> >          return -1;
> >> >      }
> >> >      s->bus.qbus.allow_hotplug = 0;
> >>
> >> I'd keep the original condition and just fix the error message loss /
> >> error object leak:
> >>
> >> diff --git a/hw/usb/dev-storage.c b/hw/usb/dev-storage.c
> >> index ae4efcb..f731b0a 100644
> >> --- a/hw/usb/dev-storage.c
> >> +++ b/hw/usb/dev-storage.c
> >> @@ -624,6 +624,8 @@ static int usb_msd_initfn_storage(USBDevice *dev)
> >>                                           s->conf.bootindex,
> >> dev->serial,
> >>                                           &err);
> >>      if (!scsi_dev) {
> >> +        error_report("%s", error_get_pretty(err));
> >> +        error_free(err);
> >>          return -1;
> >>      }
> >>      s->bus.qbus.allow_hotplug = 0;
> >>
> >> Partly because it makes the fix more obvious, partly because I prefer
> >> checking the function value when it is available.  Matter of taste.
> >>
> > Hmm..
> >
> > The main problem is I'm afraid the scenario when scsi_dev
> > is NULL the err is NULL too in someday. I have fixed a similar issue
> > and you have reviewed. :)
> 
> Short answer: doesn't help, and you shouldn't be afraid anyway.
> 
> Now the long answer :)
> 
> Basic rules about returning errors through Error **errp parameters:
> 
> 1. The errp argument may be null.
> 
> 2. If it isn't, then it points to null.
> 
> 3. The function either succeeds or fails.
> 
> 4. If the function succeeds, it doesn't touch *errp.
> 
> 5. If the function fails, and
> 
>    a. if errp isn't null, it creates an Error object and returns it in
>    *errp.  The caller is responsible for freeing it.
> 
>    b. if errp is null, no Error object is created.
> 
> Corollary: when you pass a non-null errp argument, *errp is null exactly
> on success.
> 
> Rule 3 connects the Error contract with what else the function's
> supposed to do on success and on failure.  In particular with the return
> value.
> 
> Common case: a function returns a pointer to something on success, null
> pointer on failure.  When you pass a non-null errp, argument, *errp is
> null exactly when the function value is non-null.  In other words,
> either the value or *errp is null.
> 
> To detect errors, you can either check the function value, or your errp
> argument.
> 
> If you detect an error, you can either handle it locally, or propagate
> it to your caller.
> 
> Example: consider a function new_foo() returning a newly allocated Foo
> on success, null on error.  Any error we want to propagate.
> 
> Checking the errp argument:
> 
>     Error *local_err = NULL;
> 
>     foop = new_foo(arg, &local_err);
>     if (local_err) {
>         error_propagate(errp, local_err);
>         // rely on postcondition !foop (else we'd leak it here)
>         return;
>     }
> 
> Checking the function value:
> 
>     Error *local_err = NULL;
> 
>     foop = new_foo(arg, &local_err);
>     if (!foop) {
>         error_propagate(errp, local_err);
>         return;
>     }
>     // rely on postcondition !local_err (else, we'd leak it here)
> 
> No matter which of the two you check, you rely on new_foo()'s
> postcondition "either value or *errp is null".
> 
> Therefore, switching from one to the other does not make the code more
> robust.
> 
> Checking the function value can be simplified to just
> 
>     foop = new_foo(arg, errp);
>     if (!foop) {
>         return;
>     }
> 
> I use this whenever possible, because it's simpler, and less clutter.
> 
> What you fixed is a different, less common case: a function returning a
> list on success, null on failure, where the empty list is represented as
> null.  Because a null value can be legitimately returned both on success
> and on failure, you need to examine *errp to tell the to apart.  The
> code you fixed didn't.
> 
Great! Thanks a lot!

V2 will be posted shortly!

BTW, I will include this patch in my usb patch series (usb-bus: convert init to 
realize).

Best regards,
-Gonglei

> >> scsi_hot_add() in pci-hotplug-old.c also loses the error message.  Would
> >> you care to fix that, too?
> >
> > I have noticed that, but because scsi_hot_add() pass NULL to
> > scsi_bus_legacy_add_drive(), so I let it pass. As you remainder,
> > I will post another patch to fix it. Thanks!
> 
> Appreciated!