> From: Markus Armbruster [mailto:arm...@redhat.com] > Sent: Thursday, September 18, 2014 3:38 PM > Subject: Re: [Qemu-devel] [PATCH] usb-storage: fix possible memory leak and > missing error message > > <arei.gong...@huawei.com> writes: > > > From: Gonglei <arei.gong...@huawei.com> > > > > When scsi_bus_legacy_add_drive() return NULL, meanwhile err will > > be not NULL, which will casue memory leak and missing error message. > > > > Signed-off-by: Gonglei <arei.gong...@huawei.com> > > --- > > 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. :) > 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! Best regards, -Gonglei
