Re: [Qemu-devel] [PATCH v7 11/25] qcow2: refcount_order parameter for qcow2_create2

[Max Reitz]

On 2015-02-18 at 21:51, Eric Blake wrote:
> On 02/18/2015 03:40 PM, Max Reitz wrote:
> > Add a refcount_order parameter to qcow2_create2(), use that value for
> > the image header and for calculating the size required for
> > preallocation.
> >
> > For now, always pass 4.
> >
> > This addition requires changes to the calculation of the file size for
> > the "full" and "falloc" preallocation modes. That in turn is a nice
> > opportunity to add a comment about that calculation not necessarily
> > being exact (and that being intentional).
> >
> > Signed-off-by: Max Reitz <address@hidden>
> > ---
> >   block/qcow2.c | 47 ++++++++++++++++++++++++++++++++++++-----------
> >   1 file changed, 36 insertions(+), 11 deletions(-)
> >
> > @@ -2010,6 +2022,8 @@ static int qcow2_create(const char *filename, QemuOpts 
> > *opts, Error **errp)
> >       size_t cluster_size = DEFAULT_CLUSTER_SIZE;
> >       PreallocMode prealloc;
> >       int version = 3;
> > +    uint64_t refcount_bits = 16;
> > +    int refcount_order;
> >       Error *local_err = NULL;
> >       int ret;
> >   
> > @@ -2064,8 +2078,19 @@ static int qcow2_create(const char *filename, QemuOpts *opts, Error **errp)
> >           goto finish;
> >       }
> >   
> > +    if (version < 3 && refcount_bits != 16) {
> > +        error_setg(errp, "Different refcount widths than 16 bits require "
> > +                   "compatibility level 1.1 or above (use compat=1.1 or "
> > +                   "greater)");
> > +        ret = -EINVAL;
> > +        goto finish;
> > +    }
> > +
> > +    refcount_order = ffs(refcount_bits) - 1;
> ffs() doesn't work on uint64_t (it gives the wrong answer for
> 0x100000000, for example); you want to use ffsll().  But ffsll() isn't
> portable.  But we have include/qemu/host-utils.h that gives us ctz64()
> which is what we want (where ctz==ffs-1 other than for the special case
> of 0).  So this should be:
>
> refcount_order = ctz64(refcount_bits);
>
> With that change,
> Reviewed-by: Eric Blake <address@hidden>

I intentionally left the ffs() here, because after this patch, 
refcount_bits is guaranteed to be 16, and after patch 14, it's 
guaranteed to be 64 or less (it's more obvious after patch 14 than 
here). So I would be fine with ctz64(), but in my opinion, ffs() is just 
fine, too.

Max

> (Hmm, that means that at least qcow2.c:qcow2_create2() has a bug for
> calling ffs(size_t), and we probably ought to eradicate other uses of
> ffs from the tree - but as a separate followup).