> >> > ---
> >> > 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!
