From mboxrd@z Thu Jan 1 00:00:00 1970 From: David Vrabel Subject: Re: [PATCH] libxl: use libxl_fd_set_{cloexec, nonblock} helpers Date: Mon, 28 Jul 2014 15:51:14 +0100 Message-ID: <53D66362.8000901@citrix.com> References: <53D28D9F020000780002615D@mail.emea.novell.com> <21458.36567.506481.423244@mariner.uk.xensource.com> <53D60AB602000078000265E2@mail.emea.novell.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: Received: from mail6.bemta3.messagelabs.com ([195.245.230.39]) by lists.xen.org with esmtp (Exim 4.72) (envelope-from ) id 1XBmGl-0000HA-MR for xen-devel@lists.xenproject.org; Mon, 28 Jul 2014 14:51:19 +0000 In-Reply-To: <53D60AB602000078000265E2@mail.emea.novell.com> List-Unsubscribe: , List-Post: List-Help: List-Subscribe: , Sender: xen-devel-bounces@lists.xen.org Errors-To: xen-devel-bounces@lists.xen.org To: Jan Beulich , Ian Campbell , Ian Jackson Cc: xen-devel , Stefano Stabellini List-Id: xen-devel@lists.xenproject.org On 28/07/14 07:32, Jan Beulich wrote: >>>> On 25.07.14 at 19:07, wrote: >> Jan Beulich writes ("[PATCH] libxl: use libxl_fd_set_{cloexec,nonblock} >>> --- a/tools/libxl/libxl_qmp.c >>> +++ b/tools/libxl/libxl_qmp.c >>> @@ -358,19 +358,14 @@ static int qmp_open(libxl__qmp_handler * >>> int timeout) >> ... >>> qmp->qmp_fd = socket(AF_UNIX, SOCK_STREAM, 0); >> ... >>> + ret = libxl_fd_set_nonblock(qmp->ctx, qmp->qmp_fd, 1); >>> + if (ret) return -1; >> >> This change is correct. >> >>> ret = libxl_fd_set_cloexec(qmp->ctx, qmp->qmp_fd, 1); >>> if (ret) return -1; >> >> But this (which you haven't touched) needs to be done with >> libxl__carefd* instead. (Your patch is not unacceptable due to you >> not making this change, on the basis that we should accept >> improvements even if they don't leave the code perfect.) > > And indeed I'd like to leave that conversion to you or someone > else more familiar with the libxl concepts. > >>> --- a/tools/libxl/libxl_utils.c >>> +++ b/tools/libxl/libxl_utils.c >>> @@ -1047,11 +1047,17 @@ int libxl__random_bytes(libxl__gc *gc, u >> ... >>> - fd = open(dev, O_RDONLY | O_CLOEXEC); >>> + fd = open(dev, O_RDONLY); >> >> Firstly, I don't understand why we care about cloexec on this fd, but >> it's harmless to do so. > > And of course I'd be perfectly fine with a one liner just dropping it > (all I really care about is that the file builds correctly in all cases). > The question of why cloexec is needed/wanted here I'll pass on to > author and committer, but I'd guess this is just to avoid > proliferation of file handles into clones. Surely it should be standard practice for a library to always set CLOEXEC on any files it opens, to minimize the side effects the library call has? And if CLOEXEC is required then setting it at open time is the only safe way. David