From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from [140.186.70.92] (port=40805 helo=eggs.gnu.org) by lists.gnu.org with esmtp (Exim 4.43) id 1PgPfK-0005sq-T0 for qemu-devel@nongnu.org; Fri, 21 Jan 2011 17:41:13 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1PgPfI-0007rm-W4 for qemu-devel@nongnu.org; Fri, 21 Jan 2011 17:41:10 -0500 Received: from mail-iw0-f173.google.com ([209.85.214.173]:60061) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1PgPfI-0007rS-QK for qemu-devel@nongnu.org; Fri, 21 Jan 2011 17:41:08 -0500 Received: by iwn40 with SMTP id 40so2245564iwn.4 for ; Fri, 21 Jan 2011 14:41:07 -0800 (PST) Message-ID: <4D3A0B7C.3000007@codemonkey.ws> Date: Fri, 21 Jan 2011 16:41:00 -0600 From: Anthony Liguori MIME-Version: 1.0 Subject: Re: [Qemu-devel] [PATCH 1/5] Fast Virtual Disk (FVD) Proposal Part 1 References: <1295474688-6219-1-git-send-email-ctang@us.ibm.com> In-Reply-To: <1295474688-6219-1-git-send-email-ctang@us.ibm.com> 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: Chunqiang Tang Cc: qemu-devel@nongnu.org On 01/19/2011 04:04 PM, Chunqiang Tang wrote: > Part 1 of the block device driver for the proposed FVD image format. > Multiple patches are used in order to manage the size of each patch. > This patch includes existing files that are modified by FVD. > > See the related discussions at > http://lists.gnu.org/archive/html/qemu-devel/2011-01/msg00426.html . > > Signed-off-by: Chunqiang Tang > --- > Makefile | 10 +++++--- > Makefile.objs | 1 + > block.c | 12 +++++----- > block_int.h | 5 ++- > configure | 2 +- > qemu-img-cmds.hx | 6 +++++ > qemu-img.c | 62 ++++++++++++++++++++++++++++++++++++++++++++--------- > qemu-io.c | 3 ++ > qemu-option.c | 4 +++ > qemu-tool.c | 36 ------------------------------- > 10 files changed, 81 insertions(+), 60 deletions(-) > > diff --git a/Makefile b/Makefile > index 6d601ee..da4d777 100644 > --- a/Makefile > +++ b/Makefile > @@ -151,13 +151,15 @@ version-obj-$(CONFIG_WIN32) += version.o > ###################################################################### > > qemu-img.o: qemu-img-cmds.h > -qemu-img.o qemu-tool.o qemu-nbd.o qemu-io.o cmd.o: $(GENERATED_HEADERS) > +qemu-img.o qemu-tool.o qemu-nbd.o qemu-io.o cmd.o qemu-test.o: $(GENERATED_HEADERS) > > -qemu-img$(EXESUF): qemu-img.o qemu-tool.o qemu-error.o $(oslib-obj-y) $(trace-obj-y) $(block-obj-y) $(qobject-obj-y) $(version-obj-y) qemu-timer-common.o > +qemu-img$(EXESUF): qemu-img.o qemu-tool.o qemu-tool-time.o qemu-error.o $(oslib-obj-y) $(trace-obj-y) $(block-obj-y) $(qobject-obj-y) $(version-obj-y) qemu-timer-common.o > > -qemu-nbd$(EXESUF): qemu-nbd.o qemu-tool.o qemu-error.o $(oslib-obj-y) $(trace-obj-y) $(block-obj-y) $(qobject-obj-y) $(version-obj-y) qemu-timer-common.o > +qemu-nbd$(EXESUF): qemu-nbd.o qemu-tool.o qemu-tool-time.o qemu-error.o $(oslib-obj-y) $(trace-obj-y) $(block-obj-y) $(qobject-obj-y) $(version-obj-y) qemu-timer-common.o > > -qemu-io$(EXESUF): qemu-io.o cmd.o qemu-tool.o qemu-error.o $(oslib-obj-y) $(trace-obj-y) $(block-obj-y) $(qobject-obj-y) $(version-obj-y) qemu-timer-common.o > +qemu-io$(EXESUF): qemu-io.o cmd.o qemu-tool.o qemu-tool-time.o qemu-error.o $(oslib-obj-y) $(trace-obj-y) $(block-obj-y) $(qobject-obj-y) $(version-obj-y) qemu-timer-common.o > + > +qemu-test$(EXESUF): qemu-test.o qemu-tool.o qemu-error.o $(oslib-obj-y) $(trace-obj-y) $(block-obj-y) $(qobject-obj-y) $(version-obj-y) qemu-timer-common.o > > qemu-img-cmds.h: $(SRC_PATH)/qemu-img-cmds.hx > $(call quiet-command,sh $(SRC_PATH)/hxtool -h< $< > $@," GEN $@") > diff --git a/Makefile.objs b/Makefile.objs > index c3e52c5..c0c1155 100644 > --- a/Makefile.objs > +++ b/Makefile.objs > @@ -23,6 +23,7 @@ block-nested-y += qcow2.o qcow2-refcount.o qcow2-cluster.o qcow2-snapshot.o > block-nested-y += qed.o qed-gencb.o qed-l2-cache.o qed-table.o qed-cluster.o > block-nested-y += qed-check.o > block-nested-y += parallels.o nbd.o blkdebug.o sheepdog.o blkverify.o > +block-nested-y += blksim.o fvd.o > block-nested-$(CONFIG_WIN32) += raw-win32.o > block-nested-$(CONFIG_POSIX) += raw-posix.o > block-nested-$(CONFIG_CURL) += curl.o > diff --git a/block.c b/block.c > index ff2795b..856bb1a 100644 > --- a/block.c > +++ b/block.c > @@ -58,7 +58,7 @@ static int bdrv_read_em(BlockDriverState *bs, int64_t sector_num, > static int bdrv_write_em(BlockDriverState *bs, int64_t sector_num, > const uint8_t *buf, int nb_sectors); > > -static QTAILQ_HEAD(, BlockDriverState) bdrv_states = > +QTAILQ_HEAD(, BlockDriverState) bdrv_states = > QTAILQ_HEAD_INITIALIZER(bdrv_states); > This looks suspicious and indicates your doing something bad. > > static QLIST_HEAD(, BlockDriver) bdrv_drivers = > @@ -768,7 +768,7 @@ int bdrv_commit(BlockDriverState *bs) > > if (!drv) > return -ENOMEDIUM; > - > + > if (!bs->backing_hd) { > return -ENOTSUP; > } > @@ -1538,7 +1538,7 @@ int bdrv_discard(BlockDriverState *bs, int64_t sector_num, int nb_sectors) > * 'nb_sectors' is the max value 'pnum' should be set to. > */ > int bdrv_is_allocated(BlockDriverState *bs, int64_t sector_num, int nb_sectors, > - int *pnum) > + int *pnum) > { > int64_t n; > if (!bs->drv->bdrv_is_allocated) { > @@ -2050,9 +2050,9 @@ BlockDriverAIOCB *bdrv_aio_readv(BlockDriverState *bs, int64_t sector_num, > cb, opaque); > > if (ret) { > - /* Update stats even though technically transfer has not happened. */ > - bs->rd_bytes += (unsigned) nb_sectors * BDRV_SECTOR_SIZE; > - bs->rd_ops ++; > + /* Update stats even though technically transfer has not happened. */ > + bs->rd_bytes += (unsigned) nb_sectors * BDRV_SECTOR_SIZE; > + bs->rd_ops ++; > } > > return ret; > diff --git a/block_int.h b/block_int.h > index 12663e8..2343d07 100644 > --- a/block_int.h > +++ b/block_int.h > @@ -28,8 +28,8 @@ > #include "qemu-option.h" > #include "qemu-queue.h" > > -#define BLOCK_FLAG_ENCRYPT 1 > -#define BLOCK_FLAG_COMPAT6 4 > +#define BLOCK_FLAG_ENCRYPT 1 > +#define BLOCK_FLAG_COMPAT6 4 > > #define BLOCK_OPT_SIZE "size" > #define BLOCK_OPT_ENCRYPT "encryption" > @@ -98,6 +98,7 @@ struct BlockDriver { > int (*bdrv_snapshot_load_tmp)(BlockDriverState *bs, > const char *snapshot_name); > int (*bdrv_get_info)(BlockDriverState *bs, BlockDriverInfo *bdi); > + int (*bdrv_update)(BlockDriverState *bs, int argc, char **argv); > argc/argv is an awful interface because the semantics end up varying widely. If we want to support changing disk format parameters, we should use a structured option format so we can ensure it's exposed to the user in a consistent way. IOW, a size is always parsed as [SUFFIX] and not 8 different variations of that theme. > bs = qemu_mallocz(bs_n * sizeof(BlockDriverState *)); > > total_sectors = 0; > @@ -865,7 +865,7 @@ static int img_convert(int argc, char **argv) > assume that sectors which are unallocated in the input image > are present in both the output's and input's base images (no > need to copy them). */ > - if (out_baseimg) { > + if (out_baseimg || bs[bs_i]->backing_file[0]==0) { > This looks like a bug fix of some sort and should be it's own patch with an explanation. > if (!bdrv_is_allocated(bs[bs_i], sector_num - bs_offset, > n,&n1)) { > sector_num += n1; > @@ -941,10 +941,10 @@ static int64_t get_allocated_file_size(const char *filename) > /* WinNT support GetCompressedFileSize to determine allocate size */ > get_compressed = (get_compressed_t) GetProcAddress(GetModuleHandle("kernel32"), "GetCompressedFileSizeA"); > if (get_compressed) { > - DWORD high, low; > - low = get_compressed(filename,&high); > - if (low != 0xFFFFFFFFlu || GetLastError() == NO_ERROR) > - return (((int64_t) high)<< 32) + low; > + DWORD high, low; > + low = get_compressed(filename,&high); > + if (low != 0xFFFFFFFFlu || GetLastError() == NO_ERROR) > + return (((int64_t) high)<< 32) + low; > } > > if (_stati64(filename,&st)< 0) > @@ -1036,11 +1036,6 @@ static int img_info(int argc, char **argv) > if (bdrv_is_encrypted(bs)) { > printf("encrypted: yes\n"); > } > - if (bdrv_get_info(bs,&bdi)>= 0) { > - if (bdi.cluster_size != 0) { > - printf("cluster_size: %d\n", bdi.cluster_size); > - } > - } > bdrv_get_backing_filename(bs, backing_filename, sizeof(backing_filename)); > if (backing_filename[0] != '\0') { > path_combine(backing_filename2, sizeof(backing_filename2), > @@ -1049,11 +1044,56 @@ static int img_info(int argc, char **argv) > backing_filename, > backing_filename2); > } > + if (bdrv_get_info(bs,&bdi)>= 0) { > + if (bdi.cluster_size != 0) > + printf("cluster_size: %d\n", bdi.cluster_size); > + } > dump_snapshots(bs); > bdrv_delete(bs); > 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; > + 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==NULL) { > + 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); > + } > + > + bs->drv->bdrv_update(bs, argc-optind,&argv[optind]); > + > + bdrv_delete(bs); > + return 0; > +} > + > #define SNAPSHOT_LIST 1 > #define SNAPSHOT_CREATE 2 > #define SNAPSHOT_APPLY 3 > diff --git a/qemu-io.c b/qemu-io.c > index 5b24c5e..c32f8d4 100644 > --- a/qemu-io.c > +++ b/qemu-io.c > @@ -1701,6 +1701,8 @@ init_check_command( > return 1; > } > > +#include "qemu-io-sim.c" > + > 1) I don't see qemu-io-sim.c in this patch which means this breaks the build 2) Including C files is evil. > static void usage(const char *name) > { > printf( > @@ -1807,6 +1809,7 @@ int main(int argc, char **argv) > add_command(&discard_cmd); > add_command(&alloc_cmd); > add_command(&map_cmd); > + add_command(&sim_cmd); > > add_args_command(init_args_command); > add_check_command(init_check_command); > diff --git a/qemu-option.c b/qemu-option.c > index 65db542..10ef45f 100644 > --- a/qemu-option.c > +++ b/qemu-option.c > @@ -289,6 +289,10 @@ int set_option_parameter(QEMUOptionParameter *list, const char *name, > return -1; > break; > > + case OPT_NUMBER: > + list->value.n = atoi (value); > + break; > + > default: > fprintf(stderr, "Bug: Option '%s' has an unknown type\n", name); > return -1; > diff --git a/qemu-tool.c b/qemu-tool.c > index 392e1c9..fdcb2f8 100644 > --- a/qemu-tool.c > +++ b/qemu-tool.c > @@ -23,12 +23,6 @@ QEMUClock *rt_clock; > > FILE *logfile; > > -struct QEMUBH > -{ > - QEMUBHFunc *cb; > - void *opaque; > -}; > - > void qemu_service_io(void) > { > } > @@ -73,36 +67,6 @@ void monitor_protocol_event(MonitorEvent event, QObject *data) > { > } > > -QEMUBH *qemu_bh_new(QEMUBHFunc *cb, void *opaque) > -{ > - QEMUBH *bh; > - > - bh = qemu_malloc(sizeof(*bh)); > - bh->cb = cb; > - bh->opaque = opaque; > - > - return bh; > -} > - > -int qemu_bh_poll(void) > -{ > - return 0; > -} > - > -void qemu_bh_schedule(QEMUBH *bh) > -{ > - bh->cb(bh->opaque); > -} > - > -void qemu_bh_cancel(QEMUBH *bh) > -{ > -} > - > -void qemu_bh_delete(QEMUBH *bh) > -{ > - qemu_free(bh); > -} > - > int qemu_set_fd_handler2(int fd, > IOCanReadHandler *fd_read_poll, > IOHandler *fd_read, > These functions surely cannot just be deleted like this. Regards, Anthony Liguori