From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from mailman by lists.gnu.org with tmda-scanned (Exim 4.43) id 1MWtzA-0007kX-P3 for qemu-devel@nongnu.org; Fri, 31 Jul 2009 11:25:32 -0400 Received: from exim by lists.gnu.org with spam-scanned (Exim 4.43) id 1MWtz5-0007d1-2J for qemu-devel@nongnu.org; Fri, 31 Jul 2009 11:25:31 -0400 Received: from [199.232.76.173] (port=38054 helo=monty-python.gnu.org) by lists.gnu.org with esmtp (Exim 4.43) id 1MWtz4-0007cu-G2 for qemu-devel@nongnu.org; Fri, 31 Jul 2009 11:25:26 -0400 Received: from mail-yw0-f190.google.com ([209.85.211.190]:39361) by monty-python.gnu.org with esmtp (Exim 4.60) (envelope-from ) id 1MWtz4-0006pZ-63 for qemu-devel@nongnu.org; Fri, 31 Jul 2009 11:25:26 -0400 Received: by ywh28 with SMTP id 28so2204466ywh.27 for ; Fri, 31 Jul 2009 08:25:25 -0700 (PDT) Message-ID: <4A730CE3.3000202@codemonkey.ws> Date: Fri, 31 Jul 2009 10:25:23 -0500 From: Anthony Liguori MIME-Version: 1.0 Subject: Re: [Qemu-devel] [PATCH] Add new block driver for the VDI format (use aio) References: <4A6888AC.3050509@mail.berlios.de> <1248380985-7362-1-git-send-email-weil@mail.berlios.de> In-Reply-To: <1248380985-7362-1-git-send-email-weil@mail.berlios.de> Content-Type: text/plain; charset=ISO-8859-1; format=flowed Content-Transfer-Encoding: 7bit List-Id: qemu-devel.nongnu.org List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Stefan Weil Cc: QEMU Developers 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 > --- > 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 . > + * > + * 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 > +#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