Re: [Qemu-devel] [PATCH 1/5] Fast Virtual Disk (FVD) Proposal Part 1

[Anthony Liguori <anthony@codemonkey.ws>]

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<ctang@us.ibm.com>
> ---
>   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 
<integer>[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