From mboxrd@z Thu Jan  1 00:00:00 1970
From: "Jan Beulich" <JBeulich@novell.com>
Subject: Re: [PATCH] Fix blkback/blktap sysfs read bug.
Date: Wed, 20 Jan 2010 07:46:19 +0000
Message-ID: <4B56C2DB020000780002AE45@vpn.id2.novell.com>
References: <20100119095250.GB20691@joejin-pc.cn.oracle.com>
	<4B5596B2020000780002AAF1@vpn.id2.novell.com>
	<20100119113224.GA21348@joejin-pc.cn.oracle.com>
	<4B55AE58020000780002AB39@vpn.id2.novell.com>
	<20100119141338.GA22249@joejin-pc.cn.oracle.com>
	<4B55E9E7020000780002AC17@vpn.id2.novell.com>
	<20100120020605.GA25697@joejin-pc.cn.oracle.com>
Mime-Version: 1.0
Content-Type: text/plain; charset=US-ASCII
Content-Transfer-Encoding: quoted-printable
Return-path: <xen-devel-bounces@lists.xensource.com>
In-Reply-To: <20100120020605.GA25697@joejin-pc.cn.oracle.com>
Content-Disposition: inline
List-Unsubscribe: <http://lists.xensource.com/mailman/listinfo/xen-devel>,
	<mailto:xen-devel-request@lists.xensource.com?subject=unsubscribe>
List-Post: <mailto:xen-devel@lists.xensource.com>
List-Help: <mailto:xen-devel-request@lists.xensource.com?subject=help>
List-Subscribe: <http://lists.xensource.com/mailman/listinfo/xen-devel>,
	<mailto:xen-devel-request@lists.xensource.com?subject=subscribe>
Sender: xen-devel-bounces@lists.xensource.com
Errors-To: xen-devel-bounces@lists.xensource.com
To: Joe Jin <joe.jin@oracle.com>
Cc: Keir Fraser <keir.fraser@citrix.com>, xen-devel@lists.xensource.com, deepak.patel@oracle.com, greg.marsden@oracle.com
List-Id: xen-devel@lists.xenproject.org

>>> "Joe Jin" <joe.jin@oracle.com> 20.01.10 03:06 >>>
>sysfs did not provide lock to handle this, not sure if developer
>think it is not necessary or they'd like to caller to handled it.

A lock is probably not the usual way to deal with this; ref-counting
would seem more common. Nevertheless I think adding a lock will
take care of the issue here.

>--- a/drivers/xen/blktap/xenbus.c	Fri Jan 08 13:07:17 2010 +0000
>+++ b/drivers/xen/blktap/xenbus.c	Wed Jan 20 10:00:53 2010 +0800
>...
>@@ -122,10 +123,15 @@
> 				   struct device_attribute *attr,	\
> 				   char *buf)				\
> 	{								\
>+		ssize_t ret =3D -ENODEV;					=
\
> 		struct xenbus_device *dev =3D to_xenbus_device(_dev);	\

The use of to_xenbus_device() here makes ...

>-		struct backend_info *be =3D dev->dev.driver_data;		=
\
>+		struct backend_info *be;				\
> 									\
>-		return sprintf(buf, format, ##args);			\
>+		read_lock(&sysfs_read_lock);				\
>+		if (dev && (be =3D dev->dev.driver_data) && be->blkif)	\

... the checking of dev here useless (and the blkback part of the patch
doesn't do the same).

>+			ret =3D sprintf(buf, format, ##args);		\
>+		read_unlock(&sysfs_read_lock);				\
>+		return ret;						\
> 	}								\
> 	static DEVICE_ATTR(name, S_IRUGO, show_##name, NULL)
>=20

And btw., in both cases with the lock added there's no need to check
both 'be' and 'be->blkif', since be->blkif can't be NULL when be is
non-NULL.

Jan