From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([208.118.235.92]:34707) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1T9e6Z-0001q1-G5 for qemu-devel@nongnu.org; Thu, 06 Sep 2012 11:35:06 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1T9e6U-0001mR-Cx for qemu-devel@nongnu.org; Thu, 06 Sep 2012 11:34:55 -0400 Received: from e3.ny.us.ibm.com ([32.97.182.143]:36703) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1T9e6U-0001ll-9I for qemu-devel@nongnu.org; Thu, 06 Sep 2012 11:34:50 -0400 Received: from /spool/local by e3.ny.us.ibm.com with IBM ESMTP SMTP Gateway: Authorized Use Only! Violators will be prosecuted for from ; Thu, 6 Sep 2012 11:34:44 -0400 Received: from d01relay05.pok.ibm.com (d01relay05.pok.ibm.com [9.56.227.237]) by d01dlp01.pok.ibm.com (Postfix) with ESMTP id DD94A38C8039 for ; Thu, 6 Sep 2012 11:34:39 -0400 (EDT) Received: from d03av06.boulder.ibm.com (d03av06.boulder.ibm.com [9.17.195.245]) by d01relay05.pok.ibm.com (8.13.8/8.13.8/NCO v10.0) with ESMTP id q86FYcgl080340 for ; Thu, 6 Sep 2012 11:34:38 -0400 Received: from d03av06.boulder.ibm.com (loopback [127.0.0.1]) by d03av06.boulder.ibm.com (8.14.4/8.13.1/NCO v10.0 AVout) with ESMTP id q86FZUvX002128 for ; Thu, 6 Sep 2012 09:35:31 -0600 Message-ID: <5048C268.402@linux.vnet.ibm.com> Date: Thu, 06 Sep 2012 11:34:00 -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> In-Reply-To: <50486B7C.2000406@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/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(). 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. * qemu_open/qemu_close need to be used rather than open/close so that the references for fd passing are properly accounted for. * 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. > > 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. -- Regards, Corey