Re: [Qemu-devel] [RFC PATCH 04/10] qcow2: Pass BlockdevCreateOptions to qcow2_create2()

[Kevin Wolf <kwolf@redhat.com>]

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

Am 29.01.2018 um 18:12 hat Max Reitz geschrieben:
> On 2018-01-11 20:52, Kevin Wolf wrote:
> > All of the simple options are now passed to qcow2_create2() in a
> > BlockdevCreateOptions object. Still missing: node-name and the
> > encryption options.
> > 
> > Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> > ---
> >  block/qcow2.c | 186 ++++++++++++++++++++++++++++++++++++++++++++++------------
> >  1 file changed, 148 insertions(+), 38 deletions(-)
> > 
> > diff --git a/block/qcow2.c b/block/qcow2.c
> > index b02bc39a01..09e567324d 100644
> > --- a/block/qcow2.c
> > +++ b/block/qcow2.c
> 
> [...]
> 
> > @@ -2690,12 +2697,11 @@ static uint64_t qcow2_opt_get_refcount_bits_del(QemuOpts *opts, int version,
> >      return refcount_bits;
> >  }
> >  
> > -static int qcow2_create2(BlockDriverState *bs, int64_t total_size,
> > -                         const char *backing_file, const char *backing_format,
> > -                         int flags, size_t cluster_size, PreallocMode prealloc,
> > -                         QemuOpts *opts, int version, int refcount_order,
> > -                         const char *encryptfmt, Error **errp)
> > +static int qcow2_create2(BlockDriverState *bs,
> > +                         BlockdevCreateOptions *create_options,
> 
> I'd personally really prefer this to be const...
> 
> > +                         QemuOpts *opts, const char *encryptfmt, Error **errp)
> >  {
> > +    BlockdevCreateOptionsQcow2 *qcow2_opts;
> >      QDict *options;
> >  
> >      /*
> > @@ -2712,10 +2718,88 @@ static int qcow2_create2(BlockDriverState *bs, int64_t total_size,
> 
> [...]
> 
> > +
> > +    if (!qcow2_opts->has_preallocation) {
> > +        qcow2_opts->preallocation = PREALLOC_MODE_OFF;
> > +    }
> > +    if (qcow2_opts->backing_file &&
> > +        qcow2_opts->preallocation != PREALLOC_MODE_OFF)
> > +    {
> > +        error_setg(errp, "Backing file and preallocation cannot be used at "
> > +                   "the same time");
> > +        return -EINVAL;
> > +    }
> > +
> > +    if (!qcow2_opts->has_lazy_refcounts) {
> > +        qcow2_opts->lazy_refcounts = false;
> 
> ...because modifying some ideally QMP-provided objects just looks wrong.

Isn't this pretty standard, though? Most commands don't use boxed
options, so there they only modify stack variables, but if you look at
boxed ones like do_blockdev_backup() or qmp_drive_mirror(), they do the
same.

> [...]
> 
> > @@ -2804,18 +2888,26 @@ static int qcow2_create2(BlockDriverState *bs, int64_t total_size,
> 
> [...]
> 
> >      /* Want a backing file? There you go.*/
> > -    if (backing_file) {
> > -        ret = bdrv_change_backing_file(blk_bs(blk), backing_file, backing_format);
> > +    if (qcow2_opts->has_backing_file) {
> > +        const char *backing_format = NULL;
> > +
> > +        if (qcow2_opts->has_backing_fmt) {
> > +            backing_format = BlockdevDriver_str(qcow2_opts->backing_fmt);
> > +        }
> 
> has_backing_fmt && !has_backing_file should probably be an error.

Yes.

Kevin

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 801 bytes --]