Re: [Qemu-devel] [PATCH] Add new block driver for the VDI format

[Christoph Hellwig]

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..