Re: [Qemu-devel] [PATCH V16 5/7] Use QemuOpts support in block layer

[Eric Blake <eblake@redhat.com>]

[-- Attachment #1: Type: text/plain, Size: 2641 bytes --]

On 06/18/2013 03:31 AM, Dong Xu Wang wrote:
> This patch uses QemuOpts related functions in block layer, add
> a member bdrv_create_opts to BlockDriver struct, it returns
> a QemuOptsList pointer, which includes the image format's create
> options.
> 
> And create options's primary consumer is block creating related

s/options's/options'/

> functions, so modify them together.
> 
> Signed-off-by: Dong Xu Wang <wdongxu@linux.vnet.ibm.com>
> ---

> 
>  block.c                   | 100 +++++++++++------------
>  block/cow.c               |  52 ++++++------
>  block/gluster.c           |  37 +++++----
>  block/iscsi.c             |  31 ++++----
>  block/qcow.c              |  67 ++++++++--------
>  block/qcow2.c             | 199 +++++++++++++++++++++++++---------------------
>  block/qed.c               | 108 +++++++++++++------------
>  block/qed.h               |   2 +-
>  block/raw-posix.c         |  59 ++++++--------
>  block/raw-win32.c         |  31 ++++----
>  block/raw.c               |  30 ++++---
>  block/rbd.c               |  62 +++++++--------
>  block/sheepdog.c          |  81 +++++++++----------
>  block/ssh.c               |  29 ++++---
>  block/vdi.c               |  70 ++++++++--------
>  block/vmdk.c              | 129 ++++++++++++++++--------------
>  block/vpc.c               |  65 ++++++++-------
>  block/vvfat.c             |  11 +--
>  include/block/block.h     |   5 +-
>  include/block/block_int.h |   6 +-
>  qemu-img.c                |  65 ++++++++-------
>  21 files changed, 629 insertions(+), 610 deletions(-)

Looks rather daunting.  If I had reviewed this earlier in the series, I
might have asked if it was worth splitting into one patch per block
backend (if incremental conversion works), rather than all backends in
one go.  But now that we are already at v16, I'm not so sure.  So I'll
go ahead and review under the assumption that a single patch is okay.

> +++ b/block.c

> @@ -385,7 +384,7 @@ int bdrv_create(BlockDriver *drv, const char* filename,
>      CreateCo cco = {
>          .drv = drv,
>          .filename = g_strdup(filename),
> -        .options = options,
> +        .opts = opts ?: qemu_opts_create_nofail(drv->bdrv_create_opts),

I guess I can't complain about this gcc extension, since you aren't the
first.

The rest of this file looks okay.

> +++ b/block/cow.c

...and here, I ran out of review time at the moment, so I'm splitting
the review across multiple emails when I get more time.

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 621 bytes --]