Re: [Xen-devel] [PATCH 2/4] pvSCSI : Fix many points of backend/frontend driver

[Steven Smith <steven.smith@xxxxxxxxxx>]

> 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.

Attachment: signature.asc
Description: Digital signature

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