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

[Stefan Weil]

Kevin Wolf schrieb:
> Stefan Weil schrieb:
>   
>> This is a new block driver written from scratch
>> to support the VDI format in QEMU.
>>
>> VDI is the native format used by Innotek / SUN VirtualBox.
>>
>> Signed-off-by: Stefan Weil <address@hidden>
>> ---
>>  Makefile      |    2 +-
>>  block/vdi.c   | 1105 
>> +++++++++++++++++++++++++++++++++++++++++++++++++++++++++
>>  qemu-img.texi |    2 +
>>  3 files changed, 1108 insertions(+), 1 deletions(-)
>>  create mode 100644 block/vdi.c
>>
>>     
...
>> + *
>> + * The driver keeps a block cache (little endian entries) in memory.
>> + * For the standard block size (1 MiB), a terrabyte disk will use 4 MiB RAM,
>>     
>
> Tera, not terra. ;-)
>   
Thanks. I'll replace it by "a 1 TiB disk" :-)

...
>> +/* Enable (currently) unsupported features (not implemented yet). */
>> +//~ #define CONFIG_VDI_UNSUPPORTED
>> +
>> +/* Support non-standard block (cluster) size. */
>> +//~ #define CONFIG_VDI_BLOCK_SIZE
>>     
>
> Actually, this is only about support for image creation. Any reason why
> we shouldn't support creating images with non-standard block sizes? The
> code already supports opening such images unconditionally, so the only
> effect of turning it off for image creation is that we can't test that
> functionality in qemu-iotests.
>
> [Oh, sorry, actually there is a check in open which I missed at first.
> Any reason why we can't support it? But it's consistent at least.]
>   

Multiples of 512 (SECTOR_SIZE) might work.

VirtualBox uses 1 MiB blocks, and I did not see options to create images
with different block sizes. Maybe they even don't support such images.
So I did not spend the time to test other block sizes.
Why implement things nobody needs?

>   
>> +/* Support static (pre-allocated) images. */
>> +#define CONFIG_VDI_STATIC_IMAGE
>> +
>> +/* Command line option for static images. */
>> +#define BLOCK_OPT_STATIC "static"
>>     
>
> What about calling it "preallocate" and moving it to block_int.h? I
> think this could make sense for other drivers, too.
>   

Yes, this would be reasonable if we had more drivers with support
for "preallocate".

The VDI documentation calls these images "static", and they prefer
dynamic images, so this static option is not really very important.


...

>   
>> +
>> +typedef struct {
>> +    char text[0x40];
>> +    uint32_t signature;
>> +    uint32_t version;
>> +    uint32_t header_size;
>> +    uint32_t image_type;
>> +    uint32_t image_flags;
>> +    char description[256];
>> +    uint32_t offset_bmap;
>> +    uint32_t offset_data;
>> +    uint32_t cylinders;         /* disk geometry, unused here */
>> +    uint32_t heads;             /* disk geometry, unused here */
>> +    uint32_t sectors;           /* disk geometry, unused here */
>>     
>
> Is the geometry unused by VBox? If not, leaving it unused here is most
> probably wrong. At least for image creation you need to fill the fields.
>
> In the case of VHD, the geometry was the really significant thing. Using
> the disk size in the header (which was inconsistent with the geometry)
> meant that qemu-img convert to raw resulted in a virtual hard disk of
> different size. You should check this for VDI.
>
>   

VirtualBox sets these values to zero, so my code does this, too.
They are unused, so neither QEMU nor the client can see them.

...

>> +
>> +static int vdi_make_empty(BlockDriverState *bs)
>> +{
>> +    /* TODO: missing code. */
>> +    logout("\n");
>> +    return 0;
>> +}
>>     
>
> If you don't implement it, leave it out. Setting
> bdrv_vdi.bdrv_make_empty != NULL means that you claim to have that
> functionality.
>
>   

I did not analyse what *_make_empty is supposed to do.
This is one of the details were hints of the block driver experts
would be helpful.

>> +
>> +static int vdi_probe(const uint8_t *buf, int buf_size, const char *filename)
>> +{
>> +    const VdiHeader *header = (const VdiHeader *)buf;
>> +    int result = 0;
>> +
>> +    logout("\n");
>> +
>> +    if (buf_size < sizeof(*header)) {
>> +        /* Header too small, no VDI. */
>> +    } else if (le32_to_cpu(header->signature) == VDI_SIGNATURE) {
>> +        result = 100;
>> +    }
>> +
>> +    if (result == 0) {
>> +        logout("no vdi image\n");
>> +    } else {
>> +        logout("%s", header->text);
>> +    }
>> +
>> +    return result;
>> +}
>> +
>> +#if defined(CONFIG_VDI_SNAPSHOT)
>> +static int vdi_snapshot_create(const char *filename, const char 
>> *backing_file)
>> +{
>> +    /* TODO: missing code. */
>> +    logout("\n");
>> +    return -1;
>> +}
>> +#endif
>>     
>
> I don't like such stubs. But at least they are guarded by #ifdef here...
>
>   
...

>> +
>> +#if defined(CONFIG_AIO)
>> +
>> +#if 0
>>     
>
> I guess you should remove this block before the patch is included.
>   

This is also one of the details were hints of the block driver experts
would be helpful as I did not understand this aio_remove / aio_cancel
mechanism.


...

>> +#else /* CONFIG_AIO */
>>     
>
> No reason to retain the old code. It's just duplicated code that is
> disabled by default and therefore likely to break soon.
>   

I agree. As soon as the rest is ok, this part of the code will be removed.

>   
>> +
>> +static int vdi_read(BlockDriverState *bs, int64_t sector_num,
>> +                    uint8_t *buf, int nb_sectors)
>> +{
>> +    BDRVVdiState *s = (BDRVVdiState *)bs->opaque;
>> +    logout("%p, %" PRId64 ", %p, %d\n", bs, sector_num, buf, nb_sectors);
>> +    if (sector_num < 0) {
>> +        logout("unsupported sector %" PRId64 "\n", sector_num);
>> +        return -1;
>> +    }
>> +    while (nb_sectors > 0 && sector_num < bs->total_sectors) {
>> +        uint32_t bmap_entry;
>> +        size_t block_index = sector_num / s->block_sectors;
>> +        size_t sector_in_block = sector_num % s->block_sectors;
>> +        size_t n_sectors = s->block_sectors - sector_in_block;
>> +        if (n_sectors > nb_sectors) {
>> +            n_sectors = nb_sectors;
>> +        }
>> +        bmap_entry = le32_to_cpu(s->bmap[block_index]);
>> +        if (bmap_entry == VDI_UNALLOCATED) {
>> +            /* Block not allocated, return zeros. */
>> +            memset(buf, 0, n_sectors * SECTOR_SIZE);
>> +        } else {
>> +            uint64_t offset = s->header.offset_data / SECTOR_SIZE +
>> +                (uint64_t)bmap_entry * s->block_sectors + sector_in_block;
>> +            if (bdrv_read(s->hd, offset, buf, n_sectors) < 0) {
>> +                logout("read error\n");
>> +
>>     
>   
...

> Does VDI support compression even theoretically?
>   

I think it would be possible to extend the specification
to support compression or encryption.

The official specification (as far as I know it) does not
support compression (nor encryption).

...
>>  @item vmdk
>>  VMware 3 and 4 compatible image format.
>>  @item cloop
>>     
>
> The actual read/write code looks good to me. However, I haven't tested
> it, this is just by reading the code.
>
> Kevin
>
>   


Thank you for this review.

Stefan