[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-block] [Qemu-devel] [PATCH v2 11/54] quorum: use qapi_enum_par
From: |
Alberto Garcia |
Subject: |
Re: [Qemu-block] [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