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

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

[Prasanna Kumar Kalever]

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 <armbru@redhat.com>

Signed-off-by: Prasanna Kumar Kalever <prasanna.kalever@redhat.com>
---
 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)",
         },
         {
-- 
2.7.4

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

[Markus Armbruster]

Prasanna Kumar Kalever <prasanna.kalever@redhat.com> 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 <armbru@redhat.com>

Commonly written as
Suggested-by: Markus Armbruster <armbru@redhat.com>

> Signed-off-by: Prasanna Kumar Kalever <prasanna.kalever@redhat.com>
> ---
>  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.

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

[Jeff Cody]

On Wed, Aug 10, 2016 at 09:42:04AM +0200, Markus Armbruster wrote:
> Prasanna Kumar Kalever <prasanna.kalever@redhat.com> 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 <armbru@redhat.com>
> 
> Commonly written as
> Suggested-by: Markus Armbruster <armbru@redhat.com>
> 
> > Signed-off-by: Prasanna Kumar Kalever <prasanna.kalever@redhat.com>
> > ---
> >  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.

Are these two patches intended for 2.7?

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

[Markus Armbruster]

Jeff Cody <jcody@redhat.com> writes:

> On Wed, Aug 10, 2016 at 09:42:04AM +0200, Markus Armbruster wrote:
>> Prasanna Kumar Kalever <prasanna.kalever@redhat.com> 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 <armbru@redhat.com>
>> 
>> Commonly written as
>> Suggested-by: Markus Armbruster <armbru@redhat.com>
>> 
>> > Signed-off-by: Prasanna Kumar Kalever <prasanna.kalever@redhat.com>
>> > ---
>> >  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.
>
> Are these two patches intended for 2.7?

Back when I suggested this work, I had hoped it could still make 2.7 as
a followup fix, but 1. the bug has turned out to be merely latent , and
2. it's -rc3.  I guess we need to punt them to 2.8.

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

[Jeff Cody]

On Tue, Aug 09, 2016 at 03:18:14PM +0530, Prasanna Kumar Kalever wrote:
> 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 <armbru@redhat.com>
> 
> Signed-off-by: Prasanna Kumar Kalever <prasanna.kalever@redhat.com>
> ---
>  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)",
>          },
>          {
> -- 
> 2.7.4
> 

Thanks,

Applied to my block branch:

git://github.com/codyprime/qemu-kvm-jtc.git block

[Changed 'Credits' to 'Suggested-by']

-Jeff