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

[Anthony Liguori]

Stefan Weil wrote:
> 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
>
> diff --git a/Makefile b/Makefile
> index d8fa730..29f4a65 100644
> --- a/Makefile
> +++ b/Makefile
> @@ -66,7 +66,7 @@ recurse-all: $(SUBDIR_RULES)
>  block-obj-y = cutils.o cache-utils.o qemu-malloc.o qemu-option.o module.o
>  block-obj-y += nbd.o block.o aio.o aes.o
>  
> -block-nested-y += cow.o qcow.o vmdk.o cloop.o dmg.o bochs.o vpc.o vvfat.o
> +block-nested-y += cow.o qcow.o vdi.o vmdk.o cloop.o dmg.o bochs.o vpc.o vvfat.o
>  block-nested-y += qcow2.o qcow2-refcount.o qcow2-cluster.o qcow2-snapshot.o
>  block-nested-y += parallels.o nbd.o
>  
> diff --git a/block/vdi.c b/block/vdi.c
> new file mode 100644
> index 0000000..0432446
> --- /dev/null
> +++ b/block/vdi.c
> @@ -0,0 +1,1105 @@
> +/*
> + * Block driver for the Virtual Disk Image (VDI) format
> + *
> + * Copyright (c) 2009 Stefan Weil
> + *
> + * This program is free software: you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License as published by
> + * the Free Software Foundation, either version 2 of the License, or
> + * (at your option) version 3 or any later version.
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> + * GNU General Public License for more details.
> + *
> + * You should have received a copy of the GNU General Public License
> + * along with this program.  If not, see <http://www.gnu.org/licenses/>.
> + *
> + * Reference:
> + * http://forums.virtualbox.org/viewtopic.php?t=8046
> + *
> + * This driver supports create / read / write operations on VDI images.
> + *
> + * Todo (see also TODO in code):
> + *
> + * Some features like snapshots are still missing.
> + *
> + * Deallocation of zero-filled blocks and shrinking images are missing, too
> + * (might be added to common block layer).
> + *
> + * Allocation of blocks could be optimized (less writes to block map and
> + * header).
> + *
> + * Read and write of adjacents blocks could be done in one operation
> + * (current code uses one operation per block (1 MiB).
> + *
> + * The code is not thread safe (missing locks for changes in header and
> + * block table, no problem with current QEMU).
> + *
> + * Hints:
> + *
> + * Blocks (VDI documentation) correspond to clusters (QEMU).
> + * QEMU's backing files could be implemented using VDI snapshot files (TODO).
> + * VDI snapshot files may also contain the complete machine state.
> + * Maybe this machine state can be converted to QEMU PC machine snapshot data.
> + *
> + * 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,
> + * so this seems to be reasonable.
> + */
> +
> +#include "qemu-common.h"
> +#include "block_int.h"
> +#include "module.h"
> +
> +#if defined(HAVE_UUID_H)
> +#include <uuid/uuid.h>
> +#else
> +/* TODO: move uuid emulation to some central place in QEMU. */
> +#include "sysemu.h"     /* UUID_FMT */
> +typedef unsigned char uuid_t[16];
> +void uuid_generate(uuid_t out);
> +void uuid_unparse(uuid_t uu, char *out);
> +#endif
> +
> +/* Code configuration options. */
> +
> +/* Use old (synchronous) I/O. */
> +//~ #undef CONFIG_AIO
>   

Please eliminate this define.  It just will lead to bitrot.


> +/* Enable debug messages. */
> +//~ #define CONFIG_VDI_DEBUG
> +
> +/* Support write operations on VDI images. */
> +#define CONFIG_VDI_WRITE
> +
> +/* Support snapshot images (not implemented yet). */
> +//~ #define CONFIG_VDI_SNAPSHOT
> +
> +/* Enable (currently) unsupported features (not implemented yet). */
> +//~ #define CONFIG_VDI_UNSUPPORTED
> +
> +/* Support non-standard block (cluster) size. */
> +//~ #define CONFIG_VDI_BLOCK_SIZE
> +
> +/* Support static (pre-allocated) images. */
> +#define CONFIG_VDI_STATIC_IMAGE
>   

Same thing for the rest of these.

> +/* Command line option for static images. */
> +#define BLOCK_OPT_STATIC "static"
> +
> +#define KiB     1024
> +#define MiB     (KiB * KiB)
> +
> +#define SECTOR_SIZE 512
> +
> +#if defined(CONFIG_VDI_DEBUG)
> +#define logout(fmt, ...) \
> +                fprintf(stderr, "vdi\t%-24s" fmt, __func__, ##__VA_ARGS__)
> +#else
> +#define logout(fmt, ...) ((void)0)
> +#endif
>   

do { } while (0) is better for these sort of things.

> +/* Image signature. */
> +#define VDI_SIGNATURE 0xbeda107f
> +
> +/* Image version. */
> +#define VDI_VERSION_1_1 0x00010001
> +
> +/* Image type. */
> +#define VDI_TYPE_DYNAMIC 1
> +#define VDI_TYPE_STATIC  2
> +
> +/* Innotek / SUN images use these strings in header.text:
> + * "<<< innotek VirtualBox Disk Image >>>\n"
> + * "<<< Sun xVM VirtualBox Disk Image >>>\n"
> + * "<<< Sun VirtualBox Disk Image >>>\n"
> + * The value does not matter, so QEMU created images use a different text.
> + */
> +#define VDI_TEXT "<<< QEMU VM Virtual Disk Image >>>\n"
>   

a static const char * is a bit nicer for this.

> +/* Unallocated blocks use this index (no need to convert endianess). */
> +#define VDI_UNALLOCATED UINT32_MAX
> +
> +#if !defined(HAVE_UUID_H)
> +void uuid_generate(uuid_t out)
> +{
> +    memset(out, 0, sizeof(out));
> +}
> +
> +void uuid_unparse(uuid_t uu, char *out)
> +{
> +    snprintf(out, 37, UUID_FMT,
> +            uu[0], uu[1], uu[2], uu[3], uu[4], uu[5], uu[6], uu[7],
> +            uu[8], uu[9], uu[10], uu[11], uu[12], uu[13], uu[14], uu[15]);
> +}
> +#endif
>   

Generating a 0 uuid seems odd to me.  Wouldn't it be better to depend 
unconditionally on libuuid?

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

I'm not a big fan of stubs like this.

> +#if defined(CONFIG_AIO)
> +
> +#if 0
> +static void vdi_aio_remove(VdiAIOCB *acb)
> +{
> +    logout("\n");
> +#if 0
> +    VdiAIOCB **pacb;
> +
> +    /* remove the callback from the queue */
> +    pacb = &posix_aio_state->first_aio;
> +    for(;;) {
> +        if (*pacb == NULL) {
> +            fprintf(stderr, "vdi_aio_remove: aio request not found!\n");
> +            break;
> +        } else if (*pacb == acb) {
> +            *pacb = acb->next;
> +            qemu_aio_release(acb);
> +            break;
> +        }
> +        pacb = &(*pacb)->next;
> +    }
> +#endif
> +}
> +#endif
> +
> +static void vdi_aio_cancel(BlockDriverAIOCB *blockacb)
> +{
> +    logout("\n");
> +
> +#if 0
> +    int ret;
> +    VdiAIOCB *acb = (VdiAIOCB *)blockacb;
> +
> +    ret = qemu_paio_cancel(acb->aiocb.aio_fildes, &acb->aiocb);
> +    if (ret == QEMU_PAIO_NOTCANCELED) {
> +        /* fail safe: if the aio could not be canceled, we wait for
> +           it */
> +        while (qemu_paio_error(&acb->aiocb) == EINPROGRESS);
> +    }
> +
> +    vdi_aio_remove(acb);
> +#endif
> +}
>   

These really should not be #if 0'd.  Is there a bug here?

Regards,

Anthony Liguori