[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] [PATCH 3/4] pvSCSI: Fix some part for attach/detach functionality (Re: [Xen-devel] [PATCH 2/4] pvSCSI : Fix many points of backend/frontend driver)
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 Attachment:
linux_pvscsi_fix_attach.patch _______________________________________________ Xen-devel mailing list Xen-devel@xxxxxxxxxxxxxxxxxxx http://lists.xensource.com/xen-devel
|
![]() |
Lists.xenproject.org is hosted with RackSpace, monitoring our |