Re: [Qemu-devel] [PATCH v1 1/1] block/gluster: fix port type in the QAPI options list

[Markus Armbruster]

Prasanna Kumar Kalever <address@hidden> writes:

> After introduction of qapi schema in gluster block driver code, the port
> type is now string as per InetSocketAddress
>
> { 'struct': 'InetSocketAddress',
>   'data': {
>     'host': 'str',
>     'port': 'str',
>     '*to': 'uint16',
>     '*ipv4': 'bool',
>     '*ipv6': 'bool' } }
>
> but the current code still treats it as QEMU_OPT_NUMBER, hence fixing port
> to accept QEMU_OPT_STRING.
>
> Credits: Markus Armbruster <address@hidden>

Commonly written as
Suggested-by: Markus Armbruster <address@hidden>

> Signed-off-by: Prasanna Kumar Kalever <address@hidden>
> ---
>  block/gluster.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/block/gluster.c b/block/gluster.c
> index edde1ad..e6afa48 100644
> --- a/block/gluster.c
> +++ b/block/gluster.c
> @@ -161,7 +161,7 @@ static QemuOptsList runtime_tcp_opts = {
>          },
>          {
>              .name = GLUSTER_OPT_PORT,
> -            .type = QEMU_OPT_NUMBER,
> +            .type = QEMU_OPT_STRING,
>              .help = "port number on which glusterd is listening (default 
> 24007)",
>          },
>          {

The difference between QEMU_OPT_NUMBER and QEMU_OPT_STRING:

* The string value is stored for both.  For QEMU_OPT_NUMBER, we
  additionally parse the string as decimal number (this can fail,
  obviously), and store the result as uint64_t.  See qemu_opt_parse().

* qemu_opt_get() & friends return the stored string for both.

* qemu_opt_get_number() & friends require QEMU_OPT_NUMBER and return the
  stored number.

* qemu_opts_print() prints the stored string (with comma doubled) for
  QEMU_OPT_STRING, and the stored number for QEMU_OPT_NUMBER.

Your patch works, because:

* We get the value only with qemu_opt_get().  The only effect we get
  from QEMU_OPT_NUMBER is qemu_opt_parse() failure.

* "[PATCH v2 1/1] block/gluster: improve defense over string to int
  conversion" fixes the conversion port string to port number to detect
  errors.  With QEMU_OPT_NUMBER, this can't actually fail, because
  qemu_opt_parse() fails first.  With QEMU_OPT_STRING, it can.

The commit message should explain this.

I'd squash the two patches together, because a decent commit message for
the squash will probably be simpler than separate ones.