[Qemu-devel] [PATCH v2 0/9] Embedded NBD server

[Qemu-devel] [PATCH v2 0/9] Embedded NBD server

[Paolo Bonzini]

This series rebases the previous qemu-sockets patches for error
propagation and uses the new QAPI-friendly socket functions in the
embedded NBD server.  The changes are due to Orit's patches being
now in, some early parts being in Luiz's queue, and glusterfs
patches not having touched qemu-sockets.c in the end.

Patches 1 to 4 start moving qemu-sockets functions away from error_report
(or printf) and away from QemuOpts.  Patch 5 makes it easier to reuse
the address parser of inet_parse in the new socket_parse function.
Patch 6 introduces QAPI-friendly socket parsing and creation functions.

Patches 7 and 8 introduces the QMP commands, and patch 9 introduces the
HMP version.

Paolo

Paolo Bonzini (9):
  build: add QAPI files to the tools
  qapi: add socket address types
  qemu-sockets: add error propagation to inet_parse
  qemu-sockets: add error propagation to Unix socket functions
  qemu-sockets: return IPSocketAddress from inet_parse
  qemu-sockets: add socket_listen, socket_connect, socket_parse
  block: add close notifiers
  qmp: add NBD server commands
  hmp: add NBD server commands

 Makefile.objs       |   8 +-
 block.c             |  19 +++-
 block.h             |   1 +
 block_int.h         |   2 +
 blockdev-nbd.c      | 119 ++++++++++++++++++++++++
 hmp-commands.hx     |  29 ++++++
 hmp.c               |  55 +++++++++++
 hmp.h               |   2 +
 nbd.c               |   4 +-
 qapi-schema.json    |  96 +++++++++++++++++++
 qemu-char.c         |   4 +-
 qemu-sockets.c      | 261 +++++++++++++++++++++++++++++++++++++++-------------
 qemu-tool.c         |   6 ++
 qemu_socket.h       |  12 ++-
 qga/channel-posix.c |   2 +-
 qmp-commands.hx     |  16 ++++
 ui/vnc.c            |   4 +-
 17 file modificati, 557 inserzioni(+), 83 rimozioni(-)
 create mode 100644 blockdev-nbd.c

-- 
1.7.12

[Qemu-devel] [PATCH v2 1/9] build: add QAPI files to the tools

[Paolo Bonzini]

We need them because qemu-sockets will soon be using SocketAddress.

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 Makefile.objs | 3 ++-
 1 file modificato, 2 inserzioni(+). 1 rimozione(-)

diff --git a/Makefile.objs b/Makefile.objs
index 4412757..03da150 100644
--- a/Makefile.objs
+++ b/Makefile.objs
@@ -47,6 +47,7 @@ block-obj-y += $(coroutine-obj-y) $(qobject-obj-y) $(version-obj-y)
 block-obj-$(CONFIG_POSIX) += posix-aio-compat.o
 block-obj-$(CONFIG_LINUX_AIO) += linux-aio.o
 block-obj-y += block/
+block-obj-y += $(qapi-obj-y) qapi-types.o qapi-visit.o
 
 ifeq ($(CONFIG_VIRTIO)$(CONFIG_VIRTFS)$(CONFIG_PCI),yyy)
 # Lots of the fsdev/9pcode is pulled in by vl.c via qemu_fsdev_add.
@@ -241,9 +242,9 @@ QEMU_CFLAGS+=$(GLIB_CFLAGS)
 nested-vars += \
 	hw-obj-y \
 	qga-obj-y \
-	block-obj-y \
 	qom-obj-y \
 	qapi-obj-y \
+	block-obj-y \
 	user-obj-y \
 	common-obj-y \
 	extra-obj-y
-- 
1.7.12

Re: [Qemu-devel] [PATCH v2 1/9] build: add QAPI files to the tools

[Luiz Capitulino]

On Mon,  1 Oct 2012 16:52:16 +0200
Paolo Bonzini <pbonzini@redhat.com> wrote:

> We need them because qemu-sockets will soon be using SocketAddress.
> 
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>

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

> ---
>  Makefile.objs | 3 ++-
>  1 file modificato, 2 inserzioni(+). 1 rimozione(-)
> 
> diff --git a/Makefile.objs b/Makefile.objs
> index 4412757..03da150 100644
> --- a/Makefile.objs
> +++ b/Makefile.objs
> @@ -47,6 +47,7 @@ block-obj-y += $(coroutine-obj-y) $(qobject-obj-y) $(version-obj-y)
>  block-obj-$(CONFIG_POSIX) += posix-aio-compat.o
>  block-obj-$(CONFIG_LINUX_AIO) += linux-aio.o
>  block-obj-y += block/
> +block-obj-y += $(qapi-obj-y) qapi-types.o qapi-visit.o
>  
>  ifeq ($(CONFIG_VIRTIO)$(CONFIG_VIRTFS)$(CONFIG_PCI),yyy)
>  # Lots of the fsdev/9pcode is pulled in by vl.c via qemu_fsdev_add.
> @@ -241,9 +242,9 @@ QEMU_CFLAGS+=$(GLIB_CFLAGS)
>  nested-vars += \
>  	hw-obj-y \
>  	qga-obj-y \
> -	block-obj-y \
>  	qom-obj-y \
>  	qapi-obj-y \
> +	block-obj-y \
>  	user-obj-y \
>  	common-obj-y \
>  	extra-obj-y

[Qemu-devel] [PATCH v2 2/9] qapi: add socket address types

[Paolo Bonzini]

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',
+    '*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
-- 
1.7.12

Re: [Qemu-devel] [PATCH v2 2/9] qapi: add socket address types

[Eric Blake]

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

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>

-- 
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: 617 bytes --]

Re: [Qemu-devel] [PATCH v2 2/9] qapi: add socket address types

[Paolo Bonzini]

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

Re: [Qemu-devel] [PATCH v2 2/9] qapi: add socket address types

[Eric Blake]

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

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.

-- 
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: 617 bytes --]

Re: [Qemu-devel] [PATCH v2 2/9] qapi: add socket address types

[Luiz Capitulino]

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.

Re: [Qemu-devel] [PATCH v2 2/9] qapi: add socket address types

[Paolo Bonzini]

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

Re: [Qemu-devel] [PATCH v2 2/9] qapi: add socket address types

[Luiz Capitulino]

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.

Re: [Qemu-devel] [PATCH v2 2/9] qapi: add socket address types

[Paolo Bonzini]

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

Re: [Qemu-devel] [PATCH v2 2/9] qapi: add socket address types

[Luiz Capitulino]

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

[Qemu-devel] [PATCH v2 3/9] qemu-sockets: add error propagation to inet_parse

[Paolo Bonzini]

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 qemu-sockets.c | 41 +++++++++++++++++++++--------------------
 1 file modificato, 21 inserzioni(+), 20 rimozioni(-)

diff --git a/qemu-sockets.c b/qemu-sockets.c
index 1f14e8b..bf1f794 100644
--- a/qemu-sockets.c
+++ b/qemu-sockets.c
@@ -511,7 +511,7 @@ err:
 }
 
 /* compatibility wrapper */
-static int inet_parse(QemuOpts *opts, const char *str)
+static void inet_parse(QemuOpts *opts, const char *str, Error **errp)
 {
     const char *optstr, *h;
     char addr[64];
@@ -523,32 +523,28 @@ static int inet_parse(QemuOpts *opts, const char *str)
         /* no host given */
         addr[0] = '\0';
         if (1 != sscanf(str,":%32[^,]%n",port,&pos)) {
-            fprintf(stderr, "%s: portonly parse error (%s)\n",
-                    __FUNCTION__, str);
-            return -1;
+            error_setg(errp, "error parsing port in address '%s'", str);
+            return;
         }
     } else if (str[0] == '[') {
         /* IPv6 addr */
         if (2 != sscanf(str,"[%64[^]]]:%32[^,]%n",addr,port,&pos)) {
-            fprintf(stderr, "%s: ipv6 parse error (%s)\n",
-                    __FUNCTION__, str);
-            return -1;
+            error_setg(errp, "error parsing IPv6 address '%s'", str);
+            return;
         }
         qemu_opt_set(opts, "ipv6", "on");
     } else if (qemu_isdigit(str[0])) {
         /* IPv4 addr */
         if (2 != sscanf(str,"%64[0-9.]:%32[^,]%n",addr,port,&pos)) {
-            fprintf(stderr, "%s: ipv4 parse error (%s)\n",
-                    __FUNCTION__, str);
-            return -1;
+            error_setg(errp, "error parsing IPv4 address '%s'", str);
+            return;
         }
         qemu_opt_set(opts, "ipv4", "on");
     } else {
         /* hostname */
         if (2 != sscanf(str,"%64[^:]:%32[^,]%n",addr,port,&pos)) {
-            fprintf(stderr, "%s: hostname parse error (%s)\n",
-                    __FUNCTION__, str);
-            return -1;
+            error_setg(errp, "error parsing address '%s'", str);
+            return;
         }
     }
     qemu_opt_set(opts, "host", addr);
@@ -563,7 +559,6 @@ static int inet_parse(QemuOpts *opts, const char *str)
         qemu_opt_set(opts, "ipv4", "on");
     if (strstr(optstr, ",ipv6"))
         qemu_opt_set(opts, "ipv6", "on");
-    return 0;
 }
 
 int inet_listen(const char *str, char *ostr, int olen,
@@ -572,9 +567,11 @@ int inet_listen(const char *str, char *ostr, int olen,
     QemuOpts *opts;
     char *optstr;
     int sock = -1;
+    Error *local_err = NULL;
 
     opts = qemu_opts_create(&dummy_opts, NULL, 0, NULL);
-    if (inet_parse(opts, str) == 0) {
+    inet_parse(opts, str, &local_err);
+    if (local_err == NULL) {
         sock = inet_listen_opts(opts, port_offset, errp);
         if (sock != -1 && ostr) {
             optstr = strchr(str, ',');
@@ -591,7 +588,7 @@ int inet_listen(const char *str, char *ostr, int olen,
             }
         }
     } else {
-        error_set(errp, QERR_SOCKET_CREATE_FAILED);
+        error_propagate(errp, local_err);
     }
     qemu_opts_del(opts);
     return sock;
@@ -609,12 +606,14 @@ int inet_connect(const char *str, Error **errp)
 {
     QemuOpts *opts;
     int sock = -1;
+    Error *local_err = NULL;
 
     opts = qemu_opts_create(&dummy_opts, NULL, 0, NULL);
-    if (inet_parse(opts, str) == 0) {
+    inet_parse(opts, str, &local_err);
+    if (local_err == NULL) {
         sock = inet_connect_opts(opts, errp, NULL, NULL);
     } else {
-        error_set(errp, QERR_SOCKET_CREATE_FAILED);
+        error_propagate(errp, local_err);
     }
     qemu_opts_del(opts);
     return sock;
@@ -639,14 +638,16 @@ int inet_nonblocking_connect(const char *str,
 {
     QemuOpts *opts;
     int sock = -1;
+    Error *local_err = NULL;
 
     g_assert(callback != NULL);
 
     opts = qemu_opts_create(&dummy_opts, NULL, 0, NULL);
-    if (inet_parse(opts, str) == 0) {
+    inet_parse(opts, str, &local_err);
+    if (local_err == NULL) {
         sock = inet_connect_opts(opts, errp, callback, opaque);
     } else {
-        error_set(errp, QERR_SOCKET_CREATE_FAILED);
+        error_propagate(errp, local_err);
     }
     qemu_opts_del(opts);
     return sock;
-- 
1.7.12

Re: [Qemu-devel] [PATCH v2 3/9] qemu-sockets: add error propagation to inet_parse

[Luiz Capitulino]

On Mon,  1 Oct 2012 16:52:18 +0200
Paolo Bonzini <pbonzini@redhat.com> wrote:

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

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

> ---
>  qemu-sockets.c | 41 +++++++++++++++++++++--------------------
>  1 file modificato, 21 inserzioni(+), 20 rimozioni(-)
> 
> diff --git a/qemu-sockets.c b/qemu-sockets.c
> index 1f14e8b..bf1f794 100644
> --- a/qemu-sockets.c
> +++ b/qemu-sockets.c
> @@ -511,7 +511,7 @@ err:
>  }
>  
>  /* compatibility wrapper */
> -static int inet_parse(QemuOpts *opts, const char *str)
> +static void inet_parse(QemuOpts *opts, const char *str, Error **errp)
>  {
>      const char *optstr, *h;
>      char addr[64];
> @@ -523,32 +523,28 @@ static int inet_parse(QemuOpts *opts, const char *str)
>          /* no host given */
>          addr[0] = '\0';
>          if (1 != sscanf(str,":%32[^,]%n",port,&pos)) {
> -            fprintf(stderr, "%s: portonly parse error (%s)\n",
> -                    __FUNCTION__, str);
> -            return -1;
> +            error_setg(errp, "error parsing port in address '%s'", str);
> +            return;
>          }
>      } else if (str[0] == '[') {
>          /* IPv6 addr */
>          if (2 != sscanf(str,"[%64[^]]]:%32[^,]%n",addr,port,&pos)) {
> -            fprintf(stderr, "%s: ipv6 parse error (%s)\n",
> -                    __FUNCTION__, str);
> -            return -1;
> +            error_setg(errp, "error parsing IPv6 address '%s'", str);
> +            return;
>          }
>          qemu_opt_set(opts, "ipv6", "on");
>      } else if (qemu_isdigit(str[0])) {
>          /* IPv4 addr */
>          if (2 != sscanf(str,"%64[0-9.]:%32[^,]%n",addr,port,&pos)) {
> -            fprintf(stderr, "%s: ipv4 parse error (%s)\n",
> -                    __FUNCTION__, str);
> -            return -1;
> +            error_setg(errp, "error parsing IPv4 address '%s'", str);
> +            return;
>          }
>          qemu_opt_set(opts, "ipv4", "on");
>      } else {
>          /* hostname */
>          if (2 != sscanf(str,"%64[^:]:%32[^,]%n",addr,port,&pos)) {
> -            fprintf(stderr, "%s: hostname parse error (%s)\n",
> -                    __FUNCTION__, str);
> -            return -1;
> +            error_setg(errp, "error parsing address '%s'", str);
> +            return;
>          }
>      }
>      qemu_opt_set(opts, "host", addr);
> @@ -563,7 +559,6 @@ static int inet_parse(QemuOpts *opts, const char *str)
>          qemu_opt_set(opts, "ipv4", "on");
>      if (strstr(optstr, ",ipv6"))
>          qemu_opt_set(opts, "ipv6", "on");
> -    return 0;
>  }
>  
>  int inet_listen(const char *str, char *ostr, int olen,
> @@ -572,9 +567,11 @@ int inet_listen(const char *str, char *ostr, int olen,
>      QemuOpts *opts;
>      char *optstr;
>      int sock = -1;
> +    Error *local_err = NULL;
>  
>      opts = qemu_opts_create(&dummy_opts, NULL, 0, NULL);
> -    if (inet_parse(opts, str) == 0) {
> +    inet_parse(opts, str, &local_err);
> +    if (local_err == NULL) {
>          sock = inet_listen_opts(opts, port_offset, errp);
>          if (sock != -1 && ostr) {
>              optstr = strchr(str, ',');
> @@ -591,7 +588,7 @@ int inet_listen(const char *str, char *ostr, int olen,
>              }
>          }
>      } else {
> -        error_set(errp, QERR_SOCKET_CREATE_FAILED);
> +        error_propagate(errp, local_err);
>      }
>      qemu_opts_del(opts);
>      return sock;
> @@ -609,12 +606,14 @@ int inet_connect(const char *str, Error **errp)
>  {
>      QemuOpts *opts;
>      int sock = -1;
> +    Error *local_err = NULL;
>  
>      opts = qemu_opts_create(&dummy_opts, NULL, 0, NULL);
> -    if (inet_parse(opts, str) == 0) {
> +    inet_parse(opts, str, &local_err);
> +    if (local_err == NULL) {
>          sock = inet_connect_opts(opts, errp, NULL, NULL);
>      } else {
> -        error_set(errp, QERR_SOCKET_CREATE_FAILED);
> +        error_propagate(errp, local_err);
>      }
>      qemu_opts_del(opts);
>      return sock;
> @@ -639,14 +638,16 @@ int inet_nonblocking_connect(const char *str,
>  {
>      QemuOpts *opts;
>      int sock = -1;
> +    Error *local_err = NULL;
>  
>      g_assert(callback != NULL);
>  
>      opts = qemu_opts_create(&dummy_opts, NULL, 0, NULL);
> -    if (inet_parse(opts, str) == 0) {
> +    inet_parse(opts, str, &local_err);
> +    if (local_err == NULL) {
>          sock = inet_connect_opts(opts, errp, callback, opaque);
>      } else {
> -        error_set(errp, QERR_SOCKET_CREATE_FAILED);
> +        error_propagate(errp, local_err);
>      }
>      qemu_opts_del(opts);
>      return sock;

[Qemu-devel] [PATCH v2 4/9] qemu-sockets: add error propagation to Unix socket functions

[Paolo Bonzini]

This patch starts harmonizing unix_* and inet_* functions.

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 nbd.c               |  4 ++--
 qemu-char.c         |  4 ++--
 qemu-sockets.c      | 40 ++++++++++++++++++++--------------------
 qemu_socket.h       |  8 ++++----
 qga/channel-posix.c |  2 +-
 ui/vnc.c            |  4 ++--
 6 file modificati, 31 inserzioni(+), 31 rimozioni(-)

diff --git a/nbd.c b/nbd.c
index 6f0db62..f61a288 100644
--- a/nbd.c
+++ b/nbd.c
@@ -230,12 +230,12 @@ int unix_socket_incoming(const char *path)
     char *ostr = NULL;
     int olen = 0;
 
-    return unix_listen(path, ostr, olen);
+    return unix_listen(path, ostr, olen, NULL);
 }
 
 int unix_socket_outgoing(const char *path)
 {
-    return unix_connect(path);
+    return unix_connect(path, NULL);
 }
 
 /* Basic flow for negotiation
diff --git a/qemu-char.c b/qemu-char.c
index b082bae..fa294c0 100644
--- a/qemu-char.c
+++ b/qemu-char.c
@@ -2448,9 +2448,9 @@ static CharDriverState *qemu_chr_open_socket(QemuOpts *opts)
 
     if (is_unix) {
         if (is_listen) {
-            fd = unix_listen_opts(opts);
+            fd = unix_listen_opts(opts, NULL);
         } else {
-            fd = unix_connect_opts(opts);
+            fd = unix_connect_opts(opts, NULL);
         }
     } else {
         if (is_listen) {
diff --git a/qemu-sockets.c b/qemu-sockets.c
index bf1f794..1e69274 100644
--- a/qemu-sockets.c
+++ b/qemu-sockets.c
@@ -655,7 +655,7 @@ int inet_nonblocking_connect(const char *str,
 
 #ifndef _WIN32
 
-int unix_listen_opts(QemuOpts *opts)
+int unix_listen_opts(QemuOpts *opts, Error **errp)
 {
     struct sockaddr_un un;
     const char *path = qemu_opt_get(opts, "path");
@@ -663,7 +663,7 @@ int unix_listen_opts(QemuOpts *opts)
 
     sock = qemu_socket(PF_UNIX, SOCK_STREAM, 0);
     if (sock < 0) {
-        perror("socket(unix)");
+        error_set(errp, QERR_SOCKET_CREATE_FAILED);
         return -1;
     }
 
@@ -688,11 +688,11 @@ int unix_listen_opts(QemuOpts *opts)
 
     unlink(un.sun_path);
     if (bind(sock, (struct sockaddr*) &un, sizeof(un)) < 0) {
-        fprintf(stderr, "bind(unix:%s): %s\n", un.sun_path, strerror(errno));
+        error_set(errp, QERR_SOCKET_BIND_FAILED);
         goto err;
     }
     if (listen(sock, 1) < 0) {
-        fprintf(stderr, "listen(unix:%s): %s\n", un.sun_path, strerror(errno));
+        error_set(errp, QERR_SOCKET_LISTEN_FAILED);
         goto err;
     }
 
@@ -703,20 +703,20 @@ err:
     return -1;
 }
 
-int unix_connect_opts(QemuOpts *opts)
+int unix_connect_opts(QemuOpts *opts, Error **errp)
 {
     struct sockaddr_un un;
     const char *path = qemu_opt_get(opts, "path");
     int sock;
 
     if (NULL == path) {
-        fprintf(stderr, "unix connect: no path specified\n");
+        error_setg(errp, "unix connect: no path specified\n");
         return -1;
     }
 
     sock = qemu_socket(PF_UNIX, SOCK_STREAM, 0);
     if (sock < 0) {
-        perror("socket(unix)");
+        error_set(errp, QERR_SOCKET_CREATE_FAILED);
         return -1;
     }
 
@@ -724,7 +724,7 @@ int unix_connect_opts(QemuOpts *opts)
     un.sun_family = AF_UNIX;
     snprintf(un.sun_path, sizeof(un.sun_path), "%s", path);
     if (connect(sock, (struct sockaddr*) &un, sizeof(un)) < 0) {
-        fprintf(stderr, "connect(unix:%s): %s\n", path, strerror(errno));
+        error_set(errp, QERR_SOCKET_CONNECT_FAILED);
         close(sock);
 	return -1;
     }
@@ -733,7 +733,7 @@ int unix_connect_opts(QemuOpts *opts)
 }
 
 /* compatibility wrapper */
-int unix_listen(const char *str, char *ostr, int olen)
+int unix_listen(const char *str, char *ostr, int olen, Error **errp)
 {
     QemuOpts *opts;
     char *path, *optstr;
@@ -754,7 +754,7 @@ int unix_listen(const char *str, char *ostr, int olen)
         qemu_opt_set(opts, "path", str);
     }
 
-    sock = unix_listen_opts(opts);
+    sock = unix_listen_opts(opts, errp);
 
     if (sock != -1 && ostr)
         snprintf(ostr, olen, "%s%s", qemu_opt_get(opts, "path"), optstr ? optstr : "");
@@ -762,44 +762,44 @@ int unix_listen(const char *str, char *ostr, int olen)
     return sock;
 }
 
-int unix_connect(const char *path)
+int unix_connect(const char *path, Error **errp)
 {
     QemuOpts *opts;
     int sock;
 
     opts = qemu_opts_create(&dummy_opts, NULL, 0, NULL);
     qemu_opt_set(opts, "path", path);
-    sock = unix_connect_opts(opts);
+    sock = unix_connect_opts(opts, errp);
     qemu_opts_del(opts);
     return sock;
 }
 
 #else
 
-int unix_listen_opts(QemuOpts *opts)
+int unix_listen_opts(QemuOpts *opts, Error **errp)
 {
-    fprintf(stderr, "unix sockets are not available on windows\n");
+    error_setg(errp, "unix sockets are not available on windows\n");
     errno = ENOTSUP;
     return -1;
 }
 
-int unix_connect_opts(QemuOpts *opts)
+int unix_connect_opts(QemuOpts *opts, Error **errp)
 {
-    fprintf(stderr, "unix sockets are not available on windows\n");
+    error_setg(errp, "unix sockets are not available on windows\n");
     errno = ENOTSUP;
     return -1;
 }
 
-int unix_listen(const char *path, char *ostr, int olen)
+int unix_listen(const char *path, char *ostr, int olen, Error **errp)
 {
-    fprintf(stderr, "unix sockets are not available on windows\n");
+    error_setg(errp, "unix sockets are not available on windows\n");
     errno = ENOTSUP;
     return -1;
 }
 
-int unix_connect(const char *path)
+int unix_connect(const char *path, Error **errp)
 {
-    fprintf(stderr, "unix sockets are not available on windows\n");
+    error_setg(errp, "unix sockets are not available on windows\n");
     errno = ENOTSUP;
     return -1;
 }
diff --git a/qemu_socket.h b/qemu_socket.h
index 3e8aee9..afe8689 100644
--- a/qemu_socket.h
+++ b/qemu_socket.h
@@ -56,10 +56,10 @@ int inet_nonblocking_connect(const char *str,
 int inet_dgram_opts(QemuOpts *opts);
 const char *inet_strfamily(int family);
 
-int unix_listen_opts(QemuOpts *opts);
-int unix_listen(const char *path, char *ostr, int olen);
-int unix_connect_opts(QemuOpts *opts);
-int unix_connect(const char *path);
+int unix_listen_opts(QemuOpts *opts, Error **errp);
+int unix_listen(const char *path, char *ostr, int olen, Error **errp);
+int unix_connect_opts(QemuOpts *opts, Error **errp);
+int unix_connect(const char *path, Error **errp);
 
 /* Old, ipv4 only bits.  Don't use for new code. */
 int parse_host_port(struct sockaddr_in *saddr, const char *str);
diff --git a/qga/channel-posix.c b/qga/channel-posix.c
index 57eea06..e22eee6 100644
--- a/qga/channel-posix.c
+++ b/qga/channel-posix.c
@@ -181,7 +181,7 @@ static gboolean ga_channel_open(GAChannel *c, const gchar *path, GAChannelMethod
         break;
     }
     case GA_CHANNEL_UNIX_LISTEN: {
-        int fd = unix_listen(path, NULL, strlen(path));
+        int fd = unix_listen(path, NULL, strlen(path), NULL);
         if (fd == -1) {
             g_critical("error opening path: %s", strerror(errno));
             return false;
diff --git a/ui/vnc.c b/ui/vnc.c
index 01b2daf..235596e 100644
--- a/ui/vnc.c
+++ b/ui/vnc.c
@@ -3059,7 +3059,7 @@ int vnc_display_open(DisplayState *ds, const char *display)
     if (reverse) {
         /* connect to viewer */
         if (strncmp(display, "unix:", 5) == 0)
-            vs->lsock = unix_connect(display+5);
+            vs->lsock = unix_connect(display+5, NULL);
         else
             vs->lsock = inet_connect(display, NULL);
         if (-1 == vs->lsock) {
@@ -3079,7 +3079,7 @@ int vnc_display_open(DisplayState *ds, const char *display)
         dpy = g_malloc(256);
         if (strncmp(display, "unix:", 5) == 0) {
             pstrcpy(dpy, 256, "unix:");
-            vs->lsock = unix_listen(display+5, dpy+5, 256-5);
+            vs->lsock = unix_listen(display+5, dpy+5, 256-5, NULL);
         } else {
             vs->lsock = inet_listen(display, dpy, 256,
                                     SOCK_STREAM, 5900, NULL);
-- 
1.7.12

Re: [Qemu-devel] [PATCH v2 4/9] qemu-sockets: add error propagation to Unix socket functions

[Luiz Capitulino]

On Mon,  1 Oct 2012 16:52:19 +0200
Paolo Bonzini <pbonzini@redhat.com> wrote:

> This patch starts harmonizing unix_* and inet_* functions.
> 
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
>  nbd.c               |  4 ++--
>  qemu-char.c         |  4 ++--
>  qemu-sockets.c      | 40 ++++++++++++++++++++--------------------
>  qemu_socket.h       |  8 ++++----
>  qga/channel-posix.c |  2 +-
>  ui/vnc.c            |  4 ++--
>  6 file modificati, 31 inserzioni(+), 31 rimozioni(-)
> 
> diff --git a/nbd.c b/nbd.c
> index 6f0db62..f61a288 100644
> --- a/nbd.c
> +++ b/nbd.c
> @@ -230,12 +230,12 @@ int unix_socket_incoming(const char *path)
>      char *ostr = NULL;
>      int olen = 0;
>  
> -    return unix_listen(path, ostr, olen);
> +    return unix_listen(path, ostr, olen, NULL);
>  }
>  
>  int unix_socket_outgoing(const char *path)
>  {
> -    return unix_connect(path);
> +    return unix_connect(path, NULL);
>  }
>  
>  /* Basic flow for negotiation
> diff --git a/qemu-char.c b/qemu-char.c
> index b082bae..fa294c0 100644
> --- a/qemu-char.c
> +++ b/qemu-char.c
> @@ -2448,9 +2448,9 @@ static CharDriverState *qemu_chr_open_socket(QemuOpts *opts)
>  
>      if (is_unix) {
>          if (is_listen) {
> -            fd = unix_listen_opts(opts);
> +            fd = unix_listen_opts(opts, NULL);
>          } else {
> -            fd = unix_connect_opts(opts);
> +            fd = unix_connect_opts(opts, NULL);
>          }
>      } else {
>          if (is_listen) {
> diff --git a/qemu-sockets.c b/qemu-sockets.c
> index bf1f794..1e69274 100644
> --- a/qemu-sockets.c
> +++ b/qemu-sockets.c
> @@ -655,7 +655,7 @@ int inet_nonblocking_connect(const char *str,
>  
>  #ifndef _WIN32
>  
> -int unix_listen_opts(QemuOpts *opts)
> +int unix_listen_opts(QemuOpts *opts, Error **errp)
>  {
>      struct sockaddr_un un;
>      const char *path = qemu_opt_get(opts, "path");
> @@ -663,7 +663,7 @@ int unix_listen_opts(QemuOpts *opts)
>  
>      sock = qemu_socket(PF_UNIX, SOCK_STREAM, 0);
>      if (sock < 0) {
> -        perror("socket(unix)");
> +        error_set(errp, QERR_SOCKET_CREATE_FAILED);
>          return -1;
>      }
>  
> @@ -688,11 +688,11 @@ int unix_listen_opts(QemuOpts *opts)
>  
>      unlink(un.sun_path);
>      if (bind(sock, (struct sockaddr*) &un, sizeof(un)) < 0) {
> -        fprintf(stderr, "bind(unix:%s): %s\n", un.sun_path, strerror(errno));
> +        error_set(errp, QERR_SOCKET_BIND_FAILED);

This drops error information, making the error message worse. I believe
you have a reason to not use error_setg()?

Also, I see that in some hunks you do something like:

  -            fd = unix_listen_opts(opts);
  +            fd = unix_listen_opts(opts, NULL);

This will break printing the error message to the user. It's fine by me if
you do this only temporarily (ie. this is fixed by the next or a later patch),
but want to double check that you're aware of it.

Btw, I'm making these comments in this hunk but they apply to similar hunks
as well.

>          goto err;
>      }
>      if (listen(sock, 1) < 0) {
> -        fprintf(stderr, "listen(unix:%s): %s\n", un.sun_path, strerror(errno));
> +        error_set(errp, QERR_SOCKET_LISTEN_FAILED);
>          goto err;
>      }
>  
> @@ -703,20 +703,20 @@ err:
>      return -1;
>  }
>  
> -int unix_connect_opts(QemuOpts *opts)
> +int unix_connect_opts(QemuOpts *opts, Error **errp)
>  {
>      struct sockaddr_un un;
>      const char *path = qemu_opt_get(opts, "path");
>      int sock;
>  
>      if (NULL == path) {
> -        fprintf(stderr, "unix connect: no path specified\n");
> +        error_setg(errp, "unix connect: no path specified\n");
>          return -1;
>      }
>  
>      sock = qemu_socket(PF_UNIX, SOCK_STREAM, 0);
>      if (sock < 0) {
> -        perror("socket(unix)");
> +        error_set(errp, QERR_SOCKET_CREATE_FAILED);
>          return -1;
>      }
>  
> @@ -724,7 +724,7 @@ int unix_connect_opts(QemuOpts *opts)
>      un.sun_family = AF_UNIX;
>      snprintf(un.sun_path, sizeof(un.sun_path), "%s", path);
>      if (connect(sock, (struct sockaddr*) &un, sizeof(un)) < 0) {
> -        fprintf(stderr, "connect(unix:%s): %s\n", path, strerror(errno));
> +        error_set(errp, QERR_SOCKET_CONNECT_FAILED);
>          close(sock);
>  	return -1;
>      }
> @@ -733,7 +733,7 @@ int unix_connect_opts(QemuOpts *opts)
>  }
>  
>  /* compatibility wrapper */
> -int unix_listen(const char *str, char *ostr, int olen)
> +int unix_listen(const char *str, char *ostr, int olen, Error **errp)
>  {
>      QemuOpts *opts;
>      char *path, *optstr;
> @@ -754,7 +754,7 @@ int unix_listen(const char *str, char *ostr, int olen)
>          qemu_opt_set(opts, "path", str);
>      }
>  
> -    sock = unix_listen_opts(opts);
> +    sock = unix_listen_opts(opts, errp);
>  
>      if (sock != -1 && ostr)
>          snprintf(ostr, olen, "%s%s", qemu_opt_get(opts, "path"), optstr ? optstr : "");
> @@ -762,44 +762,44 @@ int unix_listen(const char *str, char *ostr, int olen)
>      return sock;
>  }
>  
> -int unix_connect(const char *path)
> +int unix_connect(const char *path, Error **errp)
>  {
>      QemuOpts *opts;
>      int sock;
>  
>      opts = qemu_opts_create(&dummy_opts, NULL, 0, NULL);
>      qemu_opt_set(opts, "path", path);
> -    sock = unix_connect_opts(opts);
> +    sock = unix_connect_opts(opts, errp);
>      qemu_opts_del(opts);
>      return sock;
>  }
>  
>  #else
>  
> -int unix_listen_opts(QemuOpts *opts)
> +int unix_listen_opts(QemuOpts *opts, Error **errp)
>  {
> -    fprintf(stderr, "unix sockets are not available on windows\n");
> +    error_setg(errp, "unix sockets are not available on windows\n");
>      errno = ENOTSUP;
>      return -1;
>  }
>  
> -int unix_connect_opts(QemuOpts *opts)
> +int unix_connect_opts(QemuOpts *opts, Error **errp)
>  {
> -    fprintf(stderr, "unix sockets are not available on windows\n");
> +    error_setg(errp, "unix sockets are not available on windows\n");
>      errno = ENOTSUP;
>      return -1;
>  }
>  
> -int unix_listen(const char *path, char *ostr, int olen)
> +int unix_listen(const char *path, char *ostr, int olen, Error **errp)
>  {
> -    fprintf(stderr, "unix sockets are not available on windows\n");
> +    error_setg(errp, "unix sockets are not available on windows\n");
>      errno = ENOTSUP;
>      return -1;
>  }
>  
> -int unix_connect(const char *path)
> +int unix_connect(const char *path, Error **errp)
>  {
> -    fprintf(stderr, "unix sockets are not available on windows\n");
> +    error_setg(errp, "unix sockets are not available on windows\n");
>      errno = ENOTSUP;
>      return -1;
>  }
> diff --git a/qemu_socket.h b/qemu_socket.h
> index 3e8aee9..afe8689 100644
> --- a/qemu_socket.h
> +++ b/qemu_socket.h
> @@ -56,10 +56,10 @@ int inet_nonblocking_connect(const char *str,
>  int inet_dgram_opts(QemuOpts *opts);
>  const char *inet_strfamily(int family);
>  
> -int unix_listen_opts(QemuOpts *opts);
> -int unix_listen(const char *path, char *ostr, int olen);
> -int unix_connect_opts(QemuOpts *opts);
> -int unix_connect(const char *path);
> +int unix_listen_opts(QemuOpts *opts, Error **errp);
> +int unix_listen(const char *path, char *ostr, int olen, Error **errp);
> +int unix_connect_opts(QemuOpts *opts, Error **errp);
> +int unix_connect(const char *path, Error **errp);
>  
>  /* Old, ipv4 only bits.  Don't use for new code. */
>  int parse_host_port(struct sockaddr_in *saddr, const char *str);
> diff --git a/qga/channel-posix.c b/qga/channel-posix.c
> index 57eea06..e22eee6 100644
> --- a/qga/channel-posix.c
> +++ b/qga/channel-posix.c
> @@ -181,7 +181,7 @@ static gboolean ga_channel_open(GAChannel *c, const gchar *path, GAChannelMethod
>          break;
>      }
>      case GA_CHANNEL_UNIX_LISTEN: {
> -        int fd = unix_listen(path, NULL, strlen(path));
> +        int fd = unix_listen(path, NULL, strlen(path), NULL);
>          if (fd == -1) {
>              g_critical("error opening path: %s", strerror(errno));
>              return false;
> diff --git a/ui/vnc.c b/ui/vnc.c
> index 01b2daf..235596e 100644
> --- a/ui/vnc.c
> +++ b/ui/vnc.c
> @@ -3059,7 +3059,7 @@ int vnc_display_open(DisplayState *ds, const char *display)
>      if (reverse) {
>          /* connect to viewer */
>          if (strncmp(display, "unix:", 5) == 0)
> -            vs->lsock = unix_connect(display+5);
> +            vs->lsock = unix_connect(display+5, NULL);
>          else
>              vs->lsock = inet_connect(display, NULL);
>          if (-1 == vs->lsock) {
> @@ -3079,7 +3079,7 @@ int vnc_display_open(DisplayState *ds, const char *display)
>          dpy = g_malloc(256);
>          if (strncmp(display, "unix:", 5) == 0) {
>              pstrcpy(dpy, 256, "unix:");
> -            vs->lsock = unix_listen(display+5, dpy+5, 256-5);
> +            vs->lsock = unix_listen(display+5, dpy+5, 256-5, NULL);
>          } else {
>              vs->lsock = inet_listen(display, dpy, 256,
>                                      SOCK_STREAM, 5900, NULL);

Re: [Qemu-devel] [PATCH v2 4/9] qemu-sockets: add error propagation to Unix socket functions

[Paolo Bonzini]

Il 01/10/2012 19:17, Luiz Capitulino ha scritto:
>> >      if (bind(sock, (struct sockaddr*) &un, sizeof(un)) < 0) {
>> > -        fprintf(stderr, "bind(unix:%s): %s\n", un.sun_path, strerror(errno));
>> > +        error_set(errp, QERR_SOCKET_BIND_FAILED);
> This drops error information, making the error message worse. I believe
> you have a reason to not use error_setg()?

I was waiting for the end of the discussion on errno to add
error_setg_errno.

> Also, I see that in some hunks you do something like:
> 
>   -            fd = unix_listen_opts(opts);
>   +            fd = unix_listen_opts(opts, NULL);
> 
> This will break printing the error message to the user. It's fine by me if
> you do this only temporarily (ie. this is fixed by the next or a later patch),
> but want to double check that you're aware of it.

I want to avoid super-large patch series, so I would prefer to fix it
later in the 1.3 development.

Paolo

> Btw, I'm making these comments in this hunk but they apply to similar hunks
> as well.
> 

Re: [Qemu-devel] [PATCH v2 4/9] qemu-sockets: add error propagation to Unix socket functions

[Luiz Capitulino]

On Mon, 01 Oct 2012 21:07:52 +0200
Paolo Bonzini <pbonzini@redhat.com> wrote:

> Il 01/10/2012 19:17, Luiz Capitulino ha scritto:
> >> >      if (bind(sock, (struct sockaddr*) &un, sizeof(un)) < 0) {
> >> > -        fprintf(stderr, "bind(unix:%s): %s\n", un.sun_path, strerror(errno));
> >> > +        error_set(errp, QERR_SOCKET_BIND_FAILED);
> > This drops error information, making the error message worse. I believe
> > you have a reason to not use error_setg()?
> 
> I was waiting for the end of the discussion on errno to add
> error_setg_errno.

The decision was to not add errno now, right?

> > Also, I see that in some hunks you do something like:
> > 
> >   -            fd = unix_listen_opts(opts);
> >   +            fd = unix_listen_opts(opts, NULL);
> > 
> > This will break printing the error message to the user. It's fine by me if
> > you do this only temporarily (ie. this is fixed by the next or a later patch),
> > but want to double check that you're aware of it.
> 
> I want to avoid super-large patch series, so I would prefer to fix it
> later in the 1.3 development.

We at least need to have the patches flying, I don't think it's ok to
break error reporting like that.

Re: [Qemu-devel] [PATCH v2 4/9] qemu-sockets: add error propagation to Unix socket functions

[Paolo Bonzini]

Il 02/10/2012 01:05, Luiz Capitulino ha scritto:
> On Mon, 01 Oct 2012 21:07:52 +0200
> Paolo Bonzini <pbonzini@redhat.com> wrote:
> 
>> Il 01/10/2012 19:17, Luiz Capitulino ha scritto:
>>>>>      if (bind(sock, (struct sockaddr*) &un, sizeof(un)) < 0) {
>>>>> -        fprintf(stderr, "bind(unix:%s): %s\n", un.sun_path, strerror(errno));
>>>>> +        error_set(errp, QERR_SOCKET_BIND_FAILED);
>>> This drops error information, making the error message worse. I believe
>>> you have a reason to not use error_setg()?
>>
>> I was waiting for the end of the discussion on errno to add
>> error_setg_errno.
> 
> The decision was to not add errno now, right?

I don't remember, but we can still add a function that takes an errno
value and tacks on the strerror(errno).

>>> Also, I see that in some hunks you do something like:
>>>
>>>   -            fd = unix_listen_opts(opts);
>>>   +            fd = unix_listen_opts(opts, NULL);
>>>
>>> This will break printing the error message to the user. It's fine by me if
>>> you do this only temporarily (ie. this is fixed by the next or a later patch),
>>> but want to double check that you're aware of it.
>>
>> I want to avoid super-large patch series, so I would prefer to fix it
>> later in the 1.3 development.
> 
> We at least need to have the patches flying, I don't think it's ok to
> break error reporting like that.

I disagree because we still have two months before release, but I'll see
what I can do.

Paolo

[Qemu-devel] [PATCH v2 5/9] qemu-sockets: return IPSocketAddress from inet_parse

[Paolo Bonzini]

Prepare inet_parse so that it can be easily reused in socket_parse.
socket_parse will be public and will help converting addresses accepted
by HMP to SocketAddress structures accepted by the implementation of
QMP commands.

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 qemu-sockets.c | 119 +++++++++++++++++++++++++++++++++++++--------------------
 1 file modificato, 78 inserzioni(+), 41 rimozioni(-)

diff --git a/qemu-sockets.c b/qemu-sockets.c
index 1e69274..f67a113 100644
--- a/qemu-sockets.c
+++ b/qemu-sockets.c
@@ -511,54 +511,91 @@ err:
 }
 
 /* compatibility wrapper */
-static void inet_parse(QemuOpts *opts, const char *str, Error **errp)
+static IPSocketAddress *inet_parse(const char *str, Error **errp)
 {
+    IPSocketAddress *addr;
     const char *optstr, *h;
-    char addr[64];
+    char host[64];
     char port[33];
+    int to;
     int pos;
 
+    addr = g_new(IPSocketAddress, 1);
+
     /* parse address */
     if (str[0] == ':') {
         /* no host given */
-        addr[0] = '\0';
+        host[0] = '\0';
         if (1 != sscanf(str,":%32[^,]%n",port,&pos)) {
             error_setg(errp, "error parsing port in address '%s'", str);
-            return;
+            goto fail;
         }
     } else if (str[0] == '[') {
         /* IPv6 addr */
-        if (2 != sscanf(str,"[%64[^]]]:%32[^,]%n",addr,port,&pos)) {
+        if (2 != sscanf(str,"[%64[^]]]:%32[^,]%n",host,port,&pos)) {
             error_setg(errp, "error parsing IPv6 address '%s'", str);
-            return;
+            goto fail;
         }
-        qemu_opt_set(opts, "ipv6", "on");
+        addr->ipv6 = addr->has_ipv6 = true;
     } else if (qemu_isdigit(str[0])) {
         /* IPv4 addr */
-        if (2 != sscanf(str,"%64[0-9.]:%32[^,]%n",addr,port,&pos)) {
+        if (2 != sscanf(str,"%64[0-9.]:%32[^,]%n",host,port,&pos)) {
             error_setg(errp, "error parsing IPv4 address '%s'", str);
-            return;
+            goto fail;
         }
-        qemu_opt_set(opts, "ipv4", "on");
+        addr->ipv4 = addr->has_ipv4 = true;
     } else {
         /* hostname */
-        if (2 != sscanf(str,"%64[^:]:%32[^,]%n",addr,port,&pos)) {
+        if (2 != sscanf(str,"%64[^:]:%32[^,]%n",host,port,&pos)) {
             error_setg(errp, "error parsing address '%s'", str);
-            return;
+            goto fail;
         }
     }
-    qemu_opt_set(opts, "host", addr);
-    qemu_opt_set(opts, "port", port);
+
+    addr->host = g_strdup(host);
+    addr->port = g_strdup(port);
 
     /* parse options */
     optstr = str + pos;
     h = strstr(optstr, ",to=");
-    if (h)
-        qemu_opt_set(opts, "to", h+4);
-    if (strstr(optstr, ",ipv4"))
-        qemu_opt_set(opts, "ipv4", "on");
-    if (strstr(optstr, ",ipv6"))
-        qemu_opt_set(opts, "ipv6", "on");
+    if (h) {
+        if (1 != sscanf(str, "%d%n", &to, &pos) ||
+            (str[pos] != '\0' && str[pos] != ',')) {
+            error_setg(errp, "error parsing to= argument");
+            goto fail;
+        }
+        addr->has_to = true;
+        addr->to = to;
+    }
+    if (strstr(optstr, ",ipv4")) {
+        addr->ipv4 = addr->has_ipv4 = true;
+    }
+    if (strstr(optstr, ",ipv6")) {
+        addr->ipv6 = addr->has_ipv6 = true;
+    }
+    return addr;
+
+fail:
+    qapi_free_IPSocketAddress(addr);
+    return NULL;
+}
+
+static void inet_addr_to_opts(QemuOpts *opts, IPSocketAddress *addr)
+{
+    bool ipv4 = addr->ipv4 || !addr->has_ipv4;
+    bool ipv6 = addr->ipv6 || !addr->has_ipv6;
+
+    if (!ipv4 || !ipv6) {
+        qemu_opt_set_bool(opts, "ipv4", ipv4);
+        qemu_opt_set_bool(opts, "ipv6", ipv6);
+    }
+    if (addr->has_to) {
+        char to[20];
+        snprintf(to, sizeof(to), "%d", addr->to);
+        qemu_opt_set(opts, "to", to);
+    }
+    qemu_opt_set(opts, "host", addr->host);
+    qemu_opt_set(opts, "port", addr->port);
 }
 
 int inet_listen(const char *str, char *ostr, int olen,
@@ -567,11 +604,13 @@ int inet_listen(const char *str, char *ostr, int olen,
     QemuOpts *opts;
     char *optstr;
     int sock = -1;
-    Error *local_err = NULL;
+    IPSocketAddress *addr;
 
-    opts = qemu_opts_create(&dummy_opts, NULL, 0, NULL);
-    inet_parse(opts, str, &local_err);
-    if (local_err == NULL) {
+    addr = inet_parse(str, errp);
+    if (addr != NULL) {
+        opts = qemu_opts_create(&dummy_opts, NULL, 0, NULL);
+        inet_addr_to_opts(opts, addr);
+        qapi_free_IPSocketAddress(addr);
         sock = inet_listen_opts(opts, port_offset, errp);
         if (sock != -1 && ostr) {
             optstr = strchr(str, ',');
@@ -587,10 +626,8 @@ int inet_listen(const char *str, char *ostr, int olen,
                          optstr ? optstr : "");
             }
         }
-    } else {
-        error_propagate(errp, local_err);
+        qemu_opts_del(opts);
     }
-    qemu_opts_del(opts);
     return sock;
 }
 
@@ -606,16 +643,16 @@ int inet_connect(const char *str, Error **errp)
 {
     QemuOpts *opts;
     int sock = -1;
-    Error *local_err = NULL;
+    IPSocketAddress *addr;
 
-    opts = qemu_opts_create(&dummy_opts, NULL, 0, NULL);
-    inet_parse(opts, str, &local_err);
-    if (local_err == NULL) {
+    addr = inet_parse(str, errp);
+    if (addr != NULL) {
+        opts = qemu_opts_create(&dummy_opts, NULL, 0, NULL);
+        inet_addr_to_opts(opts, addr);
+        qapi_free_IPSocketAddress(addr);
         sock = inet_connect_opts(opts, errp, NULL, NULL);
-    } else {
-        error_propagate(errp, local_err);
+        qemu_opts_del(opts);
     }
-    qemu_opts_del(opts);
     return sock;
 }
 
@@ -638,18 +675,18 @@ int inet_nonblocking_connect(const char *str,
 {
     QemuOpts *opts;
     int sock = -1;
-    Error *local_err = NULL;
+    IPSocketAddress *addr;
 
     g_assert(callback != NULL);
 
-    opts = qemu_opts_create(&dummy_opts, NULL, 0, NULL);
-    inet_parse(opts, str, &local_err);
-    if (local_err == NULL) {
+    addr = inet_parse(str, errp);
+    if (addr != NULL) {
+        opts = qemu_opts_create(&dummy_opts, NULL, 0, NULL);
+        inet_addr_to_opts(opts, addr);
+        qapi_free_IPSocketAddress(addr);
         sock = inet_connect_opts(opts, errp, callback, opaque);
-    } else {
-        error_propagate(errp, local_err);
+        qemu_opts_del(opts);
     }
-    qemu_opts_del(opts);
     return sock;
 }
 
-- 
1.7.12

Re: [Qemu-devel] [PATCH v2 5/9] qemu-sockets: return IPSocketAddress from inet_parse

[Luiz Capitulino]

On Mon,  1 Oct 2012 16:52:20 +0200
Paolo Bonzini <pbonzini@redhat.com> wrote:

> Prepare inet_parse so that it can be easily reused in socket_parse.
> socket_parse will be public and will help converting addresses accepted
> by HMP to SocketAddress structures accepted by the implementation of
> QMP commands.
> 
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>

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

> ---
>  qemu-sockets.c | 119 +++++++++++++++++++++++++++++++++++++--------------------
>  1 file modificato, 78 inserzioni(+), 41 rimozioni(-)
> 
> diff --git a/qemu-sockets.c b/qemu-sockets.c
> index 1e69274..f67a113 100644
> --- a/qemu-sockets.c
> +++ b/qemu-sockets.c
> @@ -511,54 +511,91 @@ err:
>  }
>  
>  /* compatibility wrapper */
> -static void inet_parse(QemuOpts *opts, const char *str, Error **errp)
> +static IPSocketAddress *inet_parse(const char *str, Error **errp)
>  {
> +    IPSocketAddress *addr;
>      const char *optstr, *h;
> -    char addr[64];
> +    char host[64];
>      char port[33];
> +    int to;
>      int pos;
>  
> +    addr = g_new(IPSocketAddress, 1);
> +
>      /* parse address */
>      if (str[0] == ':') {
>          /* no host given */
> -        addr[0] = '\0';
> +        host[0] = '\0';
>          if (1 != sscanf(str,":%32[^,]%n",port,&pos)) {
>              error_setg(errp, "error parsing port in address '%s'", str);
> -            return;
> +            goto fail;
>          }
>      } else if (str[0] == '[') {
>          /* IPv6 addr */
> -        if (2 != sscanf(str,"[%64[^]]]:%32[^,]%n",addr,port,&pos)) {
> +        if (2 != sscanf(str,"[%64[^]]]:%32[^,]%n",host,port,&pos)) {
>              error_setg(errp, "error parsing IPv6 address '%s'", str);
> -            return;
> +            goto fail;
>          }
> -        qemu_opt_set(opts, "ipv6", "on");
> +        addr->ipv6 = addr->has_ipv6 = true;
>      } else if (qemu_isdigit(str[0])) {
>          /* IPv4 addr */
> -        if (2 != sscanf(str,"%64[0-9.]:%32[^,]%n",addr,port,&pos)) {
> +        if (2 != sscanf(str,"%64[0-9.]:%32[^,]%n",host,port,&pos)) {
>              error_setg(errp, "error parsing IPv4 address '%s'", str);
> -            return;
> +            goto fail;
>          }
> -        qemu_opt_set(opts, "ipv4", "on");
> +        addr->ipv4 = addr->has_ipv4 = true;
>      } else {
>          /* hostname */
> -        if (2 != sscanf(str,"%64[^:]:%32[^,]%n",addr,port,&pos)) {
> +        if (2 != sscanf(str,"%64[^:]:%32[^,]%n",host,port,&pos)) {
>              error_setg(errp, "error parsing address '%s'", str);
> -            return;
> +            goto fail;
>          }
>      }
> -    qemu_opt_set(opts, "host", addr);
> -    qemu_opt_set(opts, "port", port);
> +
> +    addr->host = g_strdup(host);
> +    addr->port = g_strdup(port);
>  
>      /* parse options */
>      optstr = str + pos;
>      h = strstr(optstr, ",to=");
> -    if (h)
> -        qemu_opt_set(opts, "to", h+4);
> -    if (strstr(optstr, ",ipv4"))
> -        qemu_opt_set(opts, "ipv4", "on");
> -    if (strstr(optstr, ",ipv6"))
> -        qemu_opt_set(opts, "ipv6", "on");
> +    if (h) {
> +        if (1 != sscanf(str, "%d%n", &to, &pos) ||
> +            (str[pos] != '\0' && str[pos] != ',')) {
> +            error_setg(errp, "error parsing to= argument");
> +            goto fail;
> +        }
> +        addr->has_to = true;
> +        addr->to = to;
> +    }
> +    if (strstr(optstr, ",ipv4")) {
> +        addr->ipv4 = addr->has_ipv4 = true;
> +    }
> +    if (strstr(optstr, ",ipv6")) {
> +        addr->ipv6 = addr->has_ipv6 = true;
> +    }
> +    return addr;
> +
> +fail:
> +    qapi_free_IPSocketAddress(addr);
> +    return NULL;
> +}
> +
> +static void inet_addr_to_opts(QemuOpts *opts, IPSocketAddress *addr)
> +{
> +    bool ipv4 = addr->ipv4 || !addr->has_ipv4;
> +    bool ipv6 = addr->ipv6 || !addr->has_ipv6;
> +
> +    if (!ipv4 || !ipv6) {
> +        qemu_opt_set_bool(opts, "ipv4", ipv4);
> +        qemu_opt_set_bool(opts, "ipv6", ipv6);
> +    }
> +    if (addr->has_to) {
> +        char to[20];
> +        snprintf(to, sizeof(to), "%d", addr->to);
> +        qemu_opt_set(opts, "to", to);
> +    }
> +    qemu_opt_set(opts, "host", addr->host);
> +    qemu_opt_set(opts, "port", addr->port);
>  }
>  
>  int inet_listen(const char *str, char *ostr, int olen,
> @@ -567,11 +604,13 @@ int inet_listen(const char *str, char *ostr, int olen,
>      QemuOpts *opts;
>      char *optstr;
>      int sock = -1;
> -    Error *local_err = NULL;
> +    IPSocketAddress *addr;
>  
> -    opts = qemu_opts_create(&dummy_opts, NULL, 0, NULL);
> -    inet_parse(opts, str, &local_err);
> -    if (local_err == NULL) {
> +    addr = inet_parse(str, errp);
> +    if (addr != NULL) {
> +        opts = qemu_opts_create(&dummy_opts, NULL, 0, NULL);
> +        inet_addr_to_opts(opts, addr);
> +        qapi_free_IPSocketAddress(addr);
>          sock = inet_listen_opts(opts, port_offset, errp);
>          if (sock != -1 && ostr) {
>              optstr = strchr(str, ',');
> @@ -587,10 +626,8 @@ int inet_listen(const char *str, char *ostr, int olen,
>                           optstr ? optstr : "");
>              }
>          }
> -    } else {
> -        error_propagate(errp, local_err);
> +        qemu_opts_del(opts);
>      }
> -    qemu_opts_del(opts);
>      return sock;
>  }
>  
> @@ -606,16 +643,16 @@ int inet_connect(const char *str, Error **errp)
>  {
>      QemuOpts *opts;
>      int sock = -1;
> -    Error *local_err = NULL;
> +    IPSocketAddress *addr;
>  
> -    opts = qemu_opts_create(&dummy_opts, NULL, 0, NULL);
> -    inet_parse(opts, str, &local_err);
> -    if (local_err == NULL) {
> +    addr = inet_parse(str, errp);
> +    if (addr != NULL) {
> +        opts = qemu_opts_create(&dummy_opts, NULL, 0, NULL);
> +        inet_addr_to_opts(opts, addr);
> +        qapi_free_IPSocketAddress(addr);
>          sock = inet_connect_opts(opts, errp, NULL, NULL);
> -    } else {
> -        error_propagate(errp, local_err);
> +        qemu_opts_del(opts);
>      }
> -    qemu_opts_del(opts);
>      return sock;
>  }
>  
> @@ -638,18 +675,18 @@ int inet_nonblocking_connect(const char *str,
>  {
>      QemuOpts *opts;
>      int sock = -1;
> -    Error *local_err = NULL;
> +    IPSocketAddress *addr;
>  
>      g_assert(callback != NULL);
>  
> -    opts = qemu_opts_create(&dummy_opts, NULL, 0, NULL);
> -    inet_parse(opts, str, &local_err);
> -    if (local_err == NULL) {
> +    addr = inet_parse(str, errp);
> +    if (addr != NULL) {
> +        opts = qemu_opts_create(&dummy_opts, NULL, 0, NULL);
> +        inet_addr_to_opts(opts, addr);
> +        qapi_free_IPSocketAddress(addr);
>          sock = inet_connect_opts(opts, errp, callback, opaque);
> -    } else {
> -        error_propagate(errp, local_err);
> +        qemu_opts_del(opts);
>      }
> -    qemu_opts_del(opts);
>      return sock;
>  }
>  

[Qemu-devel] [PATCH v2 6/9] qemu-sockets: add socket_listen, socket_connect, socket_parse

[Paolo Bonzini]

These are QAPI-friendly versions of the qemu-sockets functions.  They
support IP sockets, Unix sockets, and named file descriptors, using a
QAPI union to dispatch to the correct function.

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 qemu-sockets.c | 95 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
 qemu-tool.c    |  6 ++++
 qemu_socket.h  |  4 +++
 3 file modificati, 105 inserzioni(+)

diff --git a/qemu-sockets.c b/qemu-sockets.c
index f67a113..ed2d2be 100644
--- a/qemu-sockets.c
+++ b/qemu-sockets.c
@@ -22,6 +22,7 @@
 #include <errno.h>
 #include <unistd.h>
 
+#include "monitor.h"
 #include "qemu_socket.h"
 #include "qemu-common.h" /* for qemu_isdigit */
 #include "main-loop.h"
@@ -843,6 +844,100 @@ int unix_connect(const char *path, Error **errp)
 
 #endif
 
+SocketAddress *socket_parse(const char *str, Error **errp)
+{
+    SocketAddress *addr = NULL;
+
+    addr = g_new(SocketAddress, 1);
+    if (strstart(str, "unix:", NULL)) {
+        if (str[5] == '\0') {
+            error_setg(errp, "invalid Unix socket address\n");
+            goto fail;
+        } else {
+            addr->kind = SOCKET_ADDRESS_KIND_UNIX;
+            addr->q_unix = g_new(UnixSocketAddress, 1);
+            addr->q_unix->path = g_strdup(str + 5);
+        }
+    } else if (strstart(str, "fd:", NULL)) {
+        if (str[3] == '\0') {
+            error_setg(errp, "invalid file descriptor address\n");
+            goto fail;
+        } else {
+            addr->kind = SOCKET_ADDRESS_KIND_FD;
+            addr->fd = g_new(String, 1);
+            addr->fd->str = g_strdup(str + 3);
+        }
+    } else {
+        addr->kind = SOCKET_ADDRESS_KIND_INET;
+        addr->inet = g_new(IPSocketAddress, 1);
+        addr->inet = inet_parse(str, errp);
+        if (addr->inet == NULL) {
+            goto fail;
+        }
+    }
+    return addr;
+
+fail:
+    qapi_free_SocketAddress(addr);
+    return NULL;
+}
+
+int socket_connect(SocketAddress *addr, Error **errp)
+{
+    QemuOpts *opts;
+    int fd;
+
+    opts = qemu_opts_create(&dummy_opts, NULL, 0, NULL);
+    switch (addr->kind) {
+    case SOCKET_ADDRESS_KIND_INET:
+        inet_addr_to_opts(opts, addr->inet);
+        fd = inet_connect_opts(opts, true, NULL, errp, NULL);
+        break;
+
+    case SOCKET_ADDRESS_KIND_UNIX:
+        qemu_opt_set(opts, "path", addr->q_unix->path);
+        fd = unix_connect_opts(opts, errp);
+        break;
+
+    case SOCKET_ADDRESS_KIND_FD:
+        fd = monitor_get_fd(cur_mon, addr->fd->str, errp);
+        break;
+
+    default:
+        abort();
+    }
+    qemu_opts_del(opts);
+    return fd;
+}
+
+int socket_listen(SocketAddress *addr, Error **errp)
+{
+    QemuOpts *opts;
+    int fd;
+
+    opts = qemu_opts_create(&dummy_opts, NULL, 0, NULL);
+    switch (addr->kind) {
+    case SOCKET_ADDRESS_KIND_INET:
+        inet_addr_to_opts(opts, addr->inet);
+        fd = inet_listen_opts(opts, 0, errp);
+        break;
+
+    case SOCKET_ADDRESS_KIND_UNIX:
+        qemu_opt_set(opts, "path", addr->q_unix->path);
+        fd = unix_listen_opts(opts, errp);
+        break;
+
+    case SOCKET_ADDRESS_KIND_FD:
+        fd = monitor_get_fd(cur_mon, addr->fd->str, errp);
+        break;
+
+    default:
+        abort();
+    }
+    qemu_opts_del(opts);
+    return fd;
+}
+
 #ifdef _WIN32
 static void socket_cleanup(void)
 {
diff --git a/qemu-tool.c b/qemu-tool.c
index 18205ba..7658fde 100644
--- a/qemu-tool.c
+++ b/qemu-tool.c
@@ -42,6 +42,12 @@ int monitor_cur_is_qmp(void)
     return 0;
 }
 
+int monitor_get_fd(Monitor *mon, const char *name, Error **errp)
+{
+    error_setg(errp, "only QEMU supports file descriptor passing");
+    return -1;
+}
+
 void monitor_set_error(Monitor *mon, QError *qerror)
 {
 }
diff --git a/qemu_socket.h b/qemu_socket.h
index afe8689..cd083d0 100644
--- a/qemu_socket.h
+++ b/qemu_socket.h
@@ -61,6 +61,10 @@ int unix_listen(const char *path, char *ostr, int olen, Error **errp);
 int unix_connect_opts(QemuOpts *opts, Error **errp);
 int unix_connect(const char *path, Error **errp);
 
+SocketAddress *socket_parse(const char *str, Error **errp);
+int socket_connect(SocketAddress *addr, Error **errp);
+int socket_listen(SocketAddress *addr, Error **errp);
+
 /* Old, ipv4 only bits.  Don't use for new code. */
 int parse_host_port(struct sockaddr_in *saddr, const char *str);
 int socket_init(void);
-- 
1.7.12

Re: [Qemu-devel] [PATCH v2 6/9] qemu-sockets: add socket_listen, socket_connect, socket_parse

[Luiz Capitulino]

On Mon,  1 Oct 2012 16:52:21 +0200
Paolo Bonzini <pbonzini@redhat.com> wrote:

> These are QAPI-friendly versions of the qemu-sockets functions.  They
> support IP sockets, Unix sockets, and named file descriptors, using a
> QAPI union to dispatch to the correct function.
> 
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>

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

> ---
>  qemu-sockets.c | 95 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
>  qemu-tool.c    |  6 ++++
>  qemu_socket.h  |  4 +++
>  3 file modificati, 105 inserzioni(+)
> 
> diff --git a/qemu-sockets.c b/qemu-sockets.c
> index f67a113..ed2d2be 100644
> --- a/qemu-sockets.c
> +++ b/qemu-sockets.c
> @@ -22,6 +22,7 @@
>  #include <errno.h>
>  #include <unistd.h>
>  
> +#include "monitor.h"
>  #include "qemu_socket.h"
>  #include "qemu-common.h" /* for qemu_isdigit */
>  #include "main-loop.h"
> @@ -843,6 +844,100 @@ int unix_connect(const char *path, Error **errp)
>  
>  #endif
>  
> +SocketAddress *socket_parse(const char *str, Error **errp)
> +{
> +    SocketAddress *addr = NULL;
> +
> +    addr = g_new(SocketAddress, 1);
> +    if (strstart(str, "unix:", NULL)) {
> +        if (str[5] == '\0') {
> +            error_setg(errp, "invalid Unix socket address\n");
> +            goto fail;
> +        } else {
> +            addr->kind = SOCKET_ADDRESS_KIND_UNIX;
> +            addr->q_unix = g_new(UnixSocketAddress, 1);
> +            addr->q_unix->path = g_strdup(str + 5);
> +        }
> +    } else if (strstart(str, "fd:", NULL)) {
> +        if (str[3] == '\0') {
> +            error_setg(errp, "invalid file descriptor address\n");
> +            goto fail;
> +        } else {
> +            addr->kind = SOCKET_ADDRESS_KIND_FD;
> +            addr->fd = g_new(String, 1);
> +            addr->fd->str = g_strdup(str + 3);
> +        }
> +    } else {
> +        addr->kind = SOCKET_ADDRESS_KIND_INET;
> +        addr->inet = g_new(IPSocketAddress, 1);
> +        addr->inet = inet_parse(str, errp);
> +        if (addr->inet == NULL) {
> +            goto fail;
> +        }
> +    }
> +    return addr;
> +
> +fail:
> +    qapi_free_SocketAddress(addr);
> +    return NULL;
> +}
> +
> +int socket_connect(SocketAddress *addr, Error **errp)
> +{
> +    QemuOpts *opts;
> +    int fd;
> +
> +    opts = qemu_opts_create(&dummy_opts, NULL, 0, NULL);
> +    switch (addr->kind) {
> +    case SOCKET_ADDRESS_KIND_INET:
> +        inet_addr_to_opts(opts, addr->inet);
> +        fd = inet_connect_opts(opts, true, NULL, errp, NULL);
> +        break;
> +
> +    case SOCKET_ADDRESS_KIND_UNIX:
> +        qemu_opt_set(opts, "path", addr->q_unix->path);
> +        fd = unix_connect_opts(opts, errp);
> +        break;
> +
> +    case SOCKET_ADDRESS_KIND_FD:
> +        fd = monitor_get_fd(cur_mon, addr->fd->str, errp);
> +        break;
> +
> +    default:
> +        abort();
> +    }
> +    qemu_opts_del(opts);
> +    return fd;
> +}
> +
> +int socket_listen(SocketAddress *addr, Error **errp)
> +{
> +    QemuOpts *opts;
> +    int fd;
> +
> +    opts = qemu_opts_create(&dummy_opts, NULL, 0, NULL);
> +    switch (addr->kind) {
> +    case SOCKET_ADDRESS_KIND_INET:
> +        inet_addr_to_opts(opts, addr->inet);
> +        fd = inet_listen_opts(opts, 0, errp);
> +        break;
> +
> +    case SOCKET_ADDRESS_KIND_UNIX:
> +        qemu_opt_set(opts, "path", addr->q_unix->path);
> +        fd = unix_listen_opts(opts, errp);
> +        break;
> +
> +    case SOCKET_ADDRESS_KIND_FD:
> +        fd = monitor_get_fd(cur_mon, addr->fd->str, errp);
> +        break;
> +
> +    default:
> +        abort();
> +    }
> +    qemu_opts_del(opts);
> +    return fd;
> +}
> +
>  #ifdef _WIN32
>  static void socket_cleanup(void)
>  {
> diff --git a/qemu-tool.c b/qemu-tool.c
> index 18205ba..7658fde 100644
> --- a/qemu-tool.c
> +++ b/qemu-tool.c
> @@ -42,6 +42,12 @@ int monitor_cur_is_qmp(void)
>      return 0;
>  }
>  
> +int monitor_get_fd(Monitor *mon, const char *name, Error **errp)
> +{
> +    error_setg(errp, "only QEMU supports file descriptor passing");
> +    return -1;
> +}
> +
>  void monitor_set_error(Monitor *mon, QError *qerror)
>  {
>  }
> diff --git a/qemu_socket.h b/qemu_socket.h
> index afe8689..cd083d0 100644
> --- a/qemu_socket.h
> +++ b/qemu_socket.h
> @@ -61,6 +61,10 @@ int unix_listen(const char *path, char *ostr, int olen, Error **errp);
>  int unix_connect_opts(QemuOpts *opts, Error **errp);
>  int unix_connect(const char *path, Error **errp);
>  
> +SocketAddress *socket_parse(const char *str, Error **errp);
> +int socket_connect(SocketAddress *addr, Error **errp);
> +int socket_listen(SocketAddress *addr, Error **errp);
> +
>  /* Old, ipv4 only bits.  Don't use for new code. */
>  int parse_host_port(struct sockaddr_in *saddr, const char *str);
>  int socket_init(void);

[Qemu-devel] [PATCH v2 7/9] block: add close notifiers

[Paolo Bonzini]

The first user of close notifiers will be the embedded NBD server.
It is possible to use them to do some of the ad hoc processing
(e.g. for block jobs and I/O limits) that is currently done by
bdrv_close.

Acked-by: Kevin Wolf <kwolf@redhat.com>
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 Makefile.objs |  3 ++-
 block.c       | 19 ++++++++++++++-----
 block.h       |  1 +
 block_int.h   |  2 ++
 4 file modificati, 19 inserzioni(+), 6 rimozioni(-)

diff --git a/Makefile.objs b/Makefile.objs
index 03da150..124f783 100644
--- a/Makefile.objs
+++ b/Makefile.objs
@@ -43,6 +43,7 @@ coroutine-obj-$(CONFIG_WIN32) += coroutine-win32.o
 
 block-obj-y = cutils.o iov.o cache-utils.o qemu-option.o module.o async.o
 block-obj-y += nbd.o block.o aio.o aes.o qemu-config.o qemu-progress.o qemu-sockets.o
+block-obj-y += notify.o
 block-obj-y += $(coroutine-obj-y) $(qobject-obj-y) $(version-obj-y)
 block-obj-$(CONFIG_POSIX) += posix-aio-compat.o
 block-obj-$(CONFIG_LINUX_AIO) += linux-aio.o
@@ -92,7 +93,7 @@ common-obj-y += bt-host.o bt-vhci.o
 
 common-obj-y += iov.o acl.o
 common-obj-$(CONFIG_POSIX) += compatfd.o
-common-obj-y += notify.o event_notifier.o
+common-obj-y += event_notifier.o
 common-obj-y += qemu-timer.o qemu-timer-common.o
 
 common-obj-$(CONFIG_SLIRP) += slirp/
diff --git a/block.c b/block.c
index 751ebdc..795d359 100644
--- a/block.c
+++ b/block.c
@@ -28,6 +28,7 @@
 #include "block_int.h"
 #include "module.h"
 #include "qjson.h"
+#include "notify.h"
 #include "qemu-coroutine.h"
 #include "qmp-commands.h"
 #include "qemu-timer.h"
@@ -310,9 +311,16 @@ BlockDriverState *bdrv_new(const char *device_name)
         QTAILQ_INSERT_TAIL(&bdrv_states, bs, list);
     }
     bdrv_iostatus_disable(bs);
+    notifier_list_init(&bs->close_notifiers);
+
     return bs;
 }
 
+void bdrv_add_close_notifier(BlockDriverState *bs, Notifier *notify)
+{
+    notifier_list_add(&bs->close_notifiers, notify);
+}
+
 BlockDriver *bdrv_find_format(const char *format_name)
 {
     BlockDriver *drv1;
@@ -1096,12 +1104,13 @@ void bdrv_reopen_abort(BDRVReopenState *reopen_state)
 void bdrv_close(BlockDriverState *bs)
 {
     bdrv_flush(bs);
-    if (bs->drv) {
-        if (bs->job) {
-            block_job_cancel_sync(bs->job);
-        }
-        bdrv_drain_all();
+    if (bs->job) {
+        block_job_cancel_sync(bs->job);
+    }
+    bdrv_drain_all();
+    notifier_list_notify(&bs->close_notifiers, bs);
 
+    if (bs->drv) {
         if (bs == bs_snapshots) {
             bs_snapshots = NULL;
         }
diff --git a/block.h b/block.h
index b1095d8..17d8b00 100644
--- a/block.h
+++ b/block.h
@@ -149,6 +149,7 @@ int bdrv_reopen_prepare(BDRVReopenState *reopen_state,
 void bdrv_reopen_commit(BDRVReopenState *reopen_state);
 void bdrv_reopen_abort(BDRVReopenState *reopen_state);
 void bdrv_close(BlockDriverState *bs);
+void bdrv_add_close_notifier(BlockDriverState *bs, Notifier *notify);
 int bdrv_attach_dev(BlockDriverState *bs, void *dev);
 void bdrv_attach_dev_nofail(BlockDriverState *bs, void *dev);
 void bdrv_detach_dev(BlockDriverState *bs, void *dev);
diff --git a/block_int.h b/block_int.h
index ac4245c..41754ce 100644
--- a/block_int.h
+++ b/block_int.h
@@ -299,6 +299,8 @@ struct BlockDriverState {
     BlockDriverState *backing_hd;
     BlockDriverState *file;
 
+    NotifierList close_notifiers;
+
     /* number of in-flight copy-on-read requests */
     unsigned int copy_on_read_in_flight;
 
-- 
1.7.12

[Qemu-devel] [PATCH v2 8/9] qmp: add NBD server commands

[Paolo Bonzini]

Adding an NBD server inside QEMU is trivial, since all the logic is
in nbd.c and can be shared easily between qemu-nbd and QEMU itself.
The main difference is that qemu-nbd serves a single unnamed export,
while QEMU serves named exports.

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 Makefile.objs    |   2 +-
 blockdev-nbd.c   | 119 +++++++++++++++++++++++++++++++++++++++++++++++++++++++
 qapi-schema.json |  43 ++++++++++++++++++++
 qmp-commands.hx  |  16 ++++++++
 4 file modificati, 179 inserzioni(+). 1 rimozione(-)
 create mode 100644 blockdev-nbd.c

diff --git a/Makefile.objs b/Makefile.objs
index 124f783..40384ff 100644
--- a/Makefile.objs
+++ b/Makefile.objs
@@ -61,7 +61,7 @@ endif
 # suppress *all* target specific code in case of system emulation, i.e. a
 # single QEMU executable should support all CPUs and machines.
 
-common-obj-y = $(block-obj-y) blockdev.o
+common-obj-y = $(block-obj-y) blockdev.o blockdev-nbd.o
 common-obj-y += net.o net/
 common-obj-y += qom/
 common-obj-y += readline.o console.o cursor.o
diff --git a/blockdev-nbd.c b/blockdev-nbd.c
new file mode 100644
index 0000000..8031813
--- /dev/null
+++ b/blockdev-nbd.c
@@ -0,0 +1,119 @@
+/*
+ * Serving QEMU block devices via NBD
+ *
+ * Copyright (c) 2012 Red Hat, Inc.
+ *
+ * Author: Paolo Bonzini <pbonzini@redhat.com>
+ *
+ * This work is licensed under the terms of the GNU GPL, version 2 or
+ * later.  See the COPYING file in the top-level directory.
+ */
+
+#include "blockdev.h"
+#include "hw/block-common.h"
+#include "monitor.h"
+#include "qerror.h"
+#include "sysemu.h"
+#include "qmp-commands.h"
+#include "trace.h"
+#include "nbd.h"
+#include "qemu_socket.h"
+
+static int server_fd = -1;
+
+static void nbd_accept(void *opaque)
+{
+    struct sockaddr_in addr;
+    socklen_t addr_len = sizeof(addr);
+
+    int fd = accept(server_fd, (struct sockaddr *)&addr, &addr_len);
+    if (fd >= 0) {
+        nbd_client_new(NULL, fd, nbd_client_put);
+    }
+}
+
+void qmp_nbd_server_start(SocketAddress *addr, Error **errp)
+{
+    if (server_fd != -1) {
+        error_setg(errp, "NBD server already running");
+        return;
+    }
+
+    server_fd = socket_listen(addr, errp);
+    if (server_fd != -1) {
+        qemu_set_fd_handler2(server_fd, NULL, nbd_accept, NULL, NULL);
+    }
+}
+
+/* Hook into the BlockDriverState notifiers to close the export when
+ * the file is closed.
+ */
+typedef struct NBDCloseNotifier {
+    Notifier n;
+    NBDExport *exp;
+    QTAILQ_ENTRY(NBDCloseNotifier) next;
+} NBDCloseNotifier;
+
+static QTAILQ_HEAD(, NBDCloseNotifier) close_notifiers =
+    QTAILQ_HEAD_INITIALIZER(close_notifiers);
+
+static void nbd_close_notifier(Notifier *n, void *data)
+{
+    NBDCloseNotifier *cn = DO_UPCAST(NBDCloseNotifier, n, n);
+
+    notifier_remove(&cn->n);
+    QTAILQ_REMOVE(&close_notifiers, cn, next);
+
+    nbd_export_close(cn->exp);
+    nbd_export_put(cn->exp);
+    g_free(cn);
+}
+
+static void nbd_server_put_ref(NBDExport *exp)
+{
+    BlockDriverState *bs = nbd_export_get_blockdev(exp);
+    drive_put_ref(drive_get_by_blockdev(bs));
+}
+
+void qmp_nbd_server_add(const char *device, bool has_writable, bool writable,
+                        Error **errp)
+{
+    BlockDriverState *bs;
+    NBDExport *exp;
+    NBDCloseNotifier *n;
+
+    if (nbd_export_find(device)) {
+        error_setg(errp, "NBD server already exporting device '%s'", device);
+        return;
+    }
+
+    bs = bdrv_find(device);
+    if (!bs) {
+        error_set(errp, QERR_DEVICE_NOT_FOUND, device);
+        return;
+    }
+
+    exp = nbd_export_new(bs, 0, -1, writable ? 0 : NBD_FLAG_READ_ONLY,
+                         nbd_server_put_ref);
+
+    nbd_export_set_name(exp, device);
+    drive_get_ref(drive_get_by_blockdev(bs));
+
+    n = g_malloc0(sizeof(NBDCloseNotifier));
+    n->n.notify = nbd_close_notifier;
+    n->exp = exp;
+    bdrv_add_close_notifier(bs, &n->n);
+    QTAILQ_INSERT_TAIL(&close_notifiers, n, next);
+}
+
+void qmp_nbd_server_stop(Error **errp)
+{
+    while (!QTAILQ_EMPTY(&close_notifiers)) {
+        NBDCloseNotifier *cn = QTAILQ_FIRST(&close_notifiers);
+        nbd_close_notifier(&cn->n, nbd_export_get_blockdev(cn->exp));
+    }
+
+    qemu_set_fd_handler2(server_fd, NULL, NULL, NULL, NULL);
+    close(server_fd);
+    server_fd = -1;
+}
diff --git a/qapi-schema.json b/qapi-schema.json
index b443a99..229421d 100644
--- a/qapi-schema.json
+++ b/qapi-schema.json
@@ -2697,3 +2697,46 @@
 # Since: 0.14.0
 ##
 { 'command': 'screendump', 'data': {'filename': 'str'} }
+
+##
+# @nbd-server-start:
+#
+# Start an NBD server listening on the given host and port.  Block
+# devices can then be exported using @nbd-server-add.  The NBD
+# server will present them as named exports; for example, another
+# QEMU instance could refer to them as "nbd:HOST:PORT:exportname=NAME".
+#
+# @addr: Address on which to listen.
+#
+# Returns: error if the server is already running.
+#
+# Since: 1.3.0
+##
+{ 'command': 'nbd-server-start',
+  'data': { 'addr': 'SocketAddress' } }
+
+##
+# @nbd-server-add:
+#
+# Export a device to QEMU's embedded NBD server.
+#
+# @device: Block device to be exported
+#
+# @writable: Whether clients should be able to write to the device via the
+#     NBD connection (default false). #optional
+#
+# Returns: error if the device is already marked for export.
+#
+# Since: 1.3.0
+##
+{ 'command': 'nbd-server-add', 'data': {'device': 'str', '*writable': 'bool'} }
+
+##
+# @nbd-server-stop:
+#
+# Stop QEMU's embedded NBD server, and unregister all devices previously
+# added via @nbd-server-add.
+#
+# Since: 1.3.0
+##
+{ 'command': 'nbd-server-stop' }
diff --git a/qmp-commands.hx b/qmp-commands.hx
index 36e08d9..c394af0 100644
--- a/qmp-commands.hx
+++ b/qmp-commands.hx
@@ -2503,6 +2503,22 @@ EQMP
     },
 
     {
+        .name       = "nbd-server-start",
+        .args_type  = "addr:q",
+        .mhandler.cmd_new = qmp_marshal_input_nbd_server_start,
+    },
+    {
+        .name       = "nbd-server-add",
+        .args_type  = "device:B,writable:b?",
+        .mhandler.cmd_new = qmp_marshal_input_nbd_server_add,
+    },
+    {
+        .name       = "nbd-server-stop",
+        .args_type  = "",
+        .mhandler.cmd_new = qmp_marshal_input_nbd_server_stop,
+    },
+
+    {
         .name       = "change-vnc-password",
         .args_type  = "password:s",
         .mhandler.cmd_new = qmp_marshal_input_change_vnc_password,
-- 
1.7.12

Re: [Qemu-devel] [PATCH v2 8/9] qmp: add NBD server commands

[Eric Blake]

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

On 10/01/2012 08:52 AM, Paolo Bonzini wrote:
> Adding an NBD server inside QEMU is trivial, since all the logic is
> in nbd.c and can be shared easily between qemu-nbd and QEMU itself.
> The main difference is that qemu-nbd serves a single unnamed export,
> while QEMU serves named exports.
> 

> +# @nbd-server-add:
> +#
> +# Export a device to QEMU's embedded NBD server.
> +#
> +# @device: Block device to be exported
> +#
> +# @writable: Whether clients should be able to write to the device via the
> +#     NBD connection (default false). #optional

#optional should come right after @writable:

-- 
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: 617 bytes --]

Re: [Qemu-devel] [PATCH v2 8/9] qmp: add NBD server commands

[Luiz Capitulino]

On Mon,  1 Oct 2012 16:52:23 +0200
Paolo Bonzini <pbonzini@redhat.com> wrote:

> Adding an NBD server inside QEMU is trivial, since all the logic is
> in nbd.c and can be shared easily between qemu-nbd and QEMU itself.
> The main difference is that qemu-nbd serves a single unnamed export,
> while QEMU serves named exports.
> 
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>

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

> ---
>  Makefile.objs    |   2 +-
>  blockdev-nbd.c   | 119 +++++++++++++++++++++++++++++++++++++++++++++++++++++++
>  qapi-schema.json |  43 ++++++++++++++++++++
>  qmp-commands.hx  |  16 ++++++++
>  4 file modificati, 179 inserzioni(+). 1 rimozione(-)
>  create mode 100644 blockdev-nbd.c
> 
> diff --git a/Makefile.objs b/Makefile.objs
> index 124f783..40384ff 100644
> --- a/Makefile.objs
> +++ b/Makefile.objs
> @@ -61,7 +61,7 @@ endif
>  # suppress *all* target specific code in case of system emulation, i.e. a
>  # single QEMU executable should support all CPUs and machines.
>  
> -common-obj-y = $(block-obj-y) blockdev.o
> +common-obj-y = $(block-obj-y) blockdev.o blockdev-nbd.o
>  common-obj-y += net.o net/
>  common-obj-y += qom/
>  common-obj-y += readline.o console.o cursor.o
> diff --git a/blockdev-nbd.c b/blockdev-nbd.c
> new file mode 100644
> index 0000000..8031813
> --- /dev/null
> +++ b/blockdev-nbd.c
> @@ -0,0 +1,119 @@
> +/*
> + * Serving QEMU block devices via NBD
> + *
> + * Copyright (c) 2012 Red Hat, Inc.
> + *
> + * Author: Paolo Bonzini <pbonzini@redhat.com>
> + *
> + * This work is licensed under the terms of the GNU GPL, version 2 or
> + * later.  See the COPYING file in the top-level directory.
> + */
> +
> +#include "blockdev.h"
> +#include "hw/block-common.h"
> +#include "monitor.h"
> +#include "qerror.h"
> +#include "sysemu.h"
> +#include "qmp-commands.h"
> +#include "trace.h"
> +#include "nbd.h"
> +#include "qemu_socket.h"
> +
> +static int server_fd = -1;
> +
> +static void nbd_accept(void *opaque)
> +{
> +    struct sockaddr_in addr;
> +    socklen_t addr_len = sizeof(addr);
> +
> +    int fd = accept(server_fd, (struct sockaddr *)&addr, &addr_len);
> +    if (fd >= 0) {
> +        nbd_client_new(NULL, fd, nbd_client_put);
> +    }
> +}
> +
> +void qmp_nbd_server_start(SocketAddress *addr, Error **errp)
> +{
> +    if (server_fd != -1) {
> +        error_setg(errp, "NBD server already running");
> +        return;
> +    }
> +
> +    server_fd = socket_listen(addr, errp);
> +    if (server_fd != -1) {
> +        qemu_set_fd_handler2(server_fd, NULL, nbd_accept, NULL, NULL);
> +    }
> +}
> +
> +/* Hook into the BlockDriverState notifiers to close the export when
> + * the file is closed.
> + */
> +typedef struct NBDCloseNotifier {
> +    Notifier n;
> +    NBDExport *exp;
> +    QTAILQ_ENTRY(NBDCloseNotifier) next;
> +} NBDCloseNotifier;
> +
> +static QTAILQ_HEAD(, NBDCloseNotifier) close_notifiers =
> +    QTAILQ_HEAD_INITIALIZER(close_notifiers);
> +
> +static void nbd_close_notifier(Notifier *n, void *data)
> +{
> +    NBDCloseNotifier *cn = DO_UPCAST(NBDCloseNotifier, n, n);
> +
> +    notifier_remove(&cn->n);
> +    QTAILQ_REMOVE(&close_notifiers, cn, next);
> +
> +    nbd_export_close(cn->exp);
> +    nbd_export_put(cn->exp);
> +    g_free(cn);
> +}
> +
> +static void nbd_server_put_ref(NBDExport *exp)
> +{
> +    BlockDriverState *bs = nbd_export_get_blockdev(exp);
> +    drive_put_ref(drive_get_by_blockdev(bs));
> +}
> +
> +void qmp_nbd_server_add(const char *device, bool has_writable, bool writable,
> +                        Error **errp)
> +{
> +    BlockDriverState *bs;
> +    NBDExport *exp;
> +    NBDCloseNotifier *n;
> +
> +    if (nbd_export_find(device)) {
> +        error_setg(errp, "NBD server already exporting device '%s'", device);
> +        return;
> +    }
> +
> +    bs = bdrv_find(device);
> +    if (!bs) {
> +        error_set(errp, QERR_DEVICE_NOT_FOUND, device);
> +        return;
> +    }
> +
> +    exp = nbd_export_new(bs, 0, -1, writable ? 0 : NBD_FLAG_READ_ONLY,
> +                         nbd_server_put_ref);
> +
> +    nbd_export_set_name(exp, device);
> +    drive_get_ref(drive_get_by_blockdev(bs));
> +
> +    n = g_malloc0(sizeof(NBDCloseNotifier));
> +    n->n.notify = nbd_close_notifier;
> +    n->exp = exp;
> +    bdrv_add_close_notifier(bs, &n->n);
> +    QTAILQ_INSERT_TAIL(&close_notifiers, n, next);
> +}
> +
> +void qmp_nbd_server_stop(Error **errp)
> +{
> +    while (!QTAILQ_EMPTY(&close_notifiers)) {
> +        NBDCloseNotifier *cn = QTAILQ_FIRST(&close_notifiers);
> +        nbd_close_notifier(&cn->n, nbd_export_get_blockdev(cn->exp));
> +    }
> +
> +    qemu_set_fd_handler2(server_fd, NULL, NULL, NULL, NULL);
> +    close(server_fd);
> +    server_fd = -1;
> +}
> diff --git a/qapi-schema.json b/qapi-schema.json
> index b443a99..229421d 100644
> --- a/qapi-schema.json
> +++ b/qapi-schema.json
> @@ -2697,3 +2697,46 @@
>  # Since: 0.14.0
>  ##
>  { 'command': 'screendump', 'data': {'filename': 'str'} }
> +
> +##
> +# @nbd-server-start:
> +#
> +# Start an NBD server listening on the given host and port.  Block
> +# devices can then be exported using @nbd-server-add.  The NBD
> +# server will present them as named exports; for example, another
> +# QEMU instance could refer to them as "nbd:HOST:PORT:exportname=NAME".
> +#
> +# @addr: Address on which to listen.
> +#
> +# Returns: error if the server is already running.
> +#
> +# Since: 1.3.0
> +##
> +{ 'command': 'nbd-server-start',
> +  'data': { 'addr': 'SocketAddress' } }
> +
> +##
> +# @nbd-server-add:
> +#
> +# Export a device to QEMU's embedded NBD server.
> +#
> +# @device: Block device to be exported
> +#
> +# @writable: Whether clients should be able to write to the device via the
> +#     NBD connection (default false). #optional
> +#
> +# Returns: error if the device is already marked for export.
> +#
> +# Since: 1.3.0
> +##
> +{ 'command': 'nbd-server-add', 'data': {'device': 'str', '*writable': 'bool'} }
> +
> +##
> +# @nbd-server-stop:
> +#
> +# Stop QEMU's embedded NBD server, and unregister all devices previously
> +# added via @nbd-server-add.
> +#
> +# Since: 1.3.0
> +##
> +{ 'command': 'nbd-server-stop' }
> diff --git a/qmp-commands.hx b/qmp-commands.hx
> index 36e08d9..c394af0 100644
> --- a/qmp-commands.hx
> +++ b/qmp-commands.hx
> @@ -2503,6 +2503,22 @@ EQMP
>      },
>  
>      {
> +        .name       = "nbd-server-start",
> +        .args_type  = "addr:q",
> +        .mhandler.cmd_new = qmp_marshal_input_nbd_server_start,
> +    },
> +    {
> +        .name       = "nbd-server-add",
> +        .args_type  = "device:B,writable:b?",
> +        .mhandler.cmd_new = qmp_marshal_input_nbd_server_add,
> +    },
> +    {
> +        .name       = "nbd-server-stop",
> +        .args_type  = "",
> +        .mhandler.cmd_new = qmp_marshal_input_nbd_server_stop,
> +    },
> +
> +    {
>          .name       = "change-vnc-password",
>          .args_type  = "password:s",
>          .mhandler.cmd_new = qmp_marshal_input_change_vnc_password,

Re: [Qemu-devel] [PATCH v2 8/9] qmp: add NBD server commands

[Christoph Hellwig]

On Mon, Oct 01, 2012 at 04:52:23PM +0200, Paolo Bonzini wrote:
> Adding an NBD server inside QEMU is trivial, since all the logic is
> in nbd.c and can be shared easily between qemu-nbd and QEMU itself.
> The main difference is that qemu-nbd serves a single unnamed export,
> while QEMU serves named exports.

I've started playing around with qemu-nbd and have started looking into
named exports, aio, and adding snapshort support.  I'm wondering if instead
of adding this I should try to base it on your embedded server.  That would
give me the named exports, aio and monitor/qmp based snapshot creatation.

The only thing to add would be a qemu mode where it doesn't run an
actual guest.

Does anyone have opinions on this?

Re: [Qemu-devel] [PATCH v2 8/9] qmp: add NBD server commands

[Paolo Bonzini]

Il 31/10/2012 12:23, Christoph Hellwig ha scritto:
> On Mon, Oct 01, 2012 at 04:52:23PM +0200, Paolo Bonzini wrote:
>> Adding an NBD server inside QEMU is trivial, since all the logic is
>> in nbd.c and can be shared easily between qemu-nbd and QEMU itself.
>> The main difference is that qemu-nbd serves a single unnamed export,
>> while QEMU serves named exports.
> 
> I've started playing around with qemu-nbd and have started looking into
> named exports, aio, and adding snapshort support.  I'm wondering if instead
> of adding this I should try to base it on your embedded server.

qemu-nbd does support AIO in the latest versions.  There's also
--cache=MODE and --aio=MODE command-line options.  In fact, a large part
of the code is shared between qemu-nbd and the embedded server. The only
difference is that qemu-nbd uses unnamed exports, while qemu names them.
 qemu-nbd also has options to show only one partition, which you
probably do not need.

But if you need a QMP interface, adding it to qemu-nbd would really be a
bad idea. :)

> That would give me the named exports, aio and monitor/qmp based
> snapshot creation.
> 
> The only thing to add would be a qemu mode where it doesn't run an
> actual guest.

You can use qtest mode to get very close to this (even if you send
stop/cont by mistake on the monitor, no code will actually run):

qemu-system-x86_64 -chardev file,id=null,path=/dev/null -qtest null
-machine accel=qtest -m 1 -nodefaults -nographic

but having a separate do-nothing target would probably be nicer...
though Anthony may have different opinions.

Paolo

> Does anyone have opinions on this?

Re: [Qemu-devel] [PATCH v2 8/9] qmp: add NBD server commands

[Christoph Hellwig]

On Wed, Oct 31, 2012 at 01:46:22PM +0100, Paolo Bonzini wrote:
> qemu-nbd does support AIO in the latest versions.  There's also
> --cache=MODE and --aio=MODE command-line options.

Oh true, it's just hidden behind coroutines.  With --aio-native and
--nocache I actually get fairly reasonable performance out of it now.

> But if you need a QMP interface, adding it to qemu-nbd would really be a
> bad idea. :)

I don't nessecarily need QMP, I'm just looking for a way to create a
snapshot underneath an exported image.

> You can use qtest mode to get very close to this (even if you send
> stop/cont by mistake on the monitor, no code will actually run):
> 
> qemu-system-x86_64 -chardev file,id=null,path=/dev/null -qtest null
> -machine accel=qtest -m 1 -nodefaults -nographic
> 
> but having a separate do-nothing target would probably be nicer...
> though Anthony may have different opinions.

That looks fairlt reasonable, thanks.

[Qemu-devel] [PATCH v2 9/9] hmp: add NBD server commands

[Paolo Bonzini]

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 hmp-commands.hx | 29 +++++++++++++++++++++++++++++
 hmp.c           | 55 +++++++++++++++++++++++++++++++++++++++++++++++++++++++
 hmp.h           |  2 ++
 3 file modificati, 86 inserzioni(+)

diff --git a/hmp-commands.hx b/hmp-commands.hx
index ed67e99..b0af484 100644
--- a/hmp-commands.hx
+++ b/hmp-commands.hx
@@ -1247,6 +1247,35 @@ Remove all matches from the access control list, and set the default
 policy back to @code{deny}.
 ETEXI
 
+    {
+        .name       = "nbd_server_start",
+        .args_type  = "writable:-w,uri:s",
+        .params     = "nbd_server_start [-w] host:port",
+        .help       = "serve block devices on the given host and port",
+        .mhandler.cmd = hmp_nbd_server_start,
+    },
+STEXI
+@item nbd_server_start @var{host}:@var{port}
+@findex nbd_server_start
+Start an NBD server on the given host and/or port, and serve all of the
+virtual machine's block devices that have an inserted media on it.
+The @option{-w} option makes the devices writable.
+ETEXI
+
+    {
+        .name       = "nbd_server_stop",
+        .args_type  = "",
+        .params     = "nbd_server_stop",
+        .help       = "stop serving block devices using the NBD protocol",
+        .mhandler.cmd = hmp_nbd_server_stop,
+    },
+STEXI
+@item nbd_server_stop
+@findex nbd_server_stop
+Stop the QEMU embedded NBD server.
+ETEXI
+
+
 #if defined(TARGET_I386)
 
     {
diff --git a/hmp.c b/hmp.c
index ba6fbd3..3d27628 100644
--- a/hmp.c
+++ b/hmp.c
@@ -18,6 +18,7 @@
 #include "qemu-option.h"
 #include "qemu-timer.h"
 #include "qmp-commands.h"
+#include "qemu_socket.h"
 #include "monitor.h"
 #include "console.h"
 
@@ -1168,3 +1169,57 @@ void hmp_screen_dump(Monitor *mon, const QDict *qdict)
     qmp_screendump(filename, &err);
     hmp_handle_error(mon, &err);
 }
+
+void hmp_nbd_server_start(Monitor *mon, const QDict *qdict)
+{
+    const char *uri = qdict_get_str(qdict, "uri");
+    int writable = qdict_get_try_bool(qdict, "writable", 0);
+    Error *local_err = NULL;
+    BlockInfoList *block_list, *info;
+    SocketAddress *addr;
+
+    /* First check if the address is valid and start the server.  */
+    addr = socket_parse(uri, &local_err);
+    if (local_err != NULL) {
+        goto exit;
+    }
+
+    qmp_nbd_server_start(addr, &local_err);
+    qapi_free_SocketAddress(addr);
+    if (local_err != NULL) {
+        goto exit;
+    }
+
+    /* Then try adding all block devices.  If one fails, close all and
+     * exit.
+     */
+    block_list = qmp_query_block(NULL);
+
+    for (info = block_list; info; info = info->next) {
+        if (!info->value->has_inserted) {
+            continue;
+        }
+
+        qmp_nbd_server_add(info->value->device,
+                           true, !info->value->inserted->ro && writable,
+                           &local_err);
+
+        if (local_err != NULL) {
+            qmp_nbd_server_stop(NULL);
+            break;
+        }
+    }
+
+    qapi_free_BlockInfoList(block_list);
+
+exit:
+    hmp_handle_error(mon, &local_err);
+}
+
+void hmp_nbd_server_stop(Monitor *mon, const QDict *qdict)
+{
+    Error *errp = NULL;
+
+    qmp_nbd_server_stop(&errp);
+    hmp_handle_error(mon, &errp);
+}
diff --git a/hmp.h b/hmp.h
index 48b9c59..0d02479 100644
--- a/hmp.h
+++ b/hmp.h
@@ -73,5 +73,7 @@ void hmp_getfd(Monitor *mon, const QDict *qdict);
 void hmp_closefd(Monitor *mon, const QDict *qdict);
 void hmp_send_key(Monitor *mon, const QDict *qdict);
 void hmp_screen_dump(Monitor *mon, const QDict *qdict);
+void hmp_nbd_server_start(Monitor *mon, const QDict *qdict);
+void hmp_nbd_server_stop(Monitor *mon, const QDict *qdict);
 
 #endif
-- 
1.7.12

Re: [Qemu-devel] [PATCH v2 9/9] hmp: add NBD server commands

[Luiz Capitulino]

On Mon,  1 Oct 2012 16:52:24 +0200
Paolo Bonzini <pbonzini@redhat.com> wrote:

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

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

> ---
>  hmp-commands.hx | 29 +++++++++++++++++++++++++++++
>  hmp.c           | 55 +++++++++++++++++++++++++++++++++++++++++++++++++++++++
>  hmp.h           |  2 ++
>  3 file modificati, 86 inserzioni(+)
> 
> diff --git a/hmp-commands.hx b/hmp-commands.hx
> index ed67e99..b0af484 100644
> --- a/hmp-commands.hx
> +++ b/hmp-commands.hx
> @@ -1247,6 +1247,35 @@ Remove all matches from the access control list, and set the default
>  policy back to @code{deny}.
>  ETEXI
>  
> +    {
> +        .name       = "nbd_server_start",
> +        .args_type  = "writable:-w,uri:s",
> +        .params     = "nbd_server_start [-w] host:port",
> +        .help       = "serve block devices on the given host and port",
> +        .mhandler.cmd = hmp_nbd_server_start,
> +    },
> +STEXI
> +@item nbd_server_start @var{host}:@var{port}
> +@findex nbd_server_start
> +Start an NBD server on the given host and/or port, and serve all of the
> +virtual machine's block devices that have an inserted media on it.
> +The @option{-w} option makes the devices writable.
> +ETEXI
> +
> +    {
> +        .name       = "nbd_server_stop",
> +        .args_type  = "",
> +        .params     = "nbd_server_stop",
> +        .help       = "stop serving block devices using the NBD protocol",
> +        .mhandler.cmd = hmp_nbd_server_stop,
> +    },
> +STEXI
> +@item nbd_server_stop
> +@findex nbd_server_stop
> +Stop the QEMU embedded NBD server.
> +ETEXI
> +
> +
>  #if defined(TARGET_I386)
>  
>      {
> diff --git a/hmp.c b/hmp.c
> index ba6fbd3..3d27628 100644
> --- a/hmp.c
> +++ b/hmp.c
> @@ -18,6 +18,7 @@
>  #include "qemu-option.h"
>  #include "qemu-timer.h"
>  #include "qmp-commands.h"
> +#include "qemu_socket.h"
>  #include "monitor.h"
>  #include "console.h"
>  
> @@ -1168,3 +1169,57 @@ void hmp_screen_dump(Monitor *mon, const QDict *qdict)
>      qmp_screendump(filename, &err);
>      hmp_handle_error(mon, &err);
>  }
> +
> +void hmp_nbd_server_start(Monitor *mon, const QDict *qdict)
> +{
> +    const char *uri = qdict_get_str(qdict, "uri");
> +    int writable = qdict_get_try_bool(qdict, "writable", 0);
> +    Error *local_err = NULL;
> +    BlockInfoList *block_list, *info;
> +    SocketAddress *addr;
> +
> +    /* First check if the address is valid and start the server.  */
> +    addr = socket_parse(uri, &local_err);
> +    if (local_err != NULL) {
> +        goto exit;
> +    }
> +
> +    qmp_nbd_server_start(addr, &local_err);
> +    qapi_free_SocketAddress(addr);
> +    if (local_err != NULL) {
> +        goto exit;
> +    }
> +
> +    /* Then try adding all block devices.  If one fails, close all and
> +     * exit.
> +     */
> +    block_list = qmp_query_block(NULL);
> +
> +    for (info = block_list; info; info = info->next) {
> +        if (!info->value->has_inserted) {
> +            continue;
> +        }
> +
> +        qmp_nbd_server_add(info->value->device,
> +                           true, !info->value->inserted->ro && writable,
> +                           &local_err);
> +
> +        if (local_err != NULL) {
> +            qmp_nbd_server_stop(NULL);
> +            break;
> +        }
> +    }
> +
> +    qapi_free_BlockInfoList(block_list);
> +
> +exit:
> +    hmp_handle_error(mon, &local_err);
> +}
> +
> +void hmp_nbd_server_stop(Monitor *mon, const QDict *qdict)
> +{
> +    Error *errp = NULL;
> +
> +    qmp_nbd_server_stop(&errp);
> +    hmp_handle_error(mon, &errp);
> +}
> diff --git a/hmp.h b/hmp.h
> index 48b9c59..0d02479 100644
> --- a/hmp.h
> +++ b/hmp.h
> @@ -73,5 +73,7 @@ void hmp_getfd(Monitor *mon, const QDict *qdict);
>  void hmp_closefd(Monitor *mon, const QDict *qdict);
>  void hmp_send_key(Monitor *mon, const QDict *qdict);
>  void hmp_screen_dump(Monitor *mon, const QDict *qdict);
> +void hmp_nbd_server_start(Monitor *mon, const QDict *qdict);
> +void hmp_nbd_server_stop(Monitor *mon, const QDict *qdict);
>  
>  #endif

Re: [Qemu-devel] [PATCH v2 0/9] Embedded NBD server

[Luiz Capitulino]

On Mon,  1 Oct 2012 16:52:15 +0200
Paolo Bonzini <pbonzini@redhat.com> wrote:

> This series rebases the previous qemu-sockets patches for error
> propagation and uses the new QAPI-friendly socket functions in the
> embedded NBD server.  The changes are due to Orit's patches being
> now in, some early parts being in Luiz's queue, and glusterfs
> patches not having touched qemu-sockets.c in the end.
> 
> Patches 1 to 4 start moving qemu-sockets functions away from error_report
> (or printf) and away from QemuOpts.  Patch 5 makes it easier to reuse
> the address parser of inet_parse in the new socket_parse function.
> Patch 6 introduces QAPI-friendly socket parsing and creation functions.
> 
> Patches 7 and 8 introduces the QMP commands, and patch 9 introduces the
> HMP version.

Some comments against 4/9, plus this:

  CC    qemu-sockets.o
/home/lcapitulino/work/src/qmp-unstable/qemu-sockets.c: In function ‘socket_connect’:
/home/lcapitulino/work/src/qmp-unstable/qemu-sockets.c:894:9: error: passing argument 2 of ‘inet_connect_opts’ makes pointer from integer without a cast [-Werror]
/home/lcapitulino/work/src/qmp-unstable/qemu-sockets.c:365:5: note: expected ‘struct Error **’ but argument is of type ‘int’
/home/lcapitulino/work/src/qmp-unstable/qemu-sockets.c:894:9: error: too many arguments to function ‘inet_connect_opts’
/home/lcapitulino/work/src/qmp-unstable/qemu-sockets.c:365:5: note: declared here
cc1: all warnings being treated as errors
make: *** [qemu-sockets.o] Error 1
make: *** Waiting for unfinished jobs....

It's also worth it to mention that this series actually depends on my
last pull request, but it's ok if this is going through my tree.

Otherwise, looks good.

> 
> Paolo
> 
> Paolo Bonzini (9):
>   build: add QAPI files to the tools
>   qapi: add socket address types
>   qemu-sockets: add error propagation to inet_parse
>   qemu-sockets: add error propagation to Unix socket functions
>   qemu-sockets: return IPSocketAddress from inet_parse
>   qemu-sockets: add socket_listen, socket_connect, socket_parse
>   block: add close notifiers
>   qmp: add NBD server commands
>   hmp: add NBD server commands
> 
>  Makefile.objs       |   8 +-
>  block.c             |  19 +++-
>  block.h             |   1 +
>  block_int.h         |   2 +
>  blockdev-nbd.c      | 119 ++++++++++++++++++++++++
>  hmp-commands.hx     |  29 ++++++
>  hmp.c               |  55 +++++++++++
>  hmp.h               |   2 +
>  nbd.c               |   4 +-
>  qapi-schema.json    |  96 +++++++++++++++++++
>  qemu-char.c         |   4 +-
>  qemu-sockets.c      | 261 +++++++++++++++++++++++++++++++++++++++-------------
>  qemu-tool.c         |   6 ++
>  qemu_socket.h       |  12 ++-
>  qga/channel-posix.c |   2 +-
>  qmp-commands.hx     |  16 ++++
>  ui/vnc.c            |   4 +-
>  17 file modificati, 557 inserzioni(+), 83 rimozioni(-)
>  create mode 100644 blockdev-nbd.c
>