From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([208.118.235.92]:43851) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1TE69S-0004ux-VD for qemu-devel@nongnu.org; Tue, 18 Sep 2012 18:20:20 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1TE69R-0004wm-PJ for qemu-devel@nongnu.org; Tue, 18 Sep 2012 18:20:18 -0400 Received: from mx1.redhat.com ([209.132.183.28]:61689) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1TE69R-0004vQ-FO for qemu-devel@nongnu.org; Tue, 18 Sep 2012 18:20:17 -0400 Message-ID: <5058F395.7020807@redhat.com> Date: Tue, 18 Sep 2012 18:20:05 -0400 From: Jeff Cody MIME-Version: 1.0 References: <4feab4ab212834c55707c0690d8efcb4f3662b1c.1347993885.git.jcody@redhat.com> <5058E5A4.5000704@redhat.com> In-Reply-To: <5058E5A4.5000704@redhat.com> Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCH v3 09/19] block: raw-posix image file reopen Reply-To: jcody@redhat.com List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Eric Blake Cc: kwolf@redhat.com, pbonzini@redhat.com, stefanha@gmail.com, qemu-devel@nongnu.org, supriyak@linux.vnet.ibm.com On 09/18/2012 05:20 PM, Eric Blake wrote: > On 09/18/2012 12:53 PM, Jeff Cody wrote: >> This is derived from the Supriya Kannery's reopen patches. >> >> This contains the raw-posix driver changes for the bdrv_reopen_* >> functions. All changes are staged into a temporary scratch buffer >> during the prepare() stage, and copied over to the live structure >> during commit(). Upon abort(), all changes are abandoned, and the >> live structures are unmodified. >> >> The _prepare() will create an extra fd - either by means of a dup, >> if possible, or opening a new fd if not (for instance, access >> control changes). Upon _commit(), the original fd is closed and >> the new fd is used. Upon _abort(), the duplicate/new fd is closed. >> >> Signed-off-by: Jeff Cody >> --- >> block/raw-posix.c | 99 +++++++++++++++++++++++++++++++++++++++++++++++++++++++ >> 1 file changed, 99 insertions(+) >> >> diff --git a/block/raw-posix.c b/block/raw-posix.c >> index 6bf5480..edd1eca 100644 >> --- a/block/raw-posix.c >> +++ b/block/raw-posix.c >> @@ -138,6 +138,15 @@ typedef struct BDRVRawState { >> #endif >> } BDRVRawState; >> >> +typedef struct BDRVRawReopenState { >> + int fd; >> + int open_flags; >> +#ifdef CONFIG_LINUX_AIO >> + int use_aio; >> + void *aio_ctx; >> +#endif > > These members are conditional... > >> +static int raw_reopen_prepare(BDRVReopenState *state, >> + BlockReopenQueue *queue, Error **errp) >> +{ >> + BDRVRawState *s; >> + BDRVRawReopenState *raw_s; >> + int ret = 0; >> + >> + assert(state != NULL); >> + assert(state->bs != NULL); >> + >> + s = state->bs->opaque; >> + >> + state->opaque = g_malloc0(sizeof(BDRVRawReopenState)); >> + raw_s = state->opaque; >> + raw_s->use_aio = s->use_aio; >> + raw_s->aio_ctx = s->aio_ctx; > > ...but you are unconditionally assigning into them. This will introduce > compile failures on other platforms. Argh. Thanks. > >> + 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); > > F_DUPFD_CLOEXEC is not defined everywhere yet; you still need to address > your TODO. > >> + >> + /* If we cannot use fctnl, or fcntl failed, fall back to qemu_open() */ > > s/fctnl/fcntl/ > >> +static void raw_reopen_commit(BDRVReopenState *state) >> +{ >> + BDRVRawReopenState *raw_s = state->opaque; >> + BDRVRawState *s = state->bs->opaque; >> + >> + s->open_flags = raw_s->open_flags; >> + >> + qemu_close(s->fd); >> + s->fd = raw_s->fd; >> + s->use_aio = raw_s->use_aio; >> + s->aio_ctx = raw_s->aio_ctx; > > Again, more unconditional use of conditional members. >