From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from [140.186.70.92] (port=37746 helo=eggs.gnu.org) by lists.gnu.org with esmtp (Exim 4.43) id 1Pil5p-0003y5-49 for qemu-devel@nongnu.org; Fri, 28 Jan 2011 04:58:17 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1Pil5n-0005gv-Bq for qemu-devel@nongnu.org; Fri, 28 Jan 2011 04:58:12 -0500 Received: from mail-fx0-f45.google.com ([209.85.161.45]:49847) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Pil5n-0005gd-7N for qemu-devel@nongnu.org; Fri, 28 Jan 2011 04:58:11 -0500 Received: by fxm12 with SMTP id 12so3250586fxm.4 for ; Fri, 28 Jan 2011 01:58:10 -0800 (PST) Date: Fri, 28 Jan 2011 09:57:53 +0000 From: Stefan Hajnoczi Subject: Re: [Qemu-devel] [PATCH 1/3] FVD: Added support for 'qemu-img update' Message-ID: <20110128095753.GC3082@stefanha-thinkpad.localdomain> References: <1295648355-17359-1-git-send-email-ctang@us.ibm.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <1295648355-17359-1-git-send-email-ctang@us.ibm.com> List-Id: qemu-devel.nongnu.org List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Chunqiang Tang Cc: qemu-devel@nongnu.org On Fri, Jan 21, 2011 at 05:19:13PM -0500, Chunqiang Tang wrote: > This patch adds the 'update' command to qemu-img. FVD stores various > image-specific configurable parameters in the image header. A user can use > 'qemu-img update' to modify those parameters and accordingly controls FVD's > runtime behavior. This command may also be leveraged by other block device > drivers, e.g., to set the size of the in-memory metadata cache. Currently > those parameters are hard-coded in a one-size-fit-all manner. There's a high risk that users will try this command while the VM is running. A safe-guard is needed here in order to avoid corrupting the image. Please use qemu-option.h instead of int argc, char **argv just like qemu-img create -o does. Finally, is this interface really necessary? As a developer it can be useful to tweak image values (in QED I actually have a free-standing tool that can query and manipulate image internals). But should users need to micromanage every image file in order to achieve desired functionality/performance? What's the real need here? > diff --git a/qemu-img.c b/qemu-img.c > index afd9ed2..5f35c4d 100644 > --- a/qemu-img.c > +++ b/qemu-img.c > @@ -1054,6 +1054,49 @@ static int img_info(int argc, char **argv) > return 0; > } > > +static int img_update(int argc, char **argv) > +{ > + int c; > + const char *filename, *fmt; > + BlockDriverState *bs; > + > + fmt = NULL; > + for(;;) { > + c = getopt(argc, argv, "f:h"); > + if (c == -1) > + break; {}, see CODING_STYLE and HACKING. > + switch(c) { > + case 'h': > + help(); > + break; > + case 'f': > + fmt = optarg; > + break; > + } > + } > + if (optind >= argc) > + help(); {} > + filename = argv[optind++]; > + bs = bdrv_new_open(filename, fmt, BDRV_O_FLAGS | BDRV_O_NO_BACKING > + | BDRV_O_RDWR); > + if (!bs) { > + return 1; > + } > + > + if (bs->drv->bdrv_update) { > + bs->drv->bdrv_update(bs, argc-optind, &argv[optind]); > + } > + else { } else {, see CODING_STYLE > + char fmt_name[128]; > + bdrv_get_format(bs, fmt_name, sizeof(fmt_name)); > + error_report ("The 'update' command is not supported for " > + "the '%s' image format.", fmt_name); Return value should be 1? Stefan