From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([208.118.235.92]:52157) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1TaUvi-0005mY-BG for qemu-devel@nongnu.org; Mon, 19 Nov 2012 12:14:48 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1TaUva-0005bg-6W for qemu-devel@nongnu.org; Mon, 19 Nov 2012 12:14:42 -0500 Received: from mail-bk0-f45.google.com ([209.85.214.45]:52505) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1TaUvZ-0005bW-OY for qemu-devel@nongnu.org; Mon, 19 Nov 2012 12:14:34 -0500 Received: by mail-bk0-f45.google.com with SMTP id jk13so1039356bkc.4 for ; Mon, 19 Nov 2012 09:14:32 -0800 (PST) Date: Mon, 19 Nov 2012 18:14:30 +0100 From: Stefan Hajnoczi Message-ID: <20121119171430.GA19114@stefanha-thinkpad.redhat.com> References: <1351761150-20705-1-git-send-email-wdongxu@linux.vnet.ibm.com> <1351761150-20705-10-git-send-email-wdongxu@linux.vnet.ibm.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <1351761150-20705-10-git-send-email-wdongxu@linux.vnet.ibm.com> Subject: Re: [Qemu-devel] [PATCH V5 09/10] Use QemuOpts support in block layer List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Dong Xu Wang Cc: kwolf@redhat.com, qemu-devel@nongnu.org On Thu, Nov 01, 2012 at 05:12:29PM +0800, Dong Xu Wang wrote: > diff --git a/block/qcow.c b/block/qcow.c > index b239c82..9bb736a 100644 > --- a/block/qcow.c > +++ b/block/qcow.c > @@ -651,7 +651,7 @@ static void qcow_close(BlockDriverState *bs) > error_free(s->migration_blocker); > } > > -static int qcow_create(const char *filename, QEMUOptionParameter *options) > +static int qcow_create(const char *filename, QemuOpts *opts) > { > int header_size, backing_filename_len, l1_size, shift, i; > QCowHeader header; > @@ -662,19 +662,14 @@ static int qcow_create(const char *filename, QEMUOptionParameter *options) > int ret; > BlockDriverState *qcow_bs; > > - /* Read out options */ > - while (options && options->name) { > - if (!strcmp(options->name, BLOCK_OPT_SIZE)) { > - total_size = options->value.n / 512; > - } else if (!strcmp(options->name, BLOCK_OPT_BACKING_FILE)) { > - backing_file = options->value.s; > - } else if (!strcmp(options->name, BLOCK_OPT_ENCRYPT)) { > - flags |= options->value.n ? BLOCK_FLAG_ENCRYPT : 0; > - } > - options++; > + /* Read out opts */ > + total_size = qemu_opt_get_number(opts, BLOCK_OPT_SIZE, 0) / 512; > + backing_file = qemu_opt_get(opts, BLOCK_OPT_BACKING_FILE); > + if (qemu_opt_get_bool(opts, BLOCK_OPT_ENCRYPT, 0)) { > + flags |= BLOCK_FLAG_ENCRYPT; > } > > - ret = bdrv_create_file(filename, options); > + ret = bdrv_create_file(filename, opts); Previously options would be an empty QEMUOptionParameter (because the while loop iterated until options->name). Now you are passing the same opts for creating this image file, this seems wrong. Should this be bdrv_create_file(filename, NULL)? > diff --git a/block/qcow2.c b/block/qcow2.c > index c1ff31f..9cea051 100644 > --- a/block/qcow2.c > +++ b/block/qcow2.c > @@ -1180,7 +1180,7 @@ static int preallocate(BlockDriverState *bs) > static int qcow2_create2(const char *filename, int64_t total_size, > const char *backing_file, const char *backing_format, > int flags, size_t cluster_size, int prealloc, > - QEMUOptionParameter *options, int version) > + QemuOpts *opts, int version) The opts argument can be dropped since it is not used. > @@ -1314,7 +1314,7 @@ out: > return ret; > } > > -static int qcow2_create(const char *filename, QEMUOptionParameter *options) > +static int qcow2_create(const char *filename, QemuOpts *opts) > { > const char *backing_file = NULL; > const char *backing_fmt = NULL; > @@ -1323,45 +1323,41 @@ static int qcow2_create(const char *filename, QEMUOptionParameter *options) > size_t cluster_size = DEFAULT_CLUSTER_SIZE; > int prealloc = 0; > int version = 2; > + const char *buf; > > /* Read out options */ > - while (options && options->name) { > - if (!strcmp(options->name, BLOCK_OPT_SIZE)) { > - sectors = options->value.n / 512; > - } else if (!strcmp(options->name, BLOCK_OPT_BACKING_FILE)) { > - backing_file = options->value.s; > - } else if (!strcmp(options->name, BLOCK_OPT_BACKING_FMT)) { > - backing_fmt = options->value.s; > - } else if (!strcmp(options->name, BLOCK_OPT_ENCRYPT)) { > - flags |= options->value.n ? BLOCK_FLAG_ENCRYPT : 0; > - } else if (!strcmp(options->name, BLOCK_OPT_CLUSTER_SIZE)) { > - if (options->value.n) { > - cluster_size = options->value.n; > - } > - } else if (!strcmp(options->name, BLOCK_OPT_PREALLOC)) { > - if (!options->value.s || !strcmp(options->value.s, "off")) { > - prealloc = 0; > - } else if (!strcmp(options->value.s, "metadata")) { > - prealloc = 1; > - } else { > - fprintf(stderr, "Invalid preallocation mode: '%s'\n", > - options->value.s); > - return -EINVAL; > - } > - } else if (!strcmp(options->name, BLOCK_OPT_COMPAT_LEVEL)) { > - if (!options->value.s || !strcmp(options->value.s, "0.10")) { > - version = 2; > - } else if (!strcmp(options->value.s, "1.1")) { > - version = 3; > - } else { > - fprintf(stderr, "Invalid compatibility level: '%s'\n", > - options->value.s); > - return -EINVAL; > - } > - } else if (!strcmp(options->name, BLOCK_OPT_LAZY_REFCOUNTS)) { > - flags |= options->value.n ? BLOCK_FLAG_LAZY_REFCOUNTS : 0; > - } > - options++; The old code worked when options == NULL. I think the new code should also work when opts == NULL: if (opts) { .... } This should be done for all block drivers or the way that bdrv_create(..., NULL) works should be changed so that block drivers don't have to check for opts == NULL. > @@ -674,19 +687,14 @@ static int64_t raw_get_allocated_file_size(BlockDriverState *bs) > return (int64_t)st.st_blocks * 512; > } > > -static int raw_create(const char *filename, QEMUOptionParameter *options) > +static int raw_create(const char *filename, QemuOpts *opts) > { > int fd; > int result = 0; > int64_t total_size = 0; > > - /* Read out options */ > - while (options && options->name) { > - if (!strcmp(options->name, BLOCK_OPT_SIZE)) { > - total_size = options->value.n / BDRV_SECTOR_SIZE; > - } > - options++; > - } > + total_size = opts ? > + qemu_opt_get_number(opts, BLOCK_OPT_SIZE, 0) / BDRV_SECTOR_SIZE : 0; total_size is initialized to 0 above, so this line can be made more readable like this: if (opts) { total_size = qemu_opt_get_number(opts, BLOCK_OPT_SIZE, 0); } > @@ -1081,20 +1079,15 @@ static int fd_open(BlockDriverState *bs) > > #endif /* !linux && !FreeBSD */ > > -static int hdev_create(const char *filename, QEMUOptionParameter *options) > +static int hdev_create(const char *filename, QemuOpts *opts) > { > int fd; > int ret = 0; > struct stat stat_buf; > int64_t total_size = 0; > > - /* Read out options */ > - while (options && options->name) { > - if (!strcmp(options->name, "size")) { > - total_size = options->value.n / BDRV_SECTOR_SIZE; > - } > - options++; > - } > + total_size = opts ? > + qemu_opt_get_number(opts, BLOCK_OPT_SIZE, 0) / BDRV_SECTOR_SIZE : 0; Same here.