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 <hu...@cn.fujitsu.com>
> > Reviewed-by: Max Reitz <mre...@redhat.com>
> > Reviewed-by: Kevin Wolf <kw...@redhat.com>
> > ---
> > 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 <ebl...@redhat.com>
>
> --
> Eric Blake eblake redhat com +1-919-301-3266
> Libvirt virtualization library http://libvirt.org
>
