From mboxrd@z Thu Jan  1 00:00:00 1970
Received: from mailman by lists.gnu.org with tmda-scanned (Exim 4.43)
	id 1MNMj5-0007qH-VU
	for qemu-devel@nongnu.org; Sun, 05 Jul 2009 04:05:31 -0400
Received: from exim by lists.gnu.org with spam-scanned (Exim 4.43)
	id 1MNMj1-0007ox-72
	for qemu-devel@nongnu.org; Sun, 05 Jul 2009 04:05:31 -0400
Received: from [199.232.76.173] (port=41668 helo=monty-python.gnu.org)
	by lists.gnu.org with esmtp (Exim 4.43) id 1MNMj1-0007ou-08
	for qemu-devel@nongnu.org; Sun, 05 Jul 2009 04:05:27 -0400
Received: from verein.lst.de ([213.95.11.210]:43426)
	by monty-python.gnu.org with esmtps
	(TLS-1.0:DHE_RSA_3DES_EDE_CBC_SHA1:24) (Exim 4.60)
	(envelope-from <hch@lst.de>) id 1MNMj0-0000y5-Bs
	for qemu-devel@nongnu.org; Sun, 05 Jul 2009 04:05:26 -0400
Date: Sun, 5 Jul 2009 10:05:23 +0200
From: Christoph Hellwig <hch@lst.de>
Subject: Re: [Qemu-devel] [PATCH] Add new block driver for the VDI format
Message-ID: <20090705080523.GA31550@lst.de>
References: <4A4E5AFC.4020206@mail.berlios.de>
	<1246649386-6006-1-git-send-email-weil@mail.berlios.de>
	<1246649386-6006-2-git-send-email-weil@mail.berlios.de>
Mime-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <1246649386-6006-2-git-send-email-weil@mail.berlios.de>
List-Id: qemu-devel.nongnu.org
List-Unsubscribe: <http://lists.nongnu.org/mailman/listinfo/qemu-devel>,
	<mailto:qemu-devel-request@nongnu.org?subject=unsubscribe>
List-Archive: <http://lists.gnu.org/pipermail/qemu-devel>
List-Post: <mailto:qemu-devel@nongnu.org>
List-Help: <mailto:qemu-devel-request@nongnu.org?subject=help>
List-Subscribe: <http://lists.nongnu.org/mailman/listinfo/qemu-devel>,
	<mailto:qemu-devel-request@nongnu.org?subject=subscribe>
To: Stefan Weil <weil@mail.berlios.de>
Cc: QEMU Developers <qemu-devel@nongnu.org>

On Fri, Jul 03, 2009 at 09:29:46PM +0200, Stefan Weil wrote:
> +/* Enable debug messages. */
> +//~ #define CONFIG_VDI_DEBUG
> +
> +/* Support experimental write operations on VDI images. */
> +#define CONFIG_VDI_WRITE
> +
> +/* Support snapshot images. */
> +//~ #define CONFIG_VDI_SNAPSHOT
> +
> +/* Enable (currently) unsupported features. */
> +//~ #define CONFIG_VDI_UNSUPPORTED
> +
> +/* Support non-standard cluster (block) size. */
> +//~ #define CONFIG_VDI_CLUSTER_SIZE

I don't think we should keep these defines (except for the debug one)
around. CONFIG_VDI_UNSUPPORTED adds methods to the method table that
aren't actually implemented so the code will fail to compile if it's
set.  Similar for CONFIG_VDI_SNAPSHOT except that it implements a single
useless stub.  CONFIG_VDI_CLUSTER_SIZE just adds a harmless option
so it should just be unconditional, too.

I also don't see a reason for the CONFIG_VDI_WRITE ifdef as it's
apparently good enough to be enable by default.

> +static int vdi_check(BlockDriverState *bs)
> +{
> +    /* TODO: missing code. */
> +    logout("\n");
> +    return -ENOTSUP;
> +}

No need to implement this, not having the method gives the same result.

> +static int vdi_get_info(BlockDriverState *bs, BlockDriverInfo *bdi)
> +{
> +    /* TODO: unchecked code. */
> +    BDRVVdiState *s = (BDRVVdiState *)bs->opaque;
> +    logout("\n");
> +    bdi->cluster_size = s->cluster_size;
> +    bdi->vm_state_offset = -1;
> +    return -ENOTSUP;
> +}

If you return a negative value the result is ignored, so either at least
implement a stub one or just leave out the method.

> +static int vdi_make_empty(BlockDriverState *bs)
> +{
> +    /* TODO: missing code. */
> +    logout("\n");
> +    return -ENOTSUP;
> +}

Again, no need to implement an empty method here.

> +    /* Performance is terrible right now with cache=writethrough due mainly
> +     * to reference count updates.  If the user does not explicitly specify
> +     * a caching type, force to writeback caching.
> +     * TODO: This was copied from qcow2.c, maybe it is true for vdi, too.
> +     */
> +    if ((flags & BDRV_O_CACHE_DEF)) {
> +        flags |= BDRV_O_CACHE_WB;
> +        flags &= ~BDRV_O_CACHE_DEF;
> +    }

And it looks like we're going to change it for qcow2, too..