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 <be...@igalia.com> Berto