From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from mailman by lists.gnu.org with tmda-scanned (Exim 4.43) id 1MNSIg-0002QE-GD for qemu-devel@nongnu.org; Sun, 05 Jul 2009 10:02:38 -0400 Received: from exim by lists.gnu.org with spam-scanned (Exim 4.43) id 1MNSId-0002Ny-1y for qemu-devel@nongnu.org; Sun, 05 Jul 2009 10:02:38 -0400 Received: from [199.232.76.173] (port=56944 helo=monty-python.gnu.org) by lists.gnu.org with esmtp (Exim 4.43) id 1MNSIc-0002Nm-RQ for qemu-devel@nongnu.org; Sun, 05 Jul 2009 10:02:34 -0400 Received: from moutng.kundenserver.de ([212.227.17.10]:56912) by monty-python.gnu.org with esmtp (Exim 4.60) (envelope-from ) id 1MNSIc-0005lK-3o for qemu-devel@nongnu.org; Sun, 05 Jul 2009 10:02:34 -0400 Message-ID: <4A50B275.7030100@mail.berlios.de> Date: Sun, 05 Jul 2009 16:02:29 +0200 From: Stefan Weil MIME-Version: 1.0 Subject: Re: [Qemu-devel] [PATCH] Add new block driver for the VDI format References: <4A4E5AFC.4020206@mail.berlios.de> <1246649386-6006-1-git-send-email-weil@mail.berlios.de> <1246649386-6006-2-git-send-email-weil@mail.berlios.de> <20090705080523.GA31550@lst.de> In-Reply-To: <20090705080523.GA31550@lst.de> Content-Type: text/plain; charset=ISO-8859-1 Content-Transfer-Encoding: 7bit List-Id: qemu-devel.nongnu.org List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Christoph Hellwig Cc: QEMU Developers 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