From mboxrd@z Thu Jan 1 00:00:00 1970 From: Jonathan Creekmore Subject: Re: [PATCH v2] libxenstore: prefer using the character device Date: Mon, 31 Aug 2015 13:59:24 -0500 Message-ID: References: <55D2FF72.7050006@citrix.com> <1440684278-16838-1-git-send-email-jonathan.creekmore@gmail.com> <20150827165643.GU22586@zion.uk.xensource.com> <21983.20700.957792.401545@mariner.uk.xensource.com> <55E03089.7080009@citrix.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii"; Format="flowed" Content-Transfer-Encoding: 7bit Return-path: Received: from mail6.bemta14.messagelabs.com ([193.109.254.103]) by lists.xen.org with esmtp (Exim 4.72) (envelope-from ) id 1ZWUIk-0007xZ-7N for xen-devel@lists.xenproject.org; Mon, 31 Aug 2015 18:59:30 +0000 Received: by qgeb6 with SMTP id b6so71631248qge.3 for ; Mon, 31 Aug 2015 11:59:27 -0700 (PDT) In-Reply-To: <55E03089.7080009@citrix.com> (David Vrabel's message of "Fri, 28 Aug 2015 10:57:29 +0100") List-Unsubscribe: , List-Post: List-Help: List-Subscribe: , Sender: xen-devel-bounces@lists.xen.org Errors-To: xen-devel-bounces@lists.xen.org To: David Vrabel Cc: Wei Liu , xen-devel@lists.xenproject.org, Ian Jackson , ian.campbell@citrix.com, stefano.stabellini@eu.citrix.com List-Id: xen-devel@lists.xenproject.org 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 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