Re: [PATCH v2] libxenstore: prefer using the character device

[David Vrabel <david.vrabel@citrix.com>]

On 27/08/15 19:03, Ian Jackson wrote:
> Wei Liu writes ("Re: [Xen-devel] [PATCH v2] libxenstore: prefer using the character device"):
>> On Thu, Aug 27, 2015 at 09:04:38AM -0500, Jonathan Creekmore wrote:
>>> With the addition of FMODE_ATOMIC_POS in the Linux 3.14 kernel,
>>> concurrent blocking file accesses to a single open file descriptor can
>>> cause a deadlock trying to grab the file position lock. If a watch has
>>> been set up, causing a read_thread to blocking read on the file
>>> descriptor, then future writes that would cause the background read to
>>> complete will block waiting on the file position lock before they can
>>> execute. This race condition only occurs when libxenstore is accessing
>>> the xenstore daemon through the /proc/xen/xenbus file and not through
>>> the unix domain socket, which is the case when the xenstore daemon is
>>> running as a stub domain or when oxenstored is passed
>>> --disable-socket. Accessing the daemon from the true character device
>>> also does not exhibit this problem.
>>>
>>> On Linux, prefer using the character device file over the proc file if
>>> the character device exists.
> 
> I confess I still see this as working around a kernel bug.  Only this
> time we are switching from a buggy to non-buggy kernel interface.

/proc/xen/xenbus is deprecated.  The tools should use the non-deprecated
interface.

> Why don't we have the kernel provide only non-buggy interfaces ?

Fixing /proc/xen/xenbus is non-trival and since there's a fully working
non-deprecated interface (/dev/xen/xenbus), it's unlikely that anyone is
going to be inspired to fix it.

>>> diff --git a/tools/xenstore/xs_lib.c b/tools/xenstore/xs_lib.c
>>> index af4f75a..0c7744e 100644
>>> --- a/tools/xenstore/xs_lib.c
>>> +++ b/tools/xenstore/xs_lib.c
>>> @@ -81,6 +81,8 @@ const char *xs_domain_dev(void)
>>>  #if defined(__RUMPUSER_XEN__) || defined(__RUMPRUN__)
>>>  	return "/dev/xen/xenbus";
>>>  #elif defined(__linux__)
>>> +	if (access("/dev/xen/xenbus", F_OK) == 0)
>>> +		return "/dev/xen/xenbus";
> 
> Also, previously xs_domain_dev was a function which simply returned a
> static value.  I feel vaguely uneasy at putting this kind of
> autodetection logic here.

"Vaguely uneasy"?  Are we engineers or witchdoctors?

xs_domain_dev() already does a system call to query the environment so
it did not just "return a static value":

const char *xs_domain_dev(void)
{
        char *s = getenv("XENSTORED_PATH");
        if (s)
                return s;
        ...

David