From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([208.118.235.92]:46012) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1T9zZc-0002Fb-M6 for qemu-devel@nongnu.org; Fri, 07 Sep 2012 10:30:22 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1T9zZS-0006IR-Fn for qemu-devel@nongnu.org; Fri, 07 Sep 2012 10:30:20 -0400 Received: from e39.co.us.ibm.com ([32.97.110.160]:38319) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1T9zZS-000633-96 for qemu-devel@nongnu.org; Fri, 07 Sep 2012 10:30:10 -0400 Received: from /spool/local by e39.co.us.ibm.com with IBM ESMTP SMTP Gateway: Authorized Use Only! Violators will be prosecuted for from ; Fri, 7 Sep 2012 08:30:00 -0600 Received: from d03relay03.boulder.ibm.com (d03relay03.boulder.ibm.com [9.17.195.228]) by d03dlp01.boulder.ibm.com (Postfix) with ESMTP id 0A5B41FF003E for ; Fri, 7 Sep 2012 08:29:56 -0600 (MDT) Received: from d03av02.boulder.ibm.com (d03av02.boulder.ibm.com [9.17.195.168]) by d03relay03.boulder.ibm.com (8.13.8/8.13.8/NCO v10.0) with ESMTP id q87ETkt3015734 for ; Fri, 7 Sep 2012 08:29:49 -0600 Received: from d03av02.boulder.ibm.com (loopback [127.0.0.1]) by d03av02.boulder.ibm.com (8.14.4/8.13.1/NCO v10.0 AVout) with ESMTP id q87ETj45017894 for ; Fri, 7 Sep 2012 08:29:45 -0600 Message-ID: <504A04D6.9060000@linux.vnet.ibm.com> Date: Fri, 07 Sep 2012 10:29:42 -0400 From: Corey Bryant MIME-Version: 1.0 References: <7ad1da59f3ef71e4c99c083a0675508ca9279c53.1346352124.git.jcody@redhat.com> <50477020.7050903@redhat.com> <50478124.8030801@redhat.com> <50486B7C.2000406@redhat.com> <5048C268.402@linux.vnet.ibm.com> <5049CF2F.4050603@redhat.com> In-Reply-To: <5049CF2F.4050603@redhat.com> Content-Type: text/plain; charset=ISO-8859-15; format=flowed Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCH 3/7] block: raw-posix image file reopen List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Kevin Wolf Cc: supriyak@linux.vnet.ibm.com, stefanha@gmail.com, jcody@redhat.com, qemu-devel@nongnu.org, pbonzini@redhat.com, eblake@redhat.com On 09/07/2012 06:40 AM, Kevin Wolf wrote: > Am 06.09.2012 17:34, schrieb Corey Bryant: >> >> >> On 09/06/2012 05:23 AM, Kevin Wolf wrote: >>> Am 05.09.2012 18:43, schrieb Jeff Cody: >>>>>> + } >>>>>> + >>>>>> + int fcntl_flags = O_APPEND | O_ASYNC | O_NONBLOCK; >>>>>> +#ifdef O_NOATIME >>>>>> + fcntl_flags |= O_NOATIME; >>>>>> +#endif >>>>>> + if ((raw_s->open_flags & ~fcntl_flags) == (s->open_flags & ~fcntl_flags)) { >>>>>> + /* dup the original fd */ >>>>>> + /* TODO: use qemu fcntl wrapper */ >>>>>> + raw_s->fd = fcntl(s->fd, F_DUPFD_CLOEXEC, 0); >>>>>> + if (raw_s->fd == -1) { >>>>>> + ret = -1; >>>>>> + goto error; >>>>>> + } >>>>>> + ret = fcntl_setfl(raw_s->fd, raw_s->open_flags); >>>>>> + } else { >>>>>> + raw_s->fd = qemu_open(state->bs->filename, raw_s->open_flags, 0644); >>>>>> + if (raw_s->fd == -1) { >>>>>> + ret = -1; >>>>>> + } >>>>> >>>>> Ignoring this part for now, with qemu_dup_flags() it's going to look a >>>>> bit different. In particular, I'm hoping that we don't get a second >>>>> fcntl_flags enumeration here, but can just fall back to qemu_open() >>>>> whenever qemu_dup_flags() fails. >>>> >>>> That will require modification to qemu_dup_flags()... I believe >>>> qemu_dup_flags() silently filters out fcntl incompatible flags. >>>> >>>> Maybe it would be best to create a small helper function in osdep.c, that >>>> fetches the fcntl_flags. Then qemu_dup_flags() and this function would >>>> use the same helper to fetch fcntl_flags. The results of that would >>>> determine if we call qemu_dup_flags() or qemu_open(). >>>> >>>> Although, I do think it makes sense to always try qemu_open() if >>>> qemu_dup_flags() fails for some reason. >> >> I'm curious why you can't always call qemu_open(). > > I believe the original reason was that qemu_open() is more likely to > fail, for example if the image file has been renamed/moved/deleted since > the first open. You could still use fcntl() on an existing file > descriptor, but reopening would fail. > >> Some things to consider so that fd passing doesn't break when a reopen >> occurs. Mainly all the concerns revolve around how fd passing keeps >> track of references to fd sets (note: adding and removing fd set >> references is all done in qemu_open and qemu_close). >> >> * When reopening, qemu_open needs to be called before qemu_close. This >> will prevent the reference list for an fdset from becoming empty. If >> qemu_close is called before qemu_open, the reference list can become >> empty, and the fdset could be cleaned up before the qemu_open. Then >> qemu_open would fail. > > Will automatically be right when we properly implement transactional > semantics. > >> * qemu_open/qemu_close need to be used rather than open/close so that >> the references for fd passing are properly accounted for. > > Congratulations, you've just discovered a bug in Jeff's patches. It was > a good idea to CC you. ;-) > >> * I don't think you want to call qemu_dup_flags directly since it >> doesn't update the reference list for fd passing. Only qemu_open and >> qemu_close update the reference list. > > That's a good point, too. So probably a small wrapper that just updates > the reference list in addition? > You could do that. And yes you'd have to add code to add the new dup fd to an fdset's dup_fds list if in fact the fd that you dup'd was a member of an fdset's dup_fds list (see how qemu_close() and qemu_open() do this). But wouldn't it be easier to just go through qemu_close() then qemu_open() to perform the reopen? Then you don't have to add this extra code to account for fd passing. -- Regards, Corey >>> If we can modify qemu_dup_flags() to fail if it can't provide the right >>> set of flags, then I think we should do it - and I think we can. Even >>> for the existing cases with fd passing it shouldn't break anything, but >>> only add an additional safety check. >>> >>> And if touching the function motivates Corey to write some fd passing >>> test cases so that you can't break it, even better. ;-) >> >> :) Sorry, I do plan to do this soon. I've just been side-tracked with >> some other things. > > No problem, it was just such a great opportunity to remind you. ;-) > > Kevin >