From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([208.118.235.92]:38826) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1T1Eki-0007G7-L0 for qemu-devel@nongnu.org; Tue, 14 Aug 2012 06:53:38 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1T1Ekh-0000ez-J8 for qemu-devel@nongnu.org; Tue, 14 Aug 2012 06:53:36 -0400 Received: from e23smtp09.au.ibm.com ([202.81.31.142]:39208) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1T1Ekh-0000ek-1P for qemu-devel@nongnu.org; Tue, 14 Aug 2012 06:53:35 -0400 Received: from /spool/local by e23smtp09.au.ibm.com with IBM ESMTP SMTP Gateway: Authorized Use Only! Violators will be prosecuted for from ; Tue, 14 Aug 2012 20:52:39 +1000 Received: from d23av04.au.ibm.com (d23av04.au.ibm.com [9.190.235.139]) by d23relay04.au.ibm.com (8.13.8/8.13.8/NCO v10.0) with ESMTP id q7EAiujA8323162 for ; Tue, 14 Aug 2012 20:44:56 +1000 Received: from d23av04.au.ibm.com (loopback [127.0.0.1]) by d23av04.au.ibm.com (8.14.4/8.13.1/NCO v10.0 AVout) with ESMTP id q7EArShE002645 for ; Tue, 14 Aug 2012 20:53:28 +1000 Message-ID: <502A2E1F.8010509@linux.vnet.ibm.com> Date: Tue, 14 Aug 2012 16:23:19 +0530 From: Supriya Kannery MIME-Version: 1.0 References: <20120730213409.21536.7589.sendpatchset@skannery.in.ibm.com> <20120730213437.21536.87232.sendpatchset@skannery.in.ibm.com> <50181314.1040909@redhat.com> In-Reply-To: <50181314.1040909@redhat.com> Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [v2 Patch 2/9]block: raw-posix image file reopen Reply-To: supriyak@linux.vnet.ibm.com List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Eric Blake Cc: Kevin Wolf , Shrinidhi Joshi , Stefan Hajnoczi , Jeff Cody , qemu-devel@nongnu.org, Luiz Capitulino , Christoph Hellwig On 07/31/2012 10:47 PM, Eric Blake wrote: > On 07/30/2012 03:34 PM, Supriya Kannery wrote: >> + s->fd = dup3(raw_rs->stash_s->fd, s->fd, O_CLOEXEC); > > You called it out in your cover letter, but just pointing out that this > is one of the locations that needs a conditional fallback to > dup2/qemu_set_cloexec if dup3 and O_CLOEXEC are missing. > > More importantly, though, you want this to be fcntl(F_DUP_CLOEXEC) and > NOT dup3, so that you are duplicating to the first available fd rather > than accidentally overwriting whatever s->fd happened to be on entry. > Also, where is your error checking that you didn't just assign s->fd to > -1? It looks like your goal here is to stash a copy of the fd, so that > on failure you can then pivot over to your copy. > Will use fcntl(F_DUP_CLOEXEC) in updated patch. >> + >> + *prs =&(raw_rs->reopen_state); >> + >> + /* Flags that can be set using fcntl */ >> + int fcntl_flags = BDRV_O_NOCACHE; > > This variable name is misleading; fcntl_flags in my mind implies O_* not > BDRV_O_*, as we are not guaranteed that they are the same values. Maybe > bdrv_flags is a better name. Or are you trying to list the subset of > BDRV_O flags that can be changed via mapping to the appropriate O_ flag > during fcntl? > From the list of flags in bdrv->openflags (BDRV_O*), only few of them can be changed using fcntl. So to identify those allowed subset, I am using fcntl_flags. Excerpt from man fcntl for F_SETFL: On Linux this command can only change the O_APPEND, O_ASYNC, O_DIRECT, O_NOATIME, and O_NONBLOCK flags. May be naming it fcntl_supported_flags would be better? - thanks, Supriya