From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([208.118.235.92]:54831) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Sfc3N-0002jg-Q7 for qemu-devel@nongnu.org; Fri, 15 Jun 2012 15:19:31 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1Sfc3L-0005jm-Qn for qemu-devel@nongnu.org; Fri, 15 Jun 2012 15:19:29 -0400 Received: from e31.co.us.ibm.com ([32.97.110.149]:49033) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Sfc3L-0005iT-Kv for qemu-devel@nongnu.org; Fri, 15 Jun 2012 15:19:27 -0400 Received: from /spool/local by e31.co.us.ibm.com with IBM ESMTP SMTP Gateway: Authorized Use Only! Violators will be prosecuted for from ; Fri, 15 Jun 2012 13:19:23 -0600 Received: from d03relay03.boulder.ibm.com (d03relay03.boulder.ibm.com [9.17.195.228]) by d03dlp02.boulder.ibm.com (Postfix) with ESMTP id 642FF3E40054 for ; Fri, 15 Jun 2012 19:19:19 +0000 (WET) Received: from d03av01.boulder.ibm.com (d03av01.boulder.ibm.com [9.17.195.167]) by d03relay03.boulder.ibm.com (8.13.8/8.13.8/NCO v10.0) with ESMTP id q5FJJCP3044242 for ; Fri, 15 Jun 2012 13:19:15 -0600 Received: from d03av01.boulder.ibm.com (loopback [127.0.0.1]) by d03av01.boulder.ibm.com (8.14.4/8.13.1/NCO v10.0 AVout) with ESMTP id q5FJJBgU004811 for ; Fri, 15 Jun 2012 13:19:11 -0600 Message-ID: <4FDB8AAC.3030002@linux.vnet.ibm.com> Date: Fri, 15 Jun 2012 15:19:08 -0400 From: Corey Bryant MIME-Version: 1.0 References: <1339689305-27031-1-git-send-email-coreyb@linux.vnet.ibm.com> <1339689305-27031-4-git-send-email-coreyb@linux.vnet.ibm.com> <4FDB51E8.8060406@redhat.com> <4FDB7C1B.8020802@linux.vnet.ibm.com> <4FDB82F9.8020708@redhat.com> In-Reply-To: <4FDB82F9.8020708@redhat.com> Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCH v3 3/5] osdep: Enable qemu_open to dup pre-opened fd List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Kevin Wolf Cc: aliguori@us.ibm.com, stefanha@linux.vnet.ibm.com, libvir-list@redhat.com, qemu-devel@nongnu.org, lcapitulino@redhat.com, pbonzini@redhat.com, Eric Blake On 06/15/2012 02:46 PM, Kevin Wolf wrote: > Am 15.06.2012 20:16, schrieb Corey Bryant: >> >> >> On 06/15/2012 11:16 AM, Eric Blake wrote: >>> On 06/14/2012 09:55 AM, Corey Bryant wrote: >>>> This patch adds support to qemu_open to dup(fd) a pre-opened file >>>> descriptor if the filename is of the format /dev/fd/X. >>>> >>> >>>> +++ b/osdep.c >>>> @@ -82,6 +82,19 @@ int qemu_open(const char *name, int flags, ...) >>>> int ret; >>>> int mode = 0; >>>> >>>> +#ifndef _WIN32 >>>> + const char *p; >>>> + >>>> + /* Attempt dup of fd for pre-opened file */ >>>> + if (strstart(name, "/dev/fd/", &p)) { >>>> + ret = qemu_parse_fd(p); >>>> + if (ret == -1) { >>>> + return -1; >>>> + } >>>> + return dup(ret); >>> >>> I think you need to honor flags so that the end use of the fd will be as >>> if qemu had directly opened the file, rather than just doing a blind dup >>> with a resulting fd that is in a different state than the caller >>> expected. I can think of at least the following cases (there may be more): >> >> I was thinking libvirt would handle all the flag settings on open >> (obviously since that's how I coded it). I think you're right with this >> approach though as QEMU will re-open the same file various times with >> different flags. >> >> There are some flags that I don't think we'll be able to change. For >> example: O_RDONLY, O_WRONLY, O_RDWR. I assume libvirt would open all >> files O_RDWR. > > I think we need to check all of them and fail qemu_open() if they don't > match. Those that qemu can change, should be just changed, of course. > Ok. I remember a scenario where QEMU opens a file read-only (perhaps to check headers and determine the file format) before re-opening it read-write. Perhaps this is only when format= isn't specified with -drive. I'm thinking we may need to change flags to read-write where they used to be read-only, in some circumstances. >>> Oh, and are we using MSG_CMSG_CLOEXEC where possible (and where not >>> possible, falling back to fcntl(F_GETFD/F_SETFD) to set FD_CLOEXEC) on >>> all fds received by 'getfd' and 'pass-fd'? I can't think of any reason >>> why 'migrate fd:name' would need to be inheritable, and in the case of >>> /dev/fd/ parsing, while the dup() result may need to be inheritable, the >>> original that we are dup'ing from should most certainly be cloexec. >> >> It doesn't look like we use MSG_CMSG_CLOEXEC anywhere in QEMU. I don't >> think we can modify getfd at this point (compatibility) but we could >> update pass-fd to set it. It may make more sense to set it with >> fcntl(FD_CLOEXEC) in qmp_pass_fd(). > > In which scenario would any client break if we set FD_CLOEXEC? I don't > think compatibility means we can't fix any bugs. > I don't know if it breaks any client. Maybe it's not a compatibility error. It dopes change behavior down the line though. If you think it's ok to set FD_CLOEXEC for getfd too, then I'm happy to do it. >>> if (flags & O_NONBLOCK) >>> use fcntl(F_GETFL/F_SETFL) to set O_NONBLOCK >>> else >>> use fcntl(F_GETFL/F_SETFL) to clear O_NONBLOCK >>> >>> or maybe we document that callers of pass-fd must always pass fds with >>> O_NONBLOCK clear instead of clearing it ourselves. Or maybe we make >>> sure part of the process of tying name with fd in the lookup list of >>> named fds is determining the current O_NONBLOCK state in case future >>> qemu_open() need it in the opposite state. >> >> Just documenting it seems error-prone. Why not just set/clear it based >> on the flag passed to qemu_open? > > I agree. We could just check and return an error if they aren't set > correctly, but I think adjusting the flags is nicer. > > Kevin > Ok thanks for the input! -- Regards, Corey