WARNING - OLD ARCHIVES

This is an archived copy of the Xen.org mailing list, which we have preserved to ensure that existing links to archives are not broken. The live archive, which contains the latest emails, can be found at http://lists.xen.org/
   
 
 
Xen 
 
Home Products Support Community News
 
   
 

xen-devel

[PATCH 3/4] pvSCSI: Fix some part for attach/detach functionality (Re: [

To: Steven Smith <steven.smith@xxxxxxxxxx>
Subject: [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)
From: Jun Kamada <kama@xxxxxxxxxxxxxx>
Date: Tue, 08 Jul 2008 16:28:10 +0900
Cc: kama@xxxxxxxxxxxxxx, xen-devel@xxxxxxxxxxxxxxxxxxx
Delivery-date: Tue, 08 Jul 2008 00:29:31 -0700
Envelope-to: www-data@xxxxxxxxxxxxxxxxxxx
In-reply-to: <20080704162159.GB21118@xxxxxxxxxxxxxxxxxxxxxxxxxx>
List-help: <mailto:xen-devel-request@lists.xensource.com?subject=help>
List-id: Xen developer discussion <xen-devel.lists.xensource.com>
List-post: <mailto:xen-devel@lists.xensource.com>
List-subscribe: <http://lists.xensource.com/mailman/listinfo/xen-devel>, <mailto:xen-devel-request@lists.xensource.com?subject=subscribe>
List-unsubscribe: <http://lists.xensource.com/mailman/listinfo/xen-devel>, <mailto:xen-devel-request@lists.xensource.com?subject=unsubscribe>
References: <20080703203710.8586.EB2C8575@xxxxxxxxxxxxxx> <20080704162159.GB21118@xxxxxxxxxxxxxxxxxxxxxxxxxx>
Sender: xen-devel-bounces@xxxxxxxxxxxxxxxxxxx
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
<Prev in Thread] Current Thread [Next in Thread>