[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH] qapi-schema: mark InetSocketAddress as mandator
From: |
Daniel P. Berrange |
Subject: |
Re: [Qemu-devel] [PATCH] qapi-schema: mark InetSocketAddress as mandatory again |
Date: |
Thu, 22 Oct 2015 13:42:15 +0100 |
User-agent: |
Mutt/1.5.24 (2015-08-30) |
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 <address@hidden>
> > 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 <address@hidden>
> > Signed-off-by: Daniel P. Berrange <address@hidden>
> > ---
> > 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 :|