From mboxrd@z Thu Jan  1 00:00:00 1970
Received: from eggs.gnu.org ([208.118.235.92]:48319)
	by lists.gnu.org with esmtp (Exim 4.71)
	(envelope-from <kwolf@redhat.com>) id 1TEhSr-0003t6-2Z
	for qemu-devel@nongnu.org; Thu, 20 Sep 2012 10:10:54 -0400
Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71)
	(envelope-from <kwolf@redhat.com>) id 1TEhSl-0002ri-1i
	for qemu-devel@nongnu.org; Thu, 20 Sep 2012 10:10:49 -0400
Received: from mx1.redhat.com ([209.132.183.28]:13215)
	by eggs.gnu.org with esmtp (Exim 4.71)
	(envelope-from <kwolf@redhat.com>) id 1TEhSk-0002rb-Fo
	for qemu-devel@nongnu.org; Thu, 20 Sep 2012 10:10:42 -0400
Message-ID: <505B23DC.3090702@redhat.com>
Date: Thu, 20 Sep 2012 16:10:36 +0200
From: Kevin Wolf <kwolf@redhat.com>
MIME-Version: 1.0
References: <cover.1347993885.git.jcody@redhat.com>
	<4feab4ab212834c55707c0690d8efcb4f3662b1c.1347993885.git.jcody@redhat.com>
In-Reply-To: <4feab4ab212834c55707c0690d8efcb4f3662b1c.1347993885.git.jcody@redhat.com>
Content-Type: text/plain; charset=ISO-8859-15
Content-Transfer-Encoding: 7bit
Subject: Re: [Qemu-devel] [PATCH v3 09/19] block: raw-posix image file reopen
List-Id: <qemu-devel.nongnu.org>
List-Unsubscribe: <https://lists.nongnu.org/mailman/options/qemu-devel>,
	<mailto:qemu-devel-request@nongnu.org?subject=unsubscribe>
List-Archive: <http://lists.nongnu.org/archive/html/qemu-devel>
List-Post: <mailto:qemu-devel@nongnu.org>
List-Help: <mailto:qemu-devel-request@nongnu.org?subject=help>
List-Subscribe: <https://lists.nongnu.org/mailman/listinfo/qemu-devel>,
	<mailto:qemu-devel-request@nongnu.org?subject=subscribe>
To: Jeff Cody <jcody@redhat.com>
Cc: stefanha@gmail.com, pbonzini@redhat.com, eblake@redhat.com, qemu-devel@nongnu.org, supriyak@linux.vnet.ibm.com

Am 18.09.2012 20:53, schrieb Jeff Cody:
> 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 <jcody@redhat.com>

> +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;

You can immediately set s->aio_ctx instead of going through
BDRVRawReopenState with it. It seems to be valid to have it present
while use_aio = 0. If it wasn't valid, you'd have to free the context
when reopening without Linux AIO.

> +
> +    raw_parse_flags(state->flags, &raw_s->open_flags);
> +    raw_set_aio(&raw_s->aio_ctx, &raw_s->use_aio, state->flags);

At least you're consistently omitting the error check. :-)

> +
> +    raw_s->fd = -1;
> +
> +    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 */

Hm, still not addressed?

Kevin