[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)

[Jun Kamada <kama@xxxxxxxxxxxxxx>]

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
Description: Binary data

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxx
http://lists.xensource.com/xen-devel