Jun Kamada
2008-Jul-03 11:40 UTC
[Xen-devel] [PATCH 2/4] pvSCSI : Fix many points of backend/frontend driver
Please refer following Mr. Steven''s mail posted on June 24th. Message-ID: <20080624131313.GB18379@weybridge.uk.xensource.com> Message-ID: <20080624131256.GA18379@weybridge.uk.xensource.com> Signed-off-by: Tomonari Horikoshi <t.horikoshi@jp.fujitsu.com> Signed-off-by: Jun Kamada <kama@jp.fujitsu.com> ----- Jun Kamada _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
Steven Smith
2008-Jul-04 16:21 UTC
Re: [Xen-devel] [PATCH 2/4] pvSCSI : Fix many points of backend/frontend driver
> 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. _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
Jun Kamada
2008-Jul-08 07:28 UTC
[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@jp.fujitsu.com> Signed-off-by: Jun Kamada <kama@jp.fujitsu.com> On Fri, 4 Jul 2008 17:21:59 +0100 Steven Smith <steven.smith@citrix.com> 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 _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
Steven Smith
2008-Jul-10 15:25 UTC
Re: [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)
> Attached patch is for following two points you mentioned.That looks much better, thanks. Steven. _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel