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

[Markus Armbruster]

Jeff Cody <address@hidden> writes:

> On Wed, Aug 10, 2016 at 09:42:04AM +0200, Markus Armbruster wrote:
>> 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.
>
> 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.