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

[Jonathan Creekmore <jonathan.creekmore@gmail.com>]

Just wanted to follow-up and see if there was any more debate on 
this, since I hadn't seen any other commentary since last week.

David Vrabel <david.vrabel@citrix.com> writes:

> 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