[Top][All Lists]
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH] Add new block driver for the VDI format
From: |
Christoph Hellwig |
Subject: |
Re: [Qemu-devel] [PATCH] Add new block driver for the VDI format |
Date: |
Sun, 5 Jul 2009 10:05:23 +0200 |
User-agent: |
Mutt/1.3.28i |
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..
Re: [Qemu-devel] [PATCH] RFC: Add new block driver for the VDI format, Anthony Liguori, 2009/07/06