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

[Stefan Weil]

Christoph Hellwig schrieb:
> 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.
>

CONFIG_VDI_UNSUPPORTED and CONFIG_VDI_SNAPSHOT document
code parts which are still missing or unfinished.
For the same reason, they are undefined, so the unfinished
code is deactivated.

CONFIG_VDI_CLUSTER_SIZE is a harmless option but its code
is unfinished, too. For this reason, it is deactivated by
default.

CONFIG_VDI_WRITE is enabled by default. Users who want to disable
writes can use it to do so.

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

Not having the method would hide the fact that the
method might be implemented.

vdi_check is unfinished code, and there is even a comment
which says that there remains something to do.

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

Again this function exists to document an open question.
If someone can say that the function is complete, the TODO
comment will be removed and it will return zero.

As long as I don't know that it works, it would be dangerous
to return success.
>> +static int vdi_make_empty(BlockDriverState *bs)
>> +{
>> +    /* TODO: missing code. */
>> +    logout("\n");
>> +    return -ENOTSUP;
>> +}
>
> Again, no need to implement an empty method here.

There is the same need for this function like for vdi_check:
the function is just a hook to implement more code.

>
>> +    /* 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..

Therefore it was marked with a TODO comment.

To summarize my answer: the code is complete enough to be useful
(create / read / write are implemented). It can be further extended
and optimized, and therefore there are TODO comments and code parts
which show those incomplete or missing parts.

I am glad you had no real objections, so I hope the patches can soon
be commited.

By the way - is it possible to check new block drivers like this one
using qemu-io (can I use an existing test sequence)?

Regards,

Stefan