From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([208.118.235.92]:48658) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Sfb5D-0004Ff-8u for qemu-devel@nongnu.org; Fri, 15 Jun 2012 14:17:20 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1Sfb5B-0005co-5B for qemu-devel@nongnu.org; Fri, 15 Jun 2012 14:17:18 -0400 Received: from e32.co.us.ibm.com ([32.97.110.150]:48338) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Sfb5A-0005bs-Um for qemu-devel@nongnu.org; Fri, 15 Jun 2012 14:17:17 -0400 Received: from /spool/local by e32.co.us.ibm.com with IBM ESMTP SMTP Gateway: Authorized Use Only! Violators will be prosecuted for from ; Fri, 15 Jun 2012 12:17:11 -0600 Received: from d01relay03.pok.ibm.com (d01relay03.pok.ibm.com [9.56.227.235]) by d01dlp03.pok.ibm.com (Postfix) with ESMTP id CA3E9C9006F for ; Fri, 15 Jun 2012 14:17:00 -0400 (EDT) Received: from d01av04.pok.ibm.com (d01av04.pok.ibm.com [9.56.224.64]) by d01relay03.pok.ibm.com (8.13.8/8.13.8/NCO v10.0) with ESMTP id q5FIH1Z3188940 for ; Fri, 15 Jun 2012 14:17:01 -0400 Received: from d01av04.pok.ibm.com (loopback [127.0.0.1]) by d01av04.pok.ibm.com (8.14.4/8.13.1/NCO v10.0 AVout) with ESMTP id q5FIH1Fq022873 for ; Fri, 15 Jun 2012 14:17:01 -0400 Message-ID: <4FDB7C1B.8020802@linux.vnet.ibm.com> Date: Fri, 15 Jun 2012 14:16:59 -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> In-Reply-To: <4FDB51E8.8060406@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: Eric Blake Cc: kwolf@redhat.com, aliguori@us.ibm.com, stefanha@linux.vnet.ibm.com, libvir-list@redhat.com, qemu-devel@nongnu.org, lcapitulino@redhat.com, pbonzini@redhat.com 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. > > if (flags & O_TRUNCATE || > ((flags & (O_CREAT|O_EXCL)) == (O_CREAT|O_EXCL)) > ftruncate(ret, 0); > > Why do I truncate on O_CREAT|O_EXCL? Because that's a case where open() > guarantees that the file will have just been created empty. > That makes sense. > if (flags & O_CLOEXEC) > use fcntl(F_DUPFD_CLOEXEC) instead of dup(), if possible > else fallback to fcntl(F_GETFD/F_SETFD) to set FD_CLOEXEC non-atomically > That makes sense too. > 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(). > > 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? > > Treat O_APPEND similarly to O_NONBLOCK (with fcntl(F_GETFL/F_SETFL) to > match the desired open() setting). > Ok -- Regards, Corey