| Message ID | 1470736094-19194-1-git-send-email-prasanna.kalever@redhat.com |
|---|---|
| State | New |
| Headers | show |
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.
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?
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.
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
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)", }, {
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(-)