Hi Steven-san and all,
Attached patch is for following two points you mentioned.
Best regards,
Signed-off-by: Tomonari Horikoshi <t.horikoshi@xxxxxxxxxxxxxx>
Signed-off-by: Jun Kamada <kama@xxxxxxxxxxxxxx>
On Fri, 4 Jul 2008 17:21:59 +0100
Steven Smith <steven.smith@xxxxxxxxxx> wrote:
> > diff -r 3132ef07eecf -r e235e0bc18ab drivers/xen/scsiback/xenbus.c
> > --- a/drivers/xen/scsiback/xenbus.c Thu Jul 03 09:05:45 2008 +0900
> > +++ b/drivers/xen/scsiback/xenbus.c Thu Jul 03 13:46:31 2008 +0900
> > @@ -113,11 +113,42 @@ struct scsi_device *scsiback_get_scsi_de
> > goto invald_value;
> > }
> >
> > + scsi_host_put(shost);
> > invald_value:
> > return (sdev);
> > }
> >
> > #define VSCSIBACK_OP_ADD_OR_DEL_LUN 1
> > +#define VSCSIBACK_OP_UPDATEDEV_STATE 2
> > +
> > +static int scsiback_change_device_state(struct xenbus_device *dev,
> > + char *state_path, enum xenbus_state set_state)
> > +{
> > + struct xenbus_transaction tr;
> > + int err;
> > +
> > + do {
> > + err = xenbus_transaction_start(&tr);
> > + if (err != 0) {
> > + printk(KERN_ERR "scsiback: transaction start failed\n");
> > + return err;
> > + }
> > + err = xenbus_printf(tr, dev->nodename, state_path,
> > + "%d", set_state);
> > + if (err != 0) {
> > + printk(KERN_ERR "scsiback: xenbus_printf failed\n");
> > + xenbus_transaction_end(tr, 1);
> > + return err;
> > + }
> > + err = xenbus_transaction_end(tr, 0);
> > + } while (err == -EAGAIN);
> You only do one write in this transaction. That's kind of pointless;
> you could achieve the same effect more easily and more efficiently by
> just going
>
> + err = xenbus_printf(XBT_NIL, dev->nodename, state_path,
> + "%d", set_state);
>
> We have a lot of pointless transactions in other places, so I can
> understand why you were confused, but we don't really want to
> introduce any more.
> > +
> > + if (err != 0) {
> > + printk(KERN_ERR "scsiback: failed to end %s.\n", __FUNCTION__);
> > + return err;
> > + }
> > + return 0;
> > +}
> >
> > static void scsiback_do_lun_hotplug(struct backend_info *be, int op)
> > {
>
>
> > @@ -175,16 +201,22 @@ static void scsiback_do_lun_hotplug(stru
> > if (device_state == XenbusStateInitialising) {
> > sdev = scsiback_get_scsi_device(&phy);
> > if (!sdev) {
> > - xenbus_printf(xbt, dev->nodename,
> > state_str,
> > - "%d",
> > XenbusStateClosing);
> > + err = scsiback_change_device_state(dev,
> > + state_str, XenbusStateClosing);
> > + if (err)
> > + goto fail;
> > } else {
> > err =
> > scsiback_add_translation_entry(be->info, sdev, &vir);
> > if (!err) {
> > - xenbus_printf(xbt,
> > dev->nodename, state_str,
> > - "%d",
> > XenbusStateInitialised);
> > + err =
> > scsiback_change_device_state(dev,
> > + state_str,
> > XenbusStateInitialised);
> > + if (err)
> > + goto fail;
> > } else {
> > - xenbus_printf(xbt,
> > dev->nodename, state_str,
> > - "%d",
> > XenbusStateClosing);
> > + err =
> > scsiback_change_device_state(dev,
> > + state_str,
> > XenbusStateClosing);
> > + if (err)
> > + goto fail;
> > }
> > }
> > }
> I think you're leaking a reference to sdev when
> scsiback_add_translation_entry() fails, aren't you? In the success
> case scsiback_add_translation_entry() transfers it to the translation
> list, so you don't have to do anything, but in the failure case you
> do.
>
> Steven.
-----
Jun Kamada
linux_pvscsi_fix_attach.patch
Description: Binary data
_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxx
http://lists.xensource.com/xen-devel
|
Home