Re: [PATCH] xen/blkback: prevent repeated backend_changed invocations

2012-12-19 Thread Konrad Rzeszutek Wilk 
On Thu, Dec 13, 2012 at 03:02:11PM +, Jan Beulich wrote:
> >>> On 11.12.12 at 21:50, Olaf Hering  wrote:
> > backend_changed might be called multiple times, which will leak
> > be->mode. Make sure it will be called only once. Remove some unneeded
> > checks. Also the be->mode string was leaked, release the memory on
> > device shutdown.
> 
> So I decided to make an attempt myself, retaining the current
> behavior of allowing multiple calls, yet not having to sprinkle
> around multiple kfree()-s for be->mode. Slightly re-structuring
> the function made this pretty strait forward.

 Would it be possible to post a patch?

> 
> Jan
> 
> > Signed-off-by: Olaf Hering 
> > ---
> > 
> > incorporate all comments from Jan.
> > fold the oneline change to xen_blkbk_remove into this change
> > now its compile tested.
> > 
> >  drivers/block/xen-blkback/xenbus.c | 69 
> > ++
> >  1 file changed, 33 insertions(+), 36 deletions(-)
> > 
> > diff --git a/drivers/block/xen-blkback/xenbus.c 
> > b/drivers/block/xen-blkback/xenbus.c
> > index f58434c..5ca77c3 100644
> > --- a/drivers/block/xen-blkback/xenbus.c
> > +++ b/drivers/block/xen-blkback/xenbus.c
> > @@ -28,6 +28,7 @@ struct backend_info {
> > unsignedmajor;
> > unsignedminor;
> > char*mode;
> > +   unsignedalive;
> >  };
> >  
> >  static struct kmem_cache *xen_blkif_cachep;
> > @@ -366,6 +367,7 @@ static int xen_blkbk_remove(struct xenbus_device *dev)
> > be->blkif = NULL;
> > }
> >  
> > +   kfree(be->mode);
> > kfree(be);
> > dev_set_drvdata(>dev, NULL);
> > return 0;
> > @@ -501,10 +503,14 @@ static void backend_changed(struct xenbus_watch 
> > *watch,
> > = container_of(watch, struct backend_info, backend_watch);
> > struct xenbus_device *dev = be->dev;
> > int cdrom = 0;
> > -   char *device_type;
> > +   char *device_type, *p;
> > +   long handle;
> >  
> > DPRINTK("");
> >  
> > +   if (be->alive)
> > +   return;
> > +
> > err = xenbus_scanf(XBT_NIL, dev->nodename, "physical-device", "%x:%x",
> >, );
> > if (XENBUS_EXIST_ERR(err)) {
> > @@ -520,12 +526,7 @@ static void backend_changed(struct xenbus_watch *watch,
> > return;
> > }
> >  
> > -   if ((be->major || be->minor) &&
> > -   ((be->major != major) || (be->minor != minor))) {
> > -   pr_warn(DRV_PFX "changing physical device (from %x:%x to %x:%x) 
> > not 
> > supported.\n",
> > -   be->major, be->minor, major, minor);
> > -   return;
> > -   }
> > +   be->alive = 1;
> >  
> > be->mode = xenbus_read(XBT_NIL, dev->nodename, "mode", NULL);
> > if (IS_ERR(be->mode)) {
> > @@ -541,39 +542,35 @@ static void backend_changed(struct xenbus_watch 
> > *watch,
> > kfree(device_type);
> > }
> >  
> > -   if (be->major == 0 && be->minor == 0) {
> > -   /* Front end dir is a number, which is used as the handle. */
> > -
> > -   char *p = strrchr(dev->otherend, '/') + 1;
> > -   long handle;
> > -   err = strict_strtoul(p, 0, );
> > -   if (err)
> > -   return;
> > -
> > -   be->major = major;
> > -   be->minor = minor;
> > +   /* Front end dir is a number, which is used as the handle. */
> > +   p = strrchr(dev->otherend, '/') + 1;
> > +   err = strict_strtoul(p, 0, );
> > +   if (err)
> > +   return;
> >  
> > -   err = xen_vbd_create(be->blkif, handle, major, minor,
> > -(NULL == strchr(be->mode, 'w')), cdrom);
> > -   if (err) {
> > -   be->major = 0;
> > -   be->minor = 0;
> > -   xenbus_dev_fatal(dev, err, "creating vbd structure");
> > -   return;
> > -   }
> > +   be->major = major;
> > +   be->minor = minor;
> >  
> > -   err = xenvbd_sysfs_addif(dev);
> > -   if (err) {
> > -   xen_vbd_free(>blkif->vbd);
> > -   be->major = 0;
> > -   be->minor = 0;
> > -   xenbus_dev_fatal(dev, err, "creating sysfs entries");
> > -   return;
> > -   }
> > +   err = xen_vbd_create(be->blkif, handle, major, minor,
> > +(NULL == strchr(be->mode, 'w')), cdrom);
> > +   if (err) {
> > +   be->major = 0;
> > +   be->minor = 0;
> > +   xenbus_dev_fatal(dev, err, "creating vbd structure");
> > +   return;
> > +   }
> >  
> > -   /* We're potentially connected now */
> > -   xen_update_blkif_status(be->blkif);
> > +   err = xenvbd_sysfs_addif(dev);
> > +   if (err) {
> > +   xen_vbd_free(>blkif->vbd);
> > +   be->major = 0;
> > +   be->minor = 0;
> > +   xenbus_dev_fatal(dev, err, "creating sysfs entries");
> > +   return;
> > }
> > +
> > +   /*

Re: [PATCH] xen/blkback: prevent repeated backend_changed invocations

2012-12-19 Thread Konrad Rzeszutek Wilk 
On Thu, Dec 13, 2012 at 03:02:11PM +, Jan Beulich wrote:
  On 11.12.12 at 21:50, Olaf Hering o...@aepfle.de wrote:
  backend_changed might be called multiple times, which will leak
  be-mode. Make sure it will be called only once. Remove some unneeded
  checks. Also the be-mode string was leaked, release the memory on
  device shutdown.
 
 So I decided to make an attempt myself, retaining the current
 behavior of allowing multiple calls, yet not having to sprinkle
 around multiple kfree()-s for be-mode. Slightly re-structuring
 the function made this pretty strait forward.

nods Would it be possible to post a patch?

 
 Jan
 
  Signed-off-by: Olaf Hering o...@aepfle.de
  ---
  
  incorporate all comments from Jan.
  fold the oneline change to xen_blkbk_remove into this change
  now its compile tested.
  
   drivers/block/xen-blkback/xenbus.c | 69 
  ++
   1 file changed, 33 insertions(+), 36 deletions(-)
  
  diff --git a/drivers/block/xen-blkback/xenbus.c 
  b/drivers/block/xen-blkback/xenbus.c
  index f58434c..5ca77c3 100644
  --- a/drivers/block/xen-blkback/xenbus.c
  +++ b/drivers/block/xen-blkback/xenbus.c
  @@ -28,6 +28,7 @@ struct backend_info {
  unsignedmajor;
  unsignedminor;
  char*mode;
  +   unsignedalive;
   };
   
   static struct kmem_cache *xen_blkif_cachep;
  @@ -366,6 +367,7 @@ static int xen_blkbk_remove(struct xenbus_device *dev)
  be-blkif = NULL;
  }
   
  +   kfree(be-mode);
  kfree(be);
  dev_set_drvdata(dev-dev, NULL);
  return 0;
  @@ -501,10 +503,14 @@ static void backend_changed(struct xenbus_watch 
  *watch,
  = container_of(watch, struct backend_info, backend_watch);
  struct xenbus_device *dev = be-dev;
  int cdrom = 0;
  -   char *device_type;
  +   char *device_type, *p;
  +   long handle;
   
  DPRINTK();
   
  +   if (be-alive)
  +   return;
  +
  err = xenbus_scanf(XBT_NIL, dev-nodename, physical-device, %x:%x,
 major, minor);
  if (XENBUS_EXIST_ERR(err)) {
  @@ -520,12 +526,7 @@ static void backend_changed(struct xenbus_watch *watch,
  return;
  }
   
  -   if ((be-major || be-minor) 
  -   ((be-major != major) || (be-minor != minor))) {
  -   pr_warn(DRV_PFX changing physical device (from %x:%x to %x:%x) 
  not 
  supported.\n,
  -   be-major, be-minor, major, minor);
  -   return;
  -   }
  +   be-alive = 1;
   
  be-mode = xenbus_read(XBT_NIL, dev-nodename, mode, NULL);
  if (IS_ERR(be-mode)) {
  @@ -541,39 +542,35 @@ static void backend_changed(struct xenbus_watch 
  *watch,
  kfree(device_type);
  }
   
  -   if (be-major == 0  be-minor == 0) {
  -   /* Front end dir is a number, which is used as the handle. */
  -
  -   char *p = strrchr(dev-otherend, '/') + 1;
  -   long handle;
  -   err = strict_strtoul(p, 0, handle);
  -   if (err)
  -   return;
  -
  -   be-major = major;
  -   be-minor = minor;
  +   /* Front end dir is a number, which is used as the handle. */
  +   p = strrchr(dev-otherend, '/') + 1;
  +   err = strict_strtoul(p, 0, handle);
  +   if (err)
  +   return;
   
  -   err = xen_vbd_create(be-blkif, handle, major, minor,
  -(NULL == strchr(be-mode, 'w')), cdrom);
  -   if (err) {
  -   be-major = 0;
  -   be-minor = 0;
  -   xenbus_dev_fatal(dev, err, creating vbd structure);
  -   return;
  -   }
  +   be-major = major;
  +   be-minor = minor;
   
  -   err = xenvbd_sysfs_addif(dev);
  -   if (err) {
  -   xen_vbd_free(be-blkif-vbd);
  -   be-major = 0;
  -   be-minor = 0;
  -   xenbus_dev_fatal(dev, err, creating sysfs entries);
  -   return;
  -   }
  +   err = xen_vbd_create(be-blkif, handle, major, minor,
  +(NULL == strchr(be-mode, 'w')), cdrom);
  +   if (err) {
  +   be-major = 0;
  +   be-minor = 0;
  +   xenbus_dev_fatal(dev, err, creating vbd structure);
  +   return;
  +   }
   
  -   /* We're potentially connected now */
  -   xen_update_blkif_status(be-blkif);
  +   err = xenvbd_sysfs_addif(dev);
  +   if (err) {
  +   xen_vbd_free(be-blkif-vbd);
  +   be-major = 0;
  +   be-minor = 0;
  +   xenbus_dev_fatal(dev, err, creating sysfs entries);
  +   return;
  }
  +
  +   /* We're potentially connected now */
  +   xen_update_blkif_status(be-blkif);
   }
   
   
  -- 
  1.8.0.1
 
 
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at

Re: [PATCH] xen/blkback: prevent repeated backend_changed invocations

2012-12-13 Thread Jan Beulich 
>>> On 11.12.12 at 21:50, Olaf Hering  wrote:
> backend_changed might be called multiple times, which will leak
> be->mode. Make sure it will be called only once. Remove some unneeded
> checks. Also the be->mode string was leaked, release the memory on
> device shutdown.

So I decided to make an attempt myself, retaining the current
behavior of allowing multiple calls, yet not having to sprinkle
around multiple kfree()-s for be->mode. Slightly re-structuring
the function made this pretty strait forward.

Jan

> Signed-off-by: Olaf Hering 
> ---
> 
> incorporate all comments from Jan.
> fold the oneline change to xen_blkbk_remove into this change
> now its compile tested.
> 
>  drivers/block/xen-blkback/xenbus.c | 69 
> ++
>  1 file changed, 33 insertions(+), 36 deletions(-)
> 
> diff --git a/drivers/block/xen-blkback/xenbus.c 
> b/drivers/block/xen-blkback/xenbus.c
> index f58434c..5ca77c3 100644
> --- a/drivers/block/xen-blkback/xenbus.c
> +++ b/drivers/block/xen-blkback/xenbus.c
> @@ -28,6 +28,7 @@ struct backend_info {
>   unsignedmajor;
>   unsignedminor;
>   char*mode;
> + unsignedalive;
>  };
>  
>  static struct kmem_cache *xen_blkif_cachep;
> @@ -366,6 +367,7 @@ static int xen_blkbk_remove(struct xenbus_device *dev)
>   be->blkif = NULL;
>   }
>  
> + kfree(be->mode);
>   kfree(be);
>   dev_set_drvdata(>dev, NULL);
>   return 0;
> @@ -501,10 +503,14 @@ static void backend_changed(struct xenbus_watch *watch,
>   = container_of(watch, struct backend_info, backend_watch);
>   struct xenbus_device *dev = be->dev;
>   int cdrom = 0;
> - char *device_type;
> + char *device_type, *p;
> + long handle;
>  
>   DPRINTK("");
>  
> + if (be->alive)
> + return;
> +
>   err = xenbus_scanf(XBT_NIL, dev->nodename, "physical-device", "%x:%x",
>  , );
>   if (XENBUS_EXIST_ERR(err)) {
> @@ -520,12 +526,7 @@ static void backend_changed(struct xenbus_watch *watch,
>   return;
>   }
>  
> - if ((be->major || be->minor) &&
> - ((be->major != major) || (be->minor != minor))) {
> - pr_warn(DRV_PFX "changing physical device (from %x:%x to %x:%x) 
> not 
> supported.\n",
> - be->major, be->minor, major, minor);
> - return;
> - }
> + be->alive = 1;
>  
>   be->mode = xenbus_read(XBT_NIL, dev->nodename, "mode", NULL);
>   if (IS_ERR(be->mode)) {
> @@ -541,39 +542,35 @@ static void backend_changed(struct xenbus_watch *watch,
>   kfree(device_type);
>   }
>  
> - if (be->major == 0 && be->minor == 0) {
> - /* Front end dir is a number, which is used as the handle. */
> -
> - char *p = strrchr(dev->otherend, '/') + 1;
> - long handle;
> - err = strict_strtoul(p, 0, );
> - if (err)
> - return;
> -
> - be->major = major;
> - be->minor = minor;
> + /* Front end dir is a number, which is used as the handle. */
> + p = strrchr(dev->otherend, '/') + 1;
> + err = strict_strtoul(p, 0, );
> + if (err)
> + return;
>  
> - err = xen_vbd_create(be->blkif, handle, major, minor,
> -  (NULL == strchr(be->mode, 'w')), cdrom);
> - if (err) {
> - be->major = 0;
> - be->minor = 0;
> - xenbus_dev_fatal(dev, err, "creating vbd structure");
> - return;
> - }
> + be->major = major;
> + be->minor = minor;
>  
> - err = xenvbd_sysfs_addif(dev);
> - if (err) {
> - xen_vbd_free(>blkif->vbd);
> - be->major = 0;
> - be->minor = 0;
> - xenbus_dev_fatal(dev, err, "creating sysfs entries");
> - return;
> - }
> + err = xen_vbd_create(be->blkif, handle, major, minor,
> +  (NULL == strchr(be->mode, 'w')), cdrom);
> + if (err) {
> + be->major = 0;
> + be->minor = 0;
> + xenbus_dev_fatal(dev, err, "creating vbd structure");
> + return;
> + }
>  
> - /* We're potentially connected now */
> - xen_update_blkif_status(be->blkif);
> + err = xenvbd_sysfs_addif(dev);
> + if (err) {
> + xen_vbd_free(>blkif->vbd);
> + be->major = 0;
> + be->minor = 0;
> + xenbus_dev_fatal(dev, err, "creating sysfs entries");
> + return;
>   }
> +
> + /* We're potentially connected now */
> + xen_update_blkif_status(be->blkif);
>  }
>  
>  
> -- 
> 1.8.0.1


--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to

Re: [PATCH] xen/blkback: prevent repeated backend_changed invocations

2012-12-13 Thread Jan Beulich 
 On 11.12.12 at 21:50, Olaf Hering o...@aepfle.de wrote:
 backend_changed might be called multiple times, which will leak
 be-mode. Make sure it will be called only once. Remove some unneeded
 checks. Also the be-mode string was leaked, release the memory on
 device shutdown.

So I decided to make an attempt myself, retaining the current
behavior of allowing multiple calls, yet not having to sprinkle
around multiple kfree()-s for be-mode. Slightly re-structuring
the function made this pretty strait forward.

Jan

 Signed-off-by: Olaf Hering o...@aepfle.de
 ---
 
 incorporate all comments from Jan.
 fold the oneline change to xen_blkbk_remove into this change
 now its compile tested.
 
  drivers/block/xen-blkback/xenbus.c | 69 
 ++
  1 file changed, 33 insertions(+), 36 deletions(-)
 
 diff --git a/drivers/block/xen-blkback/xenbus.c 
 b/drivers/block/xen-blkback/xenbus.c
 index f58434c..5ca77c3 100644
 --- a/drivers/block/xen-blkback/xenbus.c
 +++ b/drivers/block/xen-blkback/xenbus.c
 @@ -28,6 +28,7 @@ struct backend_info {
   unsignedmajor;
   unsignedminor;
   char*mode;
 + unsignedalive;
  };
  
  static struct kmem_cache *xen_blkif_cachep;
 @@ -366,6 +367,7 @@ static int xen_blkbk_remove(struct xenbus_device *dev)
   be-blkif = NULL;
   }
  
 + kfree(be-mode);
   kfree(be);
   dev_set_drvdata(dev-dev, NULL);
   return 0;
 @@ -501,10 +503,14 @@ static void backend_changed(struct xenbus_watch *watch,
   = container_of(watch, struct backend_info, backend_watch);
   struct xenbus_device *dev = be-dev;
   int cdrom = 0;
 - char *device_type;
 + char *device_type, *p;
 + long handle;
  
   DPRINTK();
  
 + if (be-alive)
 + return;
 +
   err = xenbus_scanf(XBT_NIL, dev-nodename, physical-device, %x:%x,
  major, minor);
   if (XENBUS_EXIST_ERR(err)) {
 @@ -520,12 +526,7 @@ static void backend_changed(struct xenbus_watch *watch,
   return;
   }
  
 - if ((be-major || be-minor) 
 - ((be-major != major) || (be-minor != minor))) {
 - pr_warn(DRV_PFX changing physical device (from %x:%x to %x:%x) 
 not 
 supported.\n,
 - be-major, be-minor, major, minor);
 - return;
 - }
 + be-alive = 1;
  
   be-mode = xenbus_read(XBT_NIL, dev-nodename, mode, NULL);
   if (IS_ERR(be-mode)) {
 @@ -541,39 +542,35 @@ static void backend_changed(struct xenbus_watch *watch,
   kfree(device_type);
   }
  
 - if (be-major == 0  be-minor == 0) {
 - /* Front end dir is a number, which is used as the handle. */
 -
 - char *p = strrchr(dev-otherend, '/') + 1;
 - long handle;
 - err = strict_strtoul(p, 0, handle);
 - if (err)
 - return;
 -
 - be-major = major;
 - be-minor = minor;
 + /* Front end dir is a number, which is used as the handle. */
 + p = strrchr(dev-otherend, '/') + 1;
 + err = strict_strtoul(p, 0, handle);
 + if (err)
 + return;
  
 - err = xen_vbd_create(be-blkif, handle, major, minor,
 -  (NULL == strchr(be-mode, 'w')), cdrom);
 - if (err) {
 - be-major = 0;
 - be-minor = 0;
 - xenbus_dev_fatal(dev, err, creating vbd structure);
 - return;
 - }
 + be-major = major;
 + be-minor = minor;
  
 - err = xenvbd_sysfs_addif(dev);
 - if (err) {
 - xen_vbd_free(be-blkif-vbd);
 - be-major = 0;
 - be-minor = 0;
 - xenbus_dev_fatal(dev, err, creating sysfs entries);
 - return;
 - }
 + err = xen_vbd_create(be-blkif, handle, major, minor,
 +  (NULL == strchr(be-mode, 'w')), cdrom);
 + if (err) {
 + be-major = 0;
 + be-minor = 0;
 + xenbus_dev_fatal(dev, err, creating vbd structure);
 + return;
 + }
  
 - /* We're potentially connected now */
 - xen_update_blkif_status(be-blkif);
 + err = xenvbd_sysfs_addif(dev);
 + if (err) {
 + xen_vbd_free(be-blkif-vbd);
 + be-major = 0;
 + be-minor = 0;
 + xenbus_dev_fatal(dev, err, creating sysfs entries);
 + return;
   }
 +
 + /* We're potentially connected now */
 + xen_update_blkif_status(be-blkif);
  }
  
  
 -- 
 1.8.0.1


--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [Xen-devel] [PATCH] xen/blkback: prevent repeated backend_changed invocations

2012-12-12 Thread Olaf Hering 
On Wed, Dec 12, Ian Campbell wrote:

> On Wed, 2012-12-12 at 09:42 +, Jan Beulich wrote:
> > >>> On 11.12.12 at 21:50, Olaf Hering  wrote:
> > > backend_changed might be called multiple times, which will leak
> > > be->mode. Make sure it will be called only once. Remove some unneeded
> > > checks. Also the be->mode string was leaked, release the memory on
> > > device shutdown.
> > 
> > So did I miss some discussion here? I haven't seen any
> > confirmation of this function indeed being supposed to be called
> > just once.
> > 
> > Also, as said previously, if indeed it is to be called just once,
> > removing the watch during/after the first invocation would seem
> > to be the more appropriate thing to do.
> 
> The watch fires (often needlessly) when you first register it so in the
> common case it is going to be called twice. Of course that first time
> should abort early on so perhaps that's a moot point.

The current code handles that, if a property does not exist the function
will exit early.

Olaf
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [Xen-devel] [PATCH] xen/blkback: prevent repeated backend_changed invocations

2012-12-12 Thread Ian Campbell 
On Wed, 2012-12-12 at 09:42 +, Jan Beulich wrote:
> >>> On 11.12.12 at 21:50, Olaf Hering  wrote:
> > backend_changed might be called multiple times, which will leak
> > be->mode. Make sure it will be called only once. Remove some unneeded
> > checks. Also the be->mode string was leaked, release the memory on
> > device shutdown.
> 
> So did I miss some discussion here? I haven't seen any
> confirmation of this function indeed being supposed to be called
> just once.
> 
> Also, as said previously, if indeed it is to be called just once,
> removing the watch during/after the first invocation would seem
> to be the more appropriate thing to do.

The watch fires (often needlessly) when you first register it so in the
common case it is going to be called twice. Of course that first time
should abort early on so perhaps that's a moot point.

Ian.

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH] xen/blkback: prevent repeated backend_changed invocations

2012-12-12 Thread Jan Beulich 
>>> On 12.12.12 at 10:47, Olaf Hering  wrote:
> On Wed, Dec 12, Jan Beulich wrote:
> 
>> >>> On 11.12.12 at 21:50, Olaf Hering  wrote:
>> > backend_changed might be called multiple times, which will leak
>> > be->mode. Make sure it will be called only once. Remove some unneeded
>> > checks. Also the be->mode string was leaked, release the memory on
>> > device shutdown.
>> 
>> So did I miss some discussion here? I haven't seen any
>> confirmation of this function indeed being supposed to be called
>> just once.
>> 
>> Also, as said previously, if indeed it is to be called just once,
>> removing the watch during/after the first invocation would seem
>> to be the more appropriate thing to do.
> 
> Does the API allow this, that the called function can disable the watch?

That is what would need looking into (and why I said "during/after").

Jan

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH] xen/blkback: prevent repeated backend_changed invocations

2012-12-12 Thread Olaf Hering 
On Wed, Dec 12, Jan Beulich wrote:

> >>> On 11.12.12 at 21:50, Olaf Hering  wrote:
> > backend_changed might be called multiple times, which will leak
> > be->mode. Make sure it will be called only once. Remove some unneeded
> > checks. Also the be->mode string was leaked, release the memory on
> > device shutdown.
> 
> So did I miss some discussion here? I haven't seen any
> confirmation of this function indeed being supposed to be called
> just once.
> 
> Also, as said previously, if indeed it is to be called just once,
> removing the watch during/after the first invocation would seem
> to be the more appropriate thing to do.

Does the API allow this, that the called function can disable the watch?

Olaf
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH] xen/blkback: prevent repeated backend_changed invocations

2012-12-12 Thread Jan Beulich 
>>> On 11.12.12 at 21:50, Olaf Hering  wrote:
> backend_changed might be called multiple times, which will leak
> be->mode. Make sure it will be called only once. Remove some unneeded
> checks. Also the be->mode string was leaked, release the memory on
> device shutdown.

So did I miss some discussion here? I haven't seen any
confirmation of this function indeed being supposed to be called
just once.

Also, as said previously, if indeed it is to be called just once,
removing the watch during/after the first invocation would seem
to be the more appropriate thing to do.

Jan

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH] xen/blkback: prevent repeated backend_changed invocations

2012-12-12 Thread Jan Beulich 
 On 11.12.12 at 21:50, Olaf Hering o...@aepfle.de wrote:
 backend_changed might be called multiple times, which will leak
 be-mode. Make sure it will be called only once. Remove some unneeded
 checks. Also the be-mode string was leaked, release the memory on
 device shutdown.

So did I miss some discussion here? I haven't seen any
confirmation of this function indeed being supposed to be called
just once.

Also, as said previously, if indeed it is to be called just once,
removing the watch during/after the first invocation would seem
to be the more appropriate thing to do.

Jan

--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH] xen/blkback: prevent repeated backend_changed invocations

2012-12-12 Thread Olaf Hering 
On Wed, Dec 12, Jan Beulich wrote:

  On 11.12.12 at 21:50, Olaf Hering o...@aepfle.de wrote:
  backend_changed might be called multiple times, which will leak
  be-mode. Make sure it will be called only once. Remove some unneeded
  checks. Also the be-mode string was leaked, release the memory on
  device shutdown.
 
 So did I miss some discussion here? I haven't seen any
 confirmation of this function indeed being supposed to be called
 just once.
 
 Also, as said previously, if indeed it is to be called just once,
 removing the watch during/after the first invocation would seem
 to be the more appropriate thing to do.

Does the API allow this, that the called function can disable the watch?

Olaf
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH] xen/blkback: prevent repeated backend_changed invocations

2012-12-12 Thread Jan Beulich 
 On 12.12.12 at 10:47, Olaf Hering o...@aepfle.de wrote:
 On Wed, Dec 12, Jan Beulich wrote:
 
  On 11.12.12 at 21:50, Olaf Hering o...@aepfle.de wrote:
  backend_changed might be called multiple times, which will leak
  be-mode. Make sure it will be called only once. Remove some unneeded
  checks. Also the be-mode string was leaked, release the memory on
  device shutdown.
 
 So did I miss some discussion here? I haven't seen any
 confirmation of this function indeed being supposed to be called
 just once.
 
 Also, as said previously, if indeed it is to be called just once,
 removing the watch during/after the first invocation would seem
 to be the more appropriate thing to do.
 
 Does the API allow this, that the called function can disable the watch?

That is what would need looking into (and why I said during/after).

Jan

--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [Xen-devel] [PATCH] xen/blkback: prevent repeated backend_changed invocations

2012-12-12 Thread Ian Campbell 
On Wed, 2012-12-12 at 09:42 +, Jan Beulich wrote:
  On 11.12.12 at 21:50, Olaf Hering o...@aepfle.de wrote:
  backend_changed might be called multiple times, which will leak
  be-mode. Make sure it will be called only once. Remove some unneeded
  checks. Also the be-mode string was leaked, release the memory on
  device shutdown.
 
 So did I miss some discussion here? I haven't seen any
 confirmation of this function indeed being supposed to be called
 just once.
 
 Also, as said previously, if indeed it is to be called just once,
 removing the watch during/after the first invocation would seem
 to be the more appropriate thing to do.

The watch fires (often needlessly) when you first register it so in the
common case it is going to be called twice. Of course that first time
should abort early on so perhaps that's a moot point.

Ian.

--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [Xen-devel] [PATCH] xen/blkback: prevent repeated backend_changed invocations

2012-12-12 Thread Olaf Hering 
On Wed, Dec 12, Ian Campbell wrote:

 On Wed, 2012-12-12 at 09:42 +, Jan Beulich wrote:
   On 11.12.12 at 21:50, Olaf Hering o...@aepfle.de wrote:
   backend_changed might be called multiple times, which will leak
   be-mode. Make sure it will be called only once. Remove some unneeded
   checks. Also the be-mode string was leaked, release the memory on
   device shutdown.
  
  So did I miss some discussion here? I haven't seen any
  confirmation of this function indeed being supposed to be called
  just once.
  
  Also, as said previously, if indeed it is to be called just once,
  removing the watch during/after the first invocation would seem
  to be the more appropriate thing to do.
 
 The watch fires (often needlessly) when you first register it so in the
 common case it is going to be called twice. Of course that first time
 should abort early on so perhaps that's a moot point.

The current code handles that, if a property does not exist the function
will exit early.

Olaf
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[PATCH] xen/blkback: prevent repeated backend_changed invocations

2012-12-11 Thread Olaf Hering 
backend_changed might be called multiple times, which will leak
be->mode. Make sure it will be called only once. Remove some unneeded
checks. Also the be->mode string was leaked, release the memory on
device shutdown.

Signed-off-by: Olaf Hering 
---

incorporate all comments from Jan.
fold the oneline change to xen_blkbk_remove into this change
now its compile tested.

 drivers/block/xen-blkback/xenbus.c | 69 ++
 1 file changed, 33 insertions(+), 36 deletions(-)

diff --git a/drivers/block/xen-blkback/xenbus.c 
b/drivers/block/xen-blkback/xenbus.c
index f58434c..5ca77c3 100644
--- a/drivers/block/xen-blkback/xenbus.c
+++ b/drivers/block/xen-blkback/xenbus.c
@@ -28,6 +28,7 @@ struct backend_info {
unsignedmajor;
unsignedminor;
char*mode;
+   unsignedalive;
 };
 
 static struct kmem_cache *xen_blkif_cachep;
@@ -366,6 +367,7 @@ static int xen_blkbk_remove(struct xenbus_device *dev)
be->blkif = NULL;
}
 
+   kfree(be->mode);
kfree(be);
dev_set_drvdata(>dev, NULL);
return 0;
@@ -501,10 +503,14 @@ static void backend_changed(struct xenbus_watch *watch,
= container_of(watch, struct backend_info, backend_watch);
struct xenbus_device *dev = be->dev;
int cdrom = 0;
-   char *device_type;
+   char *device_type, *p;
+   long handle;
 
DPRINTK("");
 
+   if (be->alive)
+   return;
+
err = xenbus_scanf(XBT_NIL, dev->nodename, "physical-device", "%x:%x",
   , );
if (XENBUS_EXIST_ERR(err)) {
@@ -520,12 +526,7 @@ static void backend_changed(struct xenbus_watch *watch,
return;
}
 
-   if ((be->major || be->minor) &&
-   ((be->major != major) || (be->minor != minor))) {
-   pr_warn(DRV_PFX "changing physical device (from %x:%x to %x:%x) 
not supported.\n",
-   be->major, be->minor, major, minor);
-   return;
-   }
+   be->alive = 1;
 
be->mode = xenbus_read(XBT_NIL, dev->nodename, "mode", NULL);
if (IS_ERR(be->mode)) {
@@ -541,39 +542,35 @@ static void backend_changed(struct xenbus_watch *watch,
kfree(device_type);
}
 
-   if (be->major == 0 && be->minor == 0) {
-   /* Front end dir is a number, which is used as the handle. */
-
-   char *p = strrchr(dev->otherend, '/') + 1;
-   long handle;
-   err = strict_strtoul(p, 0, );
-   if (err)
-   return;
-
-   be->major = major;
-   be->minor = minor;
+   /* Front end dir is a number, which is used as the handle. */
+   p = strrchr(dev->otherend, '/') + 1;
+   err = strict_strtoul(p, 0, );
+   if (err)
+   return;
 
-   err = xen_vbd_create(be->blkif, handle, major, minor,
-(NULL == strchr(be->mode, 'w')), cdrom);
-   if (err) {
-   be->major = 0;
-   be->minor = 0;
-   xenbus_dev_fatal(dev, err, "creating vbd structure");
-   return;
-   }
+   be->major = major;
+   be->minor = minor;
 
-   err = xenvbd_sysfs_addif(dev);
-   if (err) {
-   xen_vbd_free(>blkif->vbd);
-   be->major = 0;
-   be->minor = 0;
-   xenbus_dev_fatal(dev, err, "creating sysfs entries");
-   return;
-   }
+   err = xen_vbd_create(be->blkif, handle, major, minor,
+(NULL == strchr(be->mode, 'w')), cdrom);
+   if (err) {
+   be->major = 0;
+   be->minor = 0;
+   xenbus_dev_fatal(dev, err, "creating vbd structure");
+   return;
+   }
 
-   /* We're potentially connected now */
-   xen_update_blkif_status(be->blkif);
+   err = xenvbd_sysfs_addif(dev);
+   if (err) {
+   xen_vbd_free(>blkif->vbd);
+   be->major = 0;
+   be->minor = 0;
+   xenbus_dev_fatal(dev, err, "creating sysfs entries");
+   return;
}
+
+   /* We're potentially connected now */
+   xen_update_blkif_status(be->blkif);
 }
 
 
-- 
1.8.0.1

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[PATCH] xen/blkback: prevent repeated backend_changed invocations

2012-12-11 Thread Olaf Hering 
backend_changed might be called multiple times, which will leak
be-mode. Make sure it will be called only once. Remove some unneeded
checks. Also the be-mode string was leaked, release the memory on
device shutdown.

Signed-off-by: Olaf Hering o...@aepfle.de
---

incorporate all comments from Jan.
fold the oneline change to xen_blkbk_remove into this change
now its compile tested.

 drivers/block/xen-blkback/xenbus.c | 69 ++
 1 file changed, 33 insertions(+), 36 deletions(-)

diff --git a/drivers/block/xen-blkback/xenbus.c 
b/drivers/block/xen-blkback/xenbus.c
index f58434c..5ca77c3 100644
--- a/drivers/block/xen-blkback/xenbus.c
+++ b/drivers/block/xen-blkback/xenbus.c
@@ -28,6 +28,7 @@ struct backend_info {
unsignedmajor;
unsignedminor;
char*mode;
+   unsignedalive;
 };
 
 static struct kmem_cache *xen_blkif_cachep;
@@ -366,6 +367,7 @@ static int xen_blkbk_remove(struct xenbus_device *dev)
be-blkif = NULL;
}
 
+   kfree(be-mode);
kfree(be);
dev_set_drvdata(dev-dev, NULL);
return 0;
@@ -501,10 +503,14 @@ static void backend_changed(struct xenbus_watch *watch,
= container_of(watch, struct backend_info, backend_watch);
struct xenbus_device *dev = be-dev;
int cdrom = 0;
-   char *device_type;
+   char *device_type, *p;
+   long handle;
 
DPRINTK();
 
+   if (be-alive)
+   return;
+
err = xenbus_scanf(XBT_NIL, dev-nodename, physical-device, %x:%x,
   major, minor);
if (XENBUS_EXIST_ERR(err)) {
@@ -520,12 +526,7 @@ static void backend_changed(struct xenbus_watch *watch,
return;
}
 
-   if ((be-major || be-minor) 
-   ((be-major != major) || (be-minor != minor))) {
-   pr_warn(DRV_PFX changing physical device (from %x:%x to %x:%x) 
not supported.\n,
-   be-major, be-minor, major, minor);
-   return;
-   }
+   be-alive = 1;
 
be-mode = xenbus_read(XBT_NIL, dev-nodename, mode, NULL);
if (IS_ERR(be-mode)) {
@@ -541,39 +542,35 @@ static void backend_changed(struct xenbus_watch *watch,
kfree(device_type);
}
 
-   if (be-major == 0  be-minor == 0) {
-   /* Front end dir is a number, which is used as the handle. */
-
-   char *p = strrchr(dev-otherend, '/') + 1;
-   long handle;
-   err = strict_strtoul(p, 0, handle);
-   if (err)
-   return;
-
-   be-major = major;
-   be-minor = minor;
+   /* Front end dir is a number, which is used as the handle. */
+   p = strrchr(dev-otherend, '/') + 1;
+   err = strict_strtoul(p, 0, handle);
+   if (err)
+   return;
 
-   err = xen_vbd_create(be-blkif, handle, major, minor,
-(NULL == strchr(be-mode, 'w')), cdrom);
-   if (err) {
-   be-major = 0;
-   be-minor = 0;
-   xenbus_dev_fatal(dev, err, creating vbd structure);
-   return;
-   }
+   be-major = major;
+   be-minor = minor;
 
-   err = xenvbd_sysfs_addif(dev);
-   if (err) {
-   xen_vbd_free(be-blkif-vbd);
-   be-major = 0;
-   be-minor = 0;
-   xenbus_dev_fatal(dev, err, creating sysfs entries);
-   return;
-   }
+   err = xen_vbd_create(be-blkif, handle, major, minor,
+(NULL == strchr(be-mode, 'w')), cdrom);
+   if (err) {
+   be-major = 0;
+   be-minor = 0;
+   xenbus_dev_fatal(dev, err, creating vbd structure);
+   return;
+   }
 
-   /* We're potentially connected now */
-   xen_update_blkif_status(be-blkif);
+   err = xenvbd_sysfs_addif(dev);
+   if (err) {
+   xen_vbd_free(be-blkif-vbd);
+   be-major = 0;
+   be-minor = 0;
+   xenbus_dev_fatal(dev, err, creating sysfs entries);
+   return;
}
+
+   /* We're potentially connected now */
+   xen_update_blkif_status(be-blkif);
 }
 
 
-- 
1.8.0.1

--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/