2014-02-18 3:01 GMT+08:00 Eric Blake <eblake@redhat.com>:

> On 02/11/2014 11:33 PM, Chunyan Liu wrote:
> > sheepdog.c: replace QEMUOptionParameter with QemuOpts
> >
> > Signed-off-by: Dong Xu Wang <wdongxu@linux.vnet.ibm.com>
> > Signed-off-by: Chunyan Liu <cyliu@suse.com>
> > ---
> >  block/sheepdog.c |  101
> +++++++++++++++++++++++++-----------------------------
> >  1 files changed, 47 insertions(+), 54 deletions(-)
> >
>
> > +
> > +    buf = NULL;
> > +    buf = qemu_opt_get_del(opts, BLOCK_OPT_REDUNDANCY);
>
> Dead NULL assignment.
>
> > +    if (buf) {
> > +        ret = parse_redundancy(s, buf);
> > +        if (ret < 0) {
> > +            goto out;
> >          }
>
> Do you need to store into ret here, or is it sufficient to use 'if
> (parse_redundancy(s, buf) < 0) {'
>
> It followed the logic in existing code. If function ret < 0, return
with that return value. Same logic in earlier codes of the function.
    if (strstr(filename, "://")) {
        ret = sd_parse_uri(s, filename, s->name, &snapid, tag);
    } else {
        ret = parse_vdiname(s, filename, s->name, &snapid, tag);
    }
    if (ret < 0) {
        goto out;
    }


> > -    {
> > -        .name = BLOCK_OPT_REDUNDANCY,
> > -        .type = OPT_STRING,
> > -        .help = "Redundancy of the image"
> > -    },
> > -    { NULL }
> > +static QemuOptsList sd_create_opts = {
> > +    .name = "sheepdog-create-opts",
> > +    .head = QTAILQ_HEAD_INITIALIZER(sd_create_opts.head),
> > +    .desc = {
> > +        {
>
> > +            .name = BLOCK_OPT_PREALLOC,
> > +            .type = QEMU_OPT_STRING,
> > +            .help = "Preallocation mode (allowed values: off, full)"
> > +        },
> > +        { /* end of list */ }
>
> Missing redundancy in the new list.
>
> > @@ -2538,7 +2533,7 @@ static BlockDriver bdrv_sheepdog = {
> >      .bdrv_save_vmstate  = sd_save_vmstate,
> >      .bdrv_load_vmstate  = sd_load_vmstate,
> >
> > -    .create_options = sd_create_options,
> > +    .create_opts   = &sd_create_opts,
>
> Another example of the inconsistent use of &.
>
> >  };
> >
> >  static BlockDriver bdrv_sheepdog_tcp = {
> > @@ -2548,7 +2543,7 @@ static BlockDriver bdrv_sheepdog_tcp = {
> >      .bdrv_needs_filename = true,
> >      .bdrv_file_open = sd_open,
> >      .bdrv_close     = sd_close,
> > -    .bdrv_create    = sd_create,
> > +    .bdrv_create2   = sd_create,
> >      .bdrv_has_zero_init = bdrv_has_zero_init_1,
> >      .bdrv_getlength = sd_getlength,
> >      .bdrv_get_allocated_file_size = sd_get_allocated_file_size,
> > @@ -2568,7 +2563,6 @@ static BlockDriver bdrv_sheepdog_tcp = {
> >      .bdrv_save_vmstate  = sd_save_vmstate,
> >      .bdrv_load_vmstate  = sd_load_vmstate,
> >
> > -    .create_options = sd_create_options,
> >  };
>
> Why no .create_opts replacement?
>
> --
> Eric Blake   eblake redhat com    +1-919-301-3266
> Libvirt virtualization library http://libvirt.org
>
>