[Qemu-devel] [PATCH] qapi-schema: mark InetSocketAddress as mandatory again

[Qemu-devel] [PATCH] qapi-schema: mark InetSocketAddress as mandatory again

[Daniel P. Berrange]

Revert the qapi-schema.json change done in:

  commit 0983f5e6af76d5df8c6346cbdfff9d8305fb6da0
  Author: Daniel P. Berrange <berrange@redhat.com>
  Date:   Tue Sep 1 14:46:50 2015 +0100

    sockets: allow port to be NULL when listening on IP address

Switching "port" from mandatory to optional causes the QAPI
code generator to add a 'has_port' field to the InetSocketAddress
struct. No code that created InetSocketAddress objects was updated
to set 'has_port = true', which caused the non-NULL port strings
to be silently dropped when copying InetSocketAddress objects.

Reported-by: Knut Omang <knuto@ifi.uio.no>
Signed-off-by: Daniel P. Berrange <berrange@redhat.com>
---
 qapi-schema.json | 6 ++----
 1 file changed, 2 insertions(+), 4 deletions(-)

diff --git a/qapi-schema.json b/qapi-schema.json
index f60be29..702b7b5 100644
--- a/qapi-schema.json
+++ b/qapi-schema.json
@@ -2614,9 +2614,7 @@
 #
 # @host: host part of the address
 #
-# @port: port part of the address, or lowest port if @to is present.
-#        Kernel selects a free port if omitted for listener addresses.
-#        #optional
+# @port: port part of the address, or lowest port if @to is present
 #
 # @to: highest port to try
 #
@@ -2631,7 +2629,7 @@
 { 'struct': 'InetSocketAddress',
   'data': {
     'host': 'str',
-    '*port': 'str',
+    'port': 'str',
     '*to': 'uint16',
     '*ipv4': 'bool',
     '*ipv6': 'bool' } }
-- 
2.4.3

Re: [Qemu-devel] [PATCH] qapi-schema: mark InetSocketAddress as mandatory again

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 2107 bytes --]

On 10/22/2015 04:25 AM, Daniel P. Berrange wrote:
> Revert the qapi-schema.json change done in:
> 
>   commit 0983f5e6af76d5df8c6346cbdfff9d8305fb6da0
>   Author: Daniel P. Berrange <berrange@redhat.com>
>   Date:   Tue Sep 1 14:46:50 2015 +0100
> 
>     sockets: allow port to be NULL when listening on IP address
> 
> Switching "port" from mandatory to optional causes the QAPI
> code generator to add a 'has_port' field to the InetSocketAddress
> struct. No code that created InetSocketAddress objects was updated
> to set 'has_port = true', which caused the non-NULL port strings
> to be silently dropped when copying InetSocketAddress objects.
> 
> Reported-by: Knut Omang <knuto@ifi.uio.no>
> Signed-off-by: Daniel P. Berrange <berrange@redhat.com>
> ---
>  qapi-schema.json | 6 ++----
>  1 file changed, 2 insertions(+), 4 deletions(-)

Yes, I have plans for a qapi improvement that will let us mark strings
where we want the convention of memb == NULL rather than has_memb ==
false (flipping the switch globally would touch too much code in one
commit, so a marker saying which structs are okay with the idea will
make it possible to convert smaller portions at a time - although we may
want to do the cleanup over the entire tree and then drop the marker).
But since I haven't posted patches on that front yet, it's missed
softfreeze and will be something for 2.6.

So this patch is necessary for 2.5.

> 
> diff --git a/qapi-schema.json b/qapi-schema.json
> index f60be29..702b7b5 100644
> --- a/qapi-schema.json
> +++ b/qapi-schema.json
> @@ -2614,9 +2614,7 @@
>  #
>  # @host: host part of the address
>  #
> -# @port: port part of the address, or lowest port if @to is present.
> -#        Kernel selects a free port if omitted for listener addresses.
> -#        #optional
> +# @port: port part of the address, or lowest port if @to is present

Don't you still want to allow an empty string as the way to allow kernel
port selection?

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]

Re: [Qemu-devel] [PATCH] qapi-schema: mark InetSocketAddress as mandatory again

[Daniel P. Berrange]

On Thu, Oct 22, 2015 at 06:22:29AM -0600, Eric Blake wrote:
> On 10/22/2015 04:25 AM, Daniel P. Berrange wrote:
> > Revert the qapi-schema.json change done in:
> > 
> >   commit 0983f5e6af76d5df8c6346cbdfff9d8305fb6da0
> >   Author: Daniel P. Berrange <berrange@redhat.com>
> >   Date:   Tue Sep 1 14:46:50 2015 +0100
> > 
> >     sockets: allow port to be NULL when listening on IP address
> > 
> > Switching "port" from mandatory to optional causes the QAPI
> > code generator to add a 'has_port' field to the InetSocketAddress
> > struct. No code that created InetSocketAddress objects was updated
> > to set 'has_port = true', which caused the non-NULL port strings
> > to be silently dropped when copying InetSocketAddress objects.
> > 
> > Reported-by: Knut Omang <knuto@ifi.uio.no>
> > Signed-off-by: Daniel P. Berrange <berrange@redhat.com>
> > ---
> >  qapi-schema.json | 6 ++----
> >  1 file changed, 2 insertions(+), 4 deletions(-)
> 
> Yes, I have plans for a qapi improvement that will let us mark strings
> where we want the convention of memb == NULL rather than has_memb ==
> false (flipping the switch globally would touch too much code in one
> commit, so a marker saying which structs are okay with the idea will
> make it possible to convert smaller portions at a time - although we may
> want to do the cleanup over the entire tree and then drop the marker).
> But since I haven't posted patches on that front yet, it's missed
> softfreeze and will be something for 2.6.
> 
> So this patch is necessary for 2.5.
> 
> > 
> > diff --git a/qapi-schema.json b/qapi-schema.json
> > index f60be29..702b7b5 100644
> > --- a/qapi-schema.json
> > +++ b/qapi-schema.json
> > @@ -2614,9 +2614,7 @@
> >  #
> >  # @host: host part of the address
> >  #
> > -# @port: port part of the address, or lowest port if @to is present.
> > -#        Kernel selects a free port if omitted for listener addresses.
> > -#        #optional
> > +# @port: port part of the address, or lowest port if @to is present
> 
> Don't you still want to allow an empty string as the way to allow kernel
> port selection?

I only needed the support for empty port for the sake of the unit
tests, which doesn't use QAPI parsing code. Enabling it via the command
line / monitor was just a happy accident, so I figure this is fine for
2.5. We can re-revert it in 2.6 when you change the QAPI generator

Regards,
Daniel
-- 
|: http://berrange.com      -o-    http://www.flickr.com/photos/dberrange/ :|
|: http://libvirt.org              -o-             http://virt-manager.org :|
|: http://autobuild.org       -o-         http://search.cpan.org/~danberr/ :|
|: http://entangle-photo.org       -o-       http://live.gnome.org/gtk-vnc :|

Re: [Qemu-devel] [PATCH] qapi-schema: mark InetSocketAddress as mandatory again

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 998 bytes --]

On 10/22/2015 06:42 AM, Daniel P. Berrange wrote:

>>> +++ b/qapi-schema.json
>>> @@ -2614,9 +2614,7 @@
>>>  #
>>>  # @host: host part of the address
>>>  #
>>> -# @port: port part of the address, or lowest port if @to is present.
>>> -#        Kernel selects a free port if omitted for listener addresses.
>>> -#        #optional
>>> +# @port: port part of the address, or lowest port if @to is present
>>
>> Don't you still want to allow an empty string as the way to allow kernel
>> port selection?
> 
> I only needed the support for empty port for the sake of the unit
> tests, which doesn't use QAPI parsing code. Enabling it via the command
> line / monitor was just a happy accident, so I figure this is fine for
> 2.5. We can re-revert it in 2.6 when you change the QAPI generator

Fair enough; then you can add:
Reviewed-by: Eric Blake <eblake@redhat.com>

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]

Re: [Qemu-devel] [PATCH] qapi-schema: mark InetSocketAddress as mandatory again

[Markus Armbruster]

"Daniel P. Berrange" <berrange@redhat.com> writes:

> Revert the qapi-schema.json change done in:
>
>   commit 0983f5e6af76d5df8c6346cbdfff9d8305fb6da0
>   Author: Daniel P. Berrange <berrange@redhat.com>
>   Date:   Tue Sep 1 14:46:50 2015 +0100
>
>     sockets: allow port to be NULL when listening on IP address
>
> Switching "port" from mandatory to optional causes the QAPI
> code generator to add a 'has_port' field to the InetSocketAddress
> struct. No code that created InetSocketAddress objects was updated
> to set 'has_port = true', which caused the non-NULL port strings
> to be silently dropped when copying InetSocketAddress objects.
>
> Reported-by: Knut Omang <knuto@ifi.uio.no>
> Signed-off-by: Daniel P. Berrange <berrange@redhat.com>

I'll take this through my tree before even more people get bitten.