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

[Jonathan Creekmore <jonathan.creekmore@xxxxxxxxx>]

Ian Jackson <Ian.Jackson@xxxxxxxxxxxxx> writes:

> 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. 
>
> Why don't we have the kernel provide only non-buggy interfaces ?
I was just trying to implement what David suggested; CC'ing him on 
this as well.
> > > 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. 
Not entirely; the existing code queried an environment variable first
and, only if that was not set, did it return a static value. I added the
autodetection logic to fall back in the case where, for some reason,
/dev/xen/xenbus did not exist. Handling that kind of a fallback at a
higher layer would be a larger refactor to probe for the existence of
the character device first.

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel