qemu-devel
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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


From: Stefan Weil
Subject: Re: [Qemu-devel] [PATCH] Add new block driver for the VDI format (use aio)
Date: Fri, 31 Jul 2009 20:27:12 +0200
User-agent: Mozilla-Thunderbird 2.0.0.22 (X11/20090707)

Anthony Liguori schrieb:
> 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.

The latest patch (which I will send to the list soon) only contains the
aio code.

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

No static / fixed / pre-allocated images? This option should stay
in the code (maybe with the name changed).

Nor would I like to remove the block size option, as there might arise
a need for larger block sizes when the image size grows above
some tera bytes.

I see no reason why removing CONFIG_VDI_DEBUG might help.

The rest will be removed in the next patch.


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

If you have more than one statement in a define, do .. while is even
essential.

It does not matter if you have just one statement (like in this code),
and for empty statements, (void)0 is also very common.

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

Maybe. static const char [] even saves memory and code
(compared with static const char *) and should give
the same result as the define.

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

Do you think so? It might be difficult for some users to get libuuid,
especially on windows. For those users, a "uuid" of zero
won't matter when they use QEMU (the current driver only sets
uuid values during image creation and does not use them overwise).

It is possible to change the uuid values using tools supplied with
VirtualBox, so VirtualBox users also can handle those zero uuid
values.

As soon as more parts of QEMU will use libuuid code, it would be
reasonable to replace this code by code which generates real
uuid values (and move it to some other place).

>
>> +static int vdi_make_empty(BlockDriverState *bs)
>> +{
>> +    /* TODO: missing code. */
>> +    logout("\n");
>> +    return 0;
>> +}
>>   
>
> I'm not a big fan of stubs like this.

Nor am I. It's a copy from the qcow2 driver which is also a stub.
I know this is a bad excuse, but as I said before, I did not
fully understand all aspects of the block driver interface
(there is not too much documentation on it).

>
>> +#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?

I have replaced this part of the code now with a code copy
from the qcow2 driver, so the latest patch no longer uses
this #if 0 code.

>
> Regards,
>
> Anthony Liguori
>


Thanks for the review.

Regards
Stefan Weil





reply via email to

[Prev in Thread] Current Thread [Next in Thread]