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

[Alberto Garcia]

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(_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, _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(_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 

Berto

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

[Markus Armbruster]

Marc-André Lureau  writes:

> Signed-off-by: Marc-André Lureau 
> ---
>  block/quorum.c | 27 ---
>  1 file changed, 8 insertions(+), 19 deletions(-)
>
> diff --git a/block/quorum.c b/block/quorum.c
> index d04da4f430..e4271caa7a 100644
> --- a/block/quorum.c
> +++ b/block/quorum.c
> @@ -22,6 +22,7 @@
>  #include "qapi/qmp/qjson.h"
>  #include "qapi/qmp/qlist.h"
>  #include "qapi/qmp/qstring.h"
> +#include "qapi/util.h"
>  #include "qapi-event.h"
>  #include "crypto/hash.h"
>  
> @@ -867,24 +868,6 @@ static QemuOptsList quorum_runtime_opts = {
>  },
>  };
>  
> -static int parse_read_pattern(const char *opt)
> -{
> -int i;
> -
> -if (!opt) {
> -/* Set quorum as default */
> -return QUORUM_READ_PATTERN_QUORUM;
> -}
> -
> -for (i = 0; i < QUORUM_READ_PATTERN__MAX; i++) {
> -if (!strcmp(opt, QuorumReadPattern_lookup[i])) {
> -return i;
> -}
> -}
> -
> -return -EINVAL;
> -}
> -
>  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(_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, _err);
   if (local_err) {
   goto exit;
   }

If not, we'd have to replace @local_err:

   if (local_err) {
   error_free(local_err);
   error_setg(_err, "Please set read-pattern as fifo or quorum");
   goto exit;
   }

or amend it with error_append_hint() or error_prepend().

Berto, what do you think?

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

[Markus Armbruster]

Alberto Garcia  writes:

> On Tue 22 Aug 2017 03:22:12 PM CEST, Marc-André Lureau wrote:
>
>> @@ -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);
>> +}
>
> I don't like so much that you call qemu_opt_get() twice with the same
> parameters,

Easy enough to avoid.

> but else the change makes sense.

Is that an R-by?  R-by if qemu_opt_get() is called just once?