Re: [Qemu-devel] [PATCH v14 3/5] qapi: introduce PreallocMode and new PreallocModes full and falloc.

[Hu Tao]

On Tue, Sep 09, 2014 at 06:42:13AM -0600, Eric Blake wrote:
> On 09/08/2014 09:54 PM, Hu Tao wrote:
> > This patch prepares for the subsequent patches.
> > 
> > Signed-off-by: Hu Tao <address@hidden>
> > Reviewed-by: Max Reitz <address@hidden>
> > Reviewed-by: Kevin Wolf <address@hidden>
> > ---
> >  block/qcow2.c              | 23 +++++++++++++++--------
> >  qapi/block-core.json       | 17 +++++++++++++++++
> >  tests/qemu-iotests/049.out |  2 +-
> >  3 files changed, 33 insertions(+), 9 deletions(-)
> > 
> 
> >      buf = qemu_opt_get_del(opts, BLOCK_OPT_PREALLOC);
> > -    if (!buf || !strcmp(buf, "off")) {
> > -        prealloc = 0;
> > -    } else if (!strcmp(buf, "metadata")) {
> > -        prealloc = 1;
> > -    } else {
> > -        error_setg(errp, "Invalid preallocation mode: '%s'", buf);
> > +    prealloc = qapi_enum_parse(PreallocMode_lookup, buf,
> > +                               PREALLOC_MODE_MAX, PREALLOC_MODE_OFF,
> > +                               &local_err);
> > +    if (local_err) {
> > +        error_propagate(errp, local_err);
> >          ret = -EINVAL;
> >          goto finish;
> >      }
> > @@ -1958,6 +1958,13 @@ static int qcow2_create(const char *filename, 
> > QemuOpts *opts, Error **errp)
> >          flags |= BLOCK_FLAG_LAZY_REFCOUNTS;
> >      }
> >  
> > +    if (prealloc && prealloc != PREALLOC_MODE_METADATA) {
> > +        ret = -EINVAL;
> > +        error_setg(errp, "Unsupported preallocate mode: %s",
> > +                   PreallocMode_lookup[prealloc]);
> > +        goto finish;
> > +    }
> 
> I _still_ think this looks weird, and would be better as either:
> 
> if (prealloc != PREALLOC_MODE_NONE &&
>     prealloc != PREALLOC_MODE_METADATA) {
> 
> to make it obvious that you are filtering for two acceptable modes, or as:
> 
> if (prealloc == PREALLOC_MODE_FALLOC ||
>     prealloc == PREALLOC_MODE_FULL) {
> 
> to make it obvious the modes that you do not support.  But my complaint
> is not strong enough to prevent this patch, especially if later in the
> series revisits this code.

After reading Benoît's comment, I understand why you think the code
looks weird. I'll change it as you suggested. Thanks! 

> 
> Reviewed-by: Eric Blake <address@hidden>
> 
> -- 
> Eric Blake   eblake redhat com    +1-919-301-3266
> Libvirt virtualization library http://libvirt.org
>