Patchwork [v2,2/9] qapi: add socket address types

login
register
mail settings
Submitter Paolo Bonzini
Date Oct. 1, 2012, 2:52 p.m.
Message ID <1349103144-6827-3-git-send-email-pbonzini@redhat.com>
Download mbox | patch
Permalink /patch/188315/
State New
Headers show

Comments

Paolo Bonzini - Oct. 1, 2012, 2:52 p.m.
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 qapi-schema.json | 53 +++++++++++++++++++++++++++++++++++++++++++++++++++++
 1 file modificato, 53 inserzioni(+)
Eric Blake - Oct. 1, 2012, 11:56 p.m.
On 10/01/2012 08:52 AM, Paolo Bonzini wrote:
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
>  qapi-schema.json | 53 +++++++++++++++++++++++++++++++++++++++++++++++++++++
>  1 file modificato, 53 inserzioni(+)
> 
> diff --git a/qapi-schema.json b/qapi-schema.json
> index 191d921..b443a99 100644
> --- a/qapi-schema.json
> +++ b/qapi-schema.json
> @@ -2367,6 +2367,59 @@
>      'opts': 'NetClientOptions' } }
>  
>  ##
> +# @IPSocketAddress
> +#
> +# Captures a range of possible destination addresses for an IP socket
> +#
> +# @host: host part of the address
> +#
> +# @port: port part of the address, or lowest port if @to is present
> +#
> +# @to: highest port to try
> +#
> +# @ipv4: whether to accept IPv4 addresses, default try both IPv4 and IPv6
> +#        #optional
> +#
> +# @ipv6: whether to accept IPv6 addresses, default try both IPv4 and IPv6
> +#        #optional
> +#
> +# Since 1.3
> +##
> +{ 'type': 'IPSocketAddress',
> +  'data': {
> +    'host': 'str',
> +    'port': 'str',
> +    '*to': 'uint16',

I still think it's a bit weird to have:

'host':'localhost', 'port':'1000', 'to':1001

for a port range, all because of the possibility of named ports; should
'*to' be a 'str' if only for symmetry in the output?  But it's
bike-shedding, so I'll live with whatever works (that is, I'm not
requesting a v3 on this patch).

> +##
> +# @SocketAddress
> +#
> +# Captures the address of a socket, which could also be a named file descriptor
> +#
> +# Since 1.3
> +##
> +{ 'union': 'SocketAddress',
> +  'data': {
> +    'inet': 'IPSocketAddress',
> +    'unix': 'UnixSocketAddress',
> +    'fd': 'String' } }

I guess you had to use the String wrapper instead of 'str'; but a bit of
reading in the file showed that this part is at least correct.

Reviewed-by: Eric Blake <eblake@redhat.com>
Paolo Bonzini - Oct. 2, 2012, 9 a.m.
Il 02/10/2012 01:56, Eric Blake ha scritto:
> On 10/01/2012 08:52 AM, Paolo Bonzini wrote:
>> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
>> ---
>>  qapi-schema.json | 53 +++++++++++++++++++++++++++++++++++++++++++++++++++++
>>  1 file modificato, 53 inserzioni(+)
>>
>> diff --git a/qapi-schema.json b/qapi-schema.json
>> index 191d921..b443a99 100644
>> --- a/qapi-schema.json
>> +++ b/qapi-schema.json
>> @@ -2367,6 +2367,59 @@
>>      'opts': 'NetClientOptions' } }
>>  
>>  ##
>> +# @IPSocketAddress
>> +#
>> +# Captures a range of possible destination addresses for an IP socket
>> +#
>> +# @host: host part of the address
>> +#
>> +# @port: port part of the address, or lowest port if @to is present
>> +#
>> +# @to: highest port to try
>> +#
>> +# @ipv4: whether to accept IPv4 addresses, default try both IPv4 and IPv6
>> +#        #optional
>> +#
>> +# @ipv6: whether to accept IPv6 addresses, default try both IPv4 and IPv6
>> +#        #optional
>> +#
>> +# Since 1.3
>> +##
>> +{ 'type': 'IPSocketAddress',
>> +  'data': {
>> +    'host': 'str',
>> +    'port': 'str',
>> +    '*to': 'uint16',
> 
> I still think it's a bit weird to have:
> 
> 'host':'localhost', 'port':'1000', 'to':1001
> 
> for a port range, all because of the possibility of named ports; should
> '*to' be a 'str' if only for symmetry in the output?  But it's
> bike-shedding, so I'll live with whatever works (that is, I'm not
> requesting a v3 on this patch).

Would it be better if I changed 'to' to 'count'?

>> +##
>> +# @SocketAddress
>> +#
>> +# Captures the address of a socket, which could also be a named file descriptor
>> +#
>> +# Since 1.3
>> +##
>> +{ 'union': 'SocketAddress',
>> +  'data': {
>> +    'inet': 'IPSocketAddress',
>> +    'unix': 'UnixSocketAddress',
>> +    'fd': 'String' } }
> 
> I guess you had to use the String wrapper instead of 'str'; but a bit of
> reading in the file showed that this part is at least correct.

It's not absolutely necessary, but I preferred it that way.

Paolo
Eric Blake - Oct. 2, 2012, 11:39 a.m.
On 10/02/2012 03:00 AM, Paolo Bonzini wrote:
> Il 02/10/2012 01:56, Eric Blake ha scritto:
>> On 10/01/2012 08:52 AM, Paolo Bonzini wrote:
>>> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
>>> ---

>>> +#
>>> +# @port: port part of the address, or lowest port if @to is present
>>> +#
>>> +# @to: highest port to try
>>> +#

>>> +{ 'type': 'IPSocketAddress',
>>> +  'data': {
>>> +    'host': 'str',
>>> +    'port': 'str',
>>> +    '*to': 'uint16',
>>
>> I still think it's a bit weird to have:
>>
>> 'host':'localhost', 'port':'1000', 'to':1001
>>
>> for a port range, all because of the possibility of named ports; should
>> '*to' be a 'str' if only for symmetry in the output?  But it's
>> bike-shedding, so I'll live with whatever works (that is, I'm not
>> requesting a v3 on this patch).
> 
> Would it be better if I changed 'to' to 'count'?

That does look a little better:

'host':'localhost', 'port':'1000', 'count':2

for the 2-port range 1000-1001.  But it's all the same information, so
I'm not strongly tied to any particular representation, as long as
libvirt can parse it when querying and produce it when starting NBD.
Luiz Capitulino - Oct. 2, 2012, 12:27 p.m.
On Tue, 02 Oct 2012 05:39:52 -0600
Eric Blake <eblake@redhat.com> wrote:

> On 10/02/2012 03:00 AM, Paolo Bonzini wrote:
> > Il 02/10/2012 01:56, Eric Blake ha scritto:
> >> On 10/01/2012 08:52 AM, Paolo Bonzini wrote:
> >>> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> >>> ---
> 
> >>> +#
> >>> +# @port: port part of the address, or lowest port if @to is present
> >>> +#
> >>> +# @to: highest port to try
> >>> +#
> 
> >>> +{ 'type': 'IPSocketAddress',
> >>> +  'data': {
> >>> +    'host': 'str',
> >>> +    'port': 'str',
> >>> +    '*to': 'uint16',
> >>
> >> I still think it's a bit weird to have:
> >>
> >> 'host':'localhost', 'port':'1000', 'to':1001
> >>
> >> for a port range, all because of the possibility of named ports; should
> >> '*to' be a 'str' if only for symmetry in the output?  But it's
> >> bike-shedding, so I'll live with whatever works (that is, I'm not
> >> requesting a v3 on this patch).
> > 
> > Would it be better if I changed 'to' to 'count'?
> 
> That does look a little better:
> 
> 'host':'localhost', 'port':'1000', 'count':2
> 
> for the 2-port range 1000-1001.  But it's all the same information, so
> I'm not strongly tied to any particular representation, as long as
> libvirt can parse it when querying and produce it when starting NBD.

Wouldn't it be cleaner to pass a list of port numbers? We could have:

 *port-list: [ 'int' ]
 *service: 'str'

Or, we could make this an union.
Luiz Capitulino - Oct. 2, 2012, 12:32 p.m.
On Mon,  1 Oct 2012 16:52:17 +0200
Paolo Bonzini <pbonzini@redhat.com> wrote:

> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>

Made a suggestion for this one that I think it's worth doing, but
as Eric says I'll avoid bike shed, so:

Acked-by: Luiz Capitulino <lcapitulino@redhat.com>

> ---
>  qapi-schema.json | 53 +++++++++++++++++++++++++++++++++++++++++++++++++++++
>  1 file modificato, 53 inserzioni(+)
> 
> diff --git a/qapi-schema.json b/qapi-schema.json
> index 191d921..b443a99 100644
> --- a/qapi-schema.json
> +++ b/qapi-schema.json
> @@ -2367,6 +2367,59 @@
>      'opts': 'NetClientOptions' } }
>  
>  ##
> +# @IPSocketAddress
> +#
> +# Captures a range of possible destination addresses for an IP socket
> +#
> +# @host: host part of the address
> +#
> +# @port: port part of the address, or lowest port if @to is present
> +#
> +# @to: highest port to try
> +#
> +# @ipv4: whether to accept IPv4 addresses, default try both IPv4 and IPv6
> +#        #optional
> +#
> +# @ipv6: whether to accept IPv6 addresses, default try both IPv4 and IPv6
> +#        #optional
> +#
> +# Since 1.3
> +##
> +{ 'type': 'IPSocketAddress',
> +  'data': {
> +    'host': 'str',
> +    'port': 'str',
> +    '*to': 'uint16',
> +    '*ipv4': 'bool',
> +    '*ipv6': 'bool' } }
> +
> +##
> +# @UnixSocketAddress
> +#
> +# Captures the destination address of a Unix socket
> +#
> +# @path: filesystem path to use
> +#
> +# Since 1.3
> +##
> +{ 'type': 'UnixSocketAddress',
> +  'data': {
> +    'path': 'str' } }
> +
> +##
> +# @SocketAddress
> +#
> +# Captures the address of a socket, which could also be a named file descriptor
> +#
> +# Since 1.3
> +##
> +{ 'union': 'SocketAddress',
> +  'data': {
> +    'inet': 'IPSocketAddress',
> +    'unix': 'UnixSocketAddress',
> +    'fd': 'String' } }
> +
> +##
>  # @getfd:
>  #
>  # Receive a file descriptor via SCM rights and assign it a name
Paolo Bonzini - Oct. 2, 2012, 2:24 p.m.
Il 02/10/2012 14:27, Luiz Capitulino ha scritto:
>>>> > >> for a port range, all because of the possibility of named ports; should
>>>> > >> '*to' be a 'str' if only for symmetry in the output?  But it's
>>>> > >> bike-shedding, so I'll live with whatever works (that is, I'm not
>>>> > >> requesting a v3 on this patch).
>>> > > 
>>> > > Would it be better if I changed 'to' to 'count'?
>> > 
>> > That does look a little better:
>> > 
>> > 'host':'localhost', 'port':'1000', 'count':2
>> > 
>> > for the 2-port range 1000-1001.  But it's all the same information, so
>> > I'm not strongly tied to any particular representation, as long as
>> > libvirt can parse it when querying and produce it when starting NBD.
> Wouldn't it be cleaner to pass a list of port numbers? We could have:
> 
>  *port-list: [ 'int' ]
>  *service: 'str'

A list of ports doesn't work too well for say 5900-5999.  I think the
port + count is the simplest.

Paolo
Luiz Capitulino - Oct. 2, 2012, 3:27 p.m.
On Tue, 02 Oct 2012 16:24:48 +0200
Paolo Bonzini <pbonzini@redhat.com> wrote:

> Il 02/10/2012 14:27, Luiz Capitulino ha scritto:
> >>>> > >> for a port range, all because of the possibility of named ports; should
> >>>> > >> '*to' be a 'str' if only for symmetry in the output?  But it's
> >>>> > >> bike-shedding, so I'll live with whatever works (that is, I'm not
> >>>> > >> requesting a v3 on this patch).
> >>> > > 
> >>> > > Would it be better if I changed 'to' to 'count'?
> >> > 
> >> > That does look a little better:
> >> > 
> >> > 'host':'localhost', 'port':'1000', 'count':2
> >> > 
> >> > for the 2-port range 1000-1001.  But it's all the same information, so
> >> > I'm not strongly tied to any particular representation, as long as
> >> > libvirt can parse it when querying and produce it when starting NBD.
> > Wouldn't it be cleaner to pass a list of port numbers? We could have:
> > 
> >  *port-list: [ 'int' ]
> >  *service: 'str'
> 
> A list of ports doesn't work too well for say 5900-5999.  I think the
> port + count is the simplest.

True, but consider making it a union then, so that we can have service
as a string and port as an integer.
Paolo Bonzini - Oct. 2, 2012, 3:31 p.m.
Il 02/10/2012 17:27, Luiz Capitulino ha scritto:
>>> > > Wouldn't it be cleaner to pass a list of port numbers? We could have:
>>> > > 
>>> > >  *port-list: [ 'int' ]
>>> > >  *service: 'str'
>> > 
>> > A list of ports doesn't work too well for say 5900-5999.  I think the
>> > port + count is the simplest.
> True, but consider making it a union then, so that we can have service
> as a string and port as an integer.

But then how would you name the containing union?

In the end this is a hack for VNC or little more... any change will
propagate all over the place due to QemuOpts and qemu-char using "to".
It is an optional argument, I don't think it's worth much time. :)

Paolo

Patch

diff --git a/qapi-schema.json b/qapi-schema.json
index 191d921..b443a99 100644
--- a/qapi-schema.json
+++ b/qapi-schema.json
@@ -2367,6 +2367,59 @@ 
     'opts': 'NetClientOptions' } }
 
 ##
+# @IPSocketAddress
+#
+# Captures a range of possible destination addresses for an IP socket
+#
+# @host: host part of the address
+#
+# @port: port part of the address, or lowest port if @to is present
+#
+# @to: highest port to try
+#
+# @ipv4: whether to accept IPv4 addresses, default try both IPv4 and IPv6
+#        #optional
+#
+# @ipv6: whether to accept IPv6 addresses, default try both IPv4 and IPv6
+#        #optional
+#
+# Since 1.3
+##
+{ 'type': 'IPSocketAddress',
+  'data': {
+    'host': 'str',
+    'port': 'str',
+    '*to': 'uint16',
+    '*ipv4': 'bool',
+    '*ipv6': 'bool' } }
+
+##
+# @UnixSocketAddress
+#
+# Captures the destination address of a Unix socket
+#
+# @path: filesystem path to use
+#
+# Since 1.3
+##
+{ 'type': 'UnixSocketAddress',
+  'data': {
+    'path': 'str' } }
+
+##
+# @SocketAddress
+#
+# Captures the address of a socket, which could also be a named file descriptor
+#
+# Since 1.3
+##
+{ 'union': 'SocketAddress',
+  'data': {
+    'inet': 'IPSocketAddress',
+    'unix': 'UnixSocketAddress',
+    'fd': 'String' } }
+
+##
 # @getfd:
 #
 # Receive a file descriptor via SCM rights and assign it a name