qemu-devel
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [Qemu-devel] [PATCH v2 11/54] quorum: use qapi_enum_parse() in quoru


From: Alberto Garcia
Subject: Re: [Qemu-devel] [PATCH v2 11/54] quorum: use qapi_enum_parse() in quorum_open
Date: Wed, 23 Aug 2017 13:53:39 +0200
User-agent: Notmuch/0.18.2 (http://notmuchmail.org) Emacs/24.4.1 (i586-pc-linux-gnu)

On Wed 23 Aug 2017 01:24:09 PM CEST, Markus Armbruster wrote:
>>  static int quorum_open(BlockDriverState *bs, QDict *options, int flags,
>>                         Error **errp)
>>  {
>> @@ -925,7 +908,13 @@ static int quorum_open(BlockDriverState *bs, QDict 
>> *options, int flags,
>>          goto exit;
>>      }
>>  
>> -    ret = parse_read_pattern(qemu_opt_get(opts, QUORUM_OPT_READ_PATTERN));
>> +    if (!qemu_opt_get(opts, QUORUM_OPT_READ_PATTERN)) {
>> +        ret = QUORUM_READ_PATTERN_QUORUM;
>> +    } else {
>> +        ret = qapi_enum_parse(QuorumReadPattern_lookup,
>> +                              qemu_opt_get(opts, QUORUM_OPT_READ_PATTERN),
>> +                              QUORUM_READ_PATTERN__MAX, -EINVAL, NULL);
>> +    }
>>      if (ret < 0) {
>>          error_setg(&local_err, "Please set read-pattern as fifo or quorum");
>>          goto exit;
>
> qapi_enum_parse() copes with null input: it returns its fourth argument
> then.  I don't like that also returns it on error, but that shouldn't be
> a problem here.
>
> If we can live with qapi_enum_parse()'s sub-par error message, then this
> should do:
>
>        ret = qapi_enum_parse(QuorumReadPattern_lookup,
>                              qemu_opt_get(opts, QUORUM_OPT_READ_PATTERN),
>                              QUORUM_READ_PATTERN__MAX,
>                              QUORUM_READ_PATTERN_QUORUM, &local_err);
>        if (local_err) {
>            goto exit;
>        }

That actually doesn't sound so bad.

> If not, we'd have to replace @local_err:
>
>        if (local_err) {
>            error_free(local_err);
>            error_setg(&local_err, "Please set read-pattern as fifo or 
> quorum");
>            goto exit;
>        }

Instead of replacing @local_err I'd rather have an intermediate variable
like this:

    pattern_str = qemu_opt_get(opts, QUORUM_OPT_READ_PATTERN); 
    if (!pattern_str) {
        ret = QUORUM_READ_PATTERN_QUORUM;
    } else {
        ret = qapi_enum_parse(QuorumReadPattern_lookup, pattern_str,
                              QUORUM_READ_PATTERN__MAX, -EINVAL, NULL);
    }

I think I prefer this last one the best, but using the default error
message is also ok. With either solution,

Reviewed-by: Alberto Garcia <address@hidden>

Berto



reply via email to

[Prev in Thread] Current Thread [Next in Thread]