[Qemu-devel] [PATCH v1 0/2] Remove use of QemuOpts from the nbd code

[Qemu-devel] [PATCH v1 0/2] Remove use of QemuOpts from the nbd code

[Daniel P. Berrange]

The intent is for all the callers of the util/qemu-sockets.c APIs
to be converted to use the QAPI SocketAddress variants, with the
QemuOpts variants eventually being deleted entirely. With that in
mind, this small series converts the NBD client and server to use
the QAPI SocketAddress object. This only leaves the VNC server
using QemuOpts for socket setup, which will be converted shortly.

Daniel P. Berrange (2):
  nbd: convert to use the QAPI SocketAddress object
  qemu-nbd: convert to use the QAPI SocketAddress object

 block/nbd.c |  69 +++++++++++++++++++++-------------------
 qemu-nbd.c  | 102 ++++++++++++++++++++++--------------------------------------
 2 files changed, 75 insertions(+), 96 deletions(-)

-- 
2.4.3

[Qemu-devel] [PATCH v1 1/2] nbd: convert to use the QAPI SocketAddress object

[Daniel P. Berrange]

The nbd block driver currently uses a QemuOpts object
when setting up sockets. Switch it over to use the
QAPI SocketAddress object instead.

Signed-off-by: Daniel P. Berrange <berrange@redhat.com>
---
 block/nbd.c | 69 +++++++++++++++++++++++++++++++++----------------------------
 1 file changed, 37 insertions(+), 32 deletions(-)

diff --git a/block/nbd.c b/block/nbd.c
index 2176186..b3ba54a 100644
--- a/block/nbd.c
+++ b/block/nbd.c
@@ -43,7 +43,6 @@
 
 typedef struct BDRVNBDState {
     NbdClientSession client;
-    QemuOpts *socket_opts;
 } BDRVNBDState;
 
 static int nbd_parse_uri(const char *filename, QDict *options)
@@ -190,10 +189,10 @@ out:
     g_free(file);
 }
 
-static void nbd_config(BDRVNBDState *s, QDict *options, char **export,
-                       Error **errp)
+static SocketAddress *nbd_config(BDRVNBDState *s, QDict *options, char **export,
+                                 Error **errp)
 {
-    Error *local_err = NULL;
+    SocketAddress *saddr;
 
     if (qdict_haskey(options, "path") == qdict_haskey(options, "host")) {
         if (qdict_haskey(options, "path")) {
@@ -201,28 +200,37 @@ static void nbd_config(BDRVNBDState *s, QDict *options, char **export,
         } else {
             error_setg(errp, "one of path and host must be specified.");
         }
-        return;
+        return NULL;
     }
 
-    s->client.is_unix = qdict_haskey(options, "path");
-    s->socket_opts = qemu_opts_create(&socket_optslist, NULL, 0,
-                                      &error_abort);
+    saddr = g_new0(SocketAddress, 1);
 
-    qemu_opts_absorb_qdict(s->socket_opts, options, &local_err);
-    if (local_err) {
-        error_propagate(errp, local_err);
-        return;
+    if (qdict_haskey(options, "path")) {
+        saddr->kind = SOCKET_ADDRESS_KIND_UNIX;
+        saddr->q_unix = g_new0(UnixSocketAddress, 1);
+        saddr->q_unix->path = g_strdup(qdict_get_str(options, "path"));
+        qdict_del(options, "path");
+    } else {
+        saddr->kind = SOCKET_ADDRESS_KIND_INET;
+        saddr->inet = g_new0(InetSocketAddress, 1);
+        saddr->inet->host = g_strdup(qdict_get_str(options, "host"));
+        if (!qdict_get_try_str(options, "port")) {
+            saddr->inet->port = g_strdup_printf("%d", NBD_DEFAULT_PORT);
+        } else {
+            saddr->inet->port = g_strdup(qdict_get_str(options, "port"));
+        }
+        qdict_del(options, "host");
+        qdict_del(options, "port");
     }
 
-    if (!qemu_opt_get(s->socket_opts, "port")) {
-        qemu_opt_set_number(s->socket_opts, "port", NBD_DEFAULT_PORT,
-                            &error_abort);
-    }
+    s->client.is_unix = saddr->kind == SOCKET_ADDRESS_KIND_UNIX;
 
     *export = g_strdup(qdict_get_try_str(options, "export"));
     if (*export) {
         qdict_del(options, "export");
     }
+
+    return saddr;
 }
 
 NbdClientSession *nbd_get_client_session(BlockDriverState *bs)
@@ -231,26 +239,24 @@ NbdClientSession *nbd_get_client_session(BlockDriverState *bs)
     return &s->client;
 }
 
-static int nbd_establish_connection(BlockDriverState *bs, Error **errp)
+static int nbd_establish_connection(BlockDriverState *bs,
+                                    SocketAddress *saddr,
+                                    Error **errp)
 {
     BDRVNBDState *s = bs->opaque;
     int sock;
 
-    if (s->client.is_unix) {
-        sock = unix_connect_opts(s->socket_opts, errp, NULL, NULL);
-    } else {
-        sock = inet_connect_opts(s->socket_opts, errp, NULL, NULL);
-        if (sock >= 0) {
-            socket_set_nodelay(sock);
-        }
-    }
+    sock = socket_connect(saddr, errp, NULL, NULL);
 
-    /* Failed to establish connection */
     if (sock < 0) {
         logout("Failed to establish connection to NBD server\n");
         return -EIO;
     }
 
+    if (!s->client.is_unix) {
+        socket_set_nodelay(sock);
+    }
+
     return sock;
 }
 
@@ -261,10 +267,11 @@ static int nbd_open(BlockDriverState *bs, QDict *options, int flags,
     char *export = NULL;
     int result, sock;
     Error *local_err = NULL;
+    SocketAddress *saddr;
 
     /* Pop the config into our state object. Exit if invalid. */
-    nbd_config(s, options, &export, &local_err);
-    if (local_err) {
+    saddr = nbd_config(s, options, &export, &local_err);
+    if (!saddr) {
         error_propagate(errp, local_err);
         return -EINVAL;
     }
@@ -272,7 +279,8 @@ static int nbd_open(BlockDriverState *bs, QDict *options, int flags,
     /* establish TCP connection, return error if it fails
      * TODO: Configurable retry-until-timeout behaviour.
      */
-    sock = nbd_establish_connection(bs, errp);
+    sock = nbd_establish_connection(bs, saddr, errp);
+    qapi_free_SocketAddress(saddr);
     if (sock < 0) {
         g_free(export);
         return sock;
@@ -315,9 +323,6 @@ static int nbd_co_discard(BlockDriverState *bs, int64_t sector_num,
 
 static void nbd_close(BlockDriverState *bs)
 {
-    BDRVNBDState *s = bs->opaque;
-
-    qemu_opts_del(s->socket_opts);
     nbd_client_close(bs);
 }
 
-- 
2.4.3

Re: [Qemu-devel] [PATCH v1 1/2] nbd: convert to use the QAPI SocketAddress object

[Eric Blake]

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

On 09/16/2015 07:52 AM, Daniel P. Berrange wrote:
> The nbd block driver currently uses a QemuOpts object
> when setting up sockets. Switch it over to use the
> QAPI SocketAddress object instead.
> 
> Signed-off-by: Daniel P. Berrange <berrange@redhat.com>
> ---
>  block/nbd.c | 69 +++++++++++++++++++++++++++++++++----------------------------
>  1 file changed, 37 insertions(+), 32 deletions(-)

Reviewed-by: Eric Blake <eblake@redhat.com>

Wen - this patch will help in your efforts to expose NBD via
blockdev-add QMP.

> -    if (!qemu_opt_get(s->socket_opts, "port")) {
> -        qemu_opt_set_number(s->socket_opts, "port", NBD_DEFAULT_PORT,
> -                            &error_abort);
> -    }
> +    s->client.is_unix = saddr->kind == SOCKET_ADDRESS_KIND_UNIX;
>  

Minor conflict with my pending patch to rename the C representation of
qapi unions to spell things with 'type':
https://lists.gnu.org/archive/html/qemu-devel/2015-09/msg02598.html
but as that needs to be rebased once Markus' introspection lands, you
don't have to wait for it.

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


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

Re: [Qemu-devel] [PATCH v1 1/2] nbd: convert to use the QAPI SocketAddress object

[Paolo Bonzini]

On 16/09/2015 15:52, Daniel P. Berrange wrote:
> +    saddr = nbd_config(s, options, &export, &local_err);

You can use errp directly here.  Otherwise looks fine.

Paolo

> +    if (!saddr) {
>          error_propagate(errp, local_err);
>          return -EINVAL;
>      }

[Qemu-devel] [PATCH v1 2/2] qemu-nbd: convert to use the QAPI SocketAddress object

[Daniel P. Berrange]

The qemu-nbd program currently uses a QemuOpts objects
when setting up sockets. Switch it over to use the
QAPI SocketAddress objects instead.

Signed-off-by: Daniel P. Berrange <berrange@redhat.com>
---
 qemu-nbd.c | 102 +++++++++++++++++++++++--------------------------------------
 1 file changed, 38 insertions(+), 64 deletions(-)

diff --git a/qemu-nbd.c b/qemu-nbd.c
index 9518b75..6428c15 100644
--- a/qemu-nbd.c
+++ b/qemu-nbd.c
@@ -49,7 +49,7 @@
 static NBDExport *exp;
 static int verbose;
 static char *srcpath;
-static char *sockpath;
+static SocketAddress *saddr;
 static int persistent = 0;
 static enum { RUNNING, TERMINATE, TERMINATING, TERMINATED } state;
 static int shared = 1;
@@ -213,52 +213,6 @@ static void termsig_handler(int signum)
     qemu_notify_event();
 }
 
-static void combine_addr(char *buf, size_t len, const char* address,
-                         uint16_t port)
-{
-    /* If the address-part contains a colon, it's an IPv6 IP so needs [] */
-    if (strstr(address, ":")) {
-        snprintf(buf, len, "[%s]:%u", address, port);
-    } else {
-        snprintf(buf, len, "%s:%u", address, port);
-    }
-}
-
-static int tcp_socket_incoming(const char *address, uint16_t port)
-{
-    char address_and_port[128];
-    Error *local_err = NULL;
-
-    combine_addr(address_and_port, 128, address, port);
-    int fd = inet_listen(address_and_port, NULL, 0, SOCK_STREAM, 0, &local_err);
-
-    if (local_err != NULL) {
-        error_report_err(local_err);
-    }
-    return fd;
-}
-
-static int unix_socket_incoming(const char *path)
-{
-    Error *local_err = NULL;
-    int fd = unix_listen(path, NULL, 0, &local_err);
-
-    if (local_err != NULL) {
-        error_report_err(local_err);
-    }
-    return fd;
-}
-
-static int unix_socket_outgoing(const char *path)
-{
-    Error *local_err = NULL;
-    int fd = unix_connect(path, &local_err);
-
-    if (local_err != NULL) {
-        error_report_err(local_err);
-    }
-    return fd;
-}
 
 static void *show_parts(void *arg)
 {
@@ -287,8 +241,10 @@ static void *nbd_client_thread(void *arg)
     pthread_t show_parts_thread;
     Error *local_error = NULL;
 
-    sock = unix_socket_outgoing(sockpath);
+
+    sock = socket_connect(saddr, &local_error, NULL, NULL);
     if (sock < 0) {
+        error_report_err(local_error);
         goto out;
     }
 
@@ -399,6 +355,33 @@ static void nbd_update_server_fd_handler(int fd)
     }
 }
 
+
+static SocketAddress *nbd_build_socket_address(const char *sockpath,
+                                               const char *bindto,
+                                               const char *port)
+{
+    SocketAddress *saddr;
+
+    saddr = g_new0(SocketAddress, 1);
+    if (sockpath) {
+        saddr->kind = SOCKET_ADDRESS_KIND_UNIX;
+        saddr->q_unix = g_new0(UnixSocketAddress, 1);
+        saddr->q_unix->path = g_strdup(sockpath);
+    } else {
+        saddr->kind = SOCKET_ADDRESS_KIND_INET;
+        saddr->inet = g_new0(InetSocketAddress, 1);
+        saddr->inet->host = g_strdup(bindto);
+        if (port) {
+            saddr->inet->port = g_strdup(port);
+        } else  {
+            saddr->inet->port = g_strdup_printf("%d", NBD_DEFAULT_PORT);
+        }
+    }
+
+    return saddr;
+}
+
+
 int main(int argc, char **argv)
 {
     BlockBackend *blk;
@@ -407,8 +390,9 @@ int main(int argc, char **argv)
     uint32_t nbdflags = 0;
     bool disconnect = false;
     const char *bindto = "0.0.0.0";
+    const char *port = NULL;
+    char *sockpath = NULL;
     char *device = NULL;
-    int port = NBD_DEFAULT_PORT;
     off_t fd_size;
     QemuOpts *sn_opts = NULL;
     const char *sn_id_or_name = NULL;
@@ -441,7 +425,6 @@ int main(int argc, char **argv)
     };
     int ch;
     int opt_ind = 0;
-    int li;
     char *end;
     int flags = BDRV_O_RDWR;
     int partition = -1;
@@ -529,14 +512,7 @@ int main(int argc, char **argv)
             bindto = optarg;
             break;
         case 'p':
-            li = strtol(optarg, &end, 0);
-            if (*end) {
-                errx(EXIT_FAILURE, "Invalid port `%s'", optarg);
-            }
-            if (li < 1 || li > 65535) {
-                errx(EXIT_FAILURE, "Port out of range `%s'", optarg);
-            }
-            port = (uint16_t)li;
+            port = optarg;
             break;
         case 'o':
                 dev_offset = strtoll (optarg, &end, 0);
@@ -695,6 +671,8 @@ int main(int argc, char **argv)
         snprintf(sockpath, 128, SOCKET_PATH, basename(device));
     }
 
+    saddr = nbd_build_socket_address(sockpath, bindto, port);
+
     if (qemu_init_main_loop(&local_err)) {
         error_report_err(local_err);
         exit(EXIT_FAILURE);
@@ -752,13 +730,9 @@ int main(int argc, char **argv)
         errx(EXIT_FAILURE, "%s", error_get_pretty(local_err));
     }
 
-    if (sockpath) {
-        fd = unix_socket_incoming(sockpath);
-    } else {
-        fd = tcp_socket_incoming(bindto, port);
-    }
-
+    fd = socket_listen(saddr, &local_err);
     if (fd < 0) {
+        error_report_err(local_err);
         return 1;
     }
 
-- 
2.4.3

Re: [Qemu-devel] [PATCH v1 2/2] qemu-nbd: convert to use the QAPI SocketAddress object

[Eric Blake]

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

On 09/16/2015 07:52 AM, Daniel P. Berrange wrote:
> The qemu-nbd program currently uses a QemuOpts objects
> when setting up sockets. Switch it over to use the
> QAPI SocketAddress objects instead.
> 
> Signed-off-by: Daniel P. Berrange <berrange@redhat.com>
> ---
>  qemu-nbd.c | 102 +++++++++++++++++++++++--------------------------------------
>  1 file changed, 38 insertions(+), 64 deletions(-)
> 

> +static SocketAddress *nbd_build_socket_address(const char *sockpath,
> +                                               const char *bindto,
> +                                               const char *port)
> +{
> +    SocketAddress *saddr;
> +
> +    saddr = g_new0(SocketAddress, 1);
> +    if (sockpath) {
> +        saddr->kind = SOCKET_ADDRESS_KIND_UNIX;

More minor conflicts with my qapi cleanups.  Not a show-stopper.

> +        saddr->q_unix = g_new0(UnixSocketAddress, 1);
> +        saddr->q_unix->path = g_strdup(sockpath);
> +    } else {
> +        saddr->kind = SOCKET_ADDRESS_KIND_INET;
> +        saddr->inet = g_new0(InetSocketAddress, 1);
> +        saddr->inet->host = g_strdup(bindto);
> +        if (port) {
> +            saddr->inet->port = g_strdup(port);
> +        } else  {
> +            saddr->inet->port = g_strdup_printf("%d", NBD_DEFAULT_PORT);

The qapi type is gross for requiring port as a string. But we have plans
to clean that up, not a showstopper for this patch.

Reviewed-by: Eric Blake <eblake@redhat.com>

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


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

Re: [Qemu-devel] [PATCH v1 2/2] qemu-nbd: convert to use the QAPI SocketAddress object

[Daniel P. Berrange]

On Wed, Sep 16, 2015 at 08:08:35AM -0600, Eric Blake wrote:
> On 09/16/2015 07:52 AM, Daniel P. Berrange wrote:
> > The qemu-nbd program currently uses a QemuOpts objects
> > when setting up sockets. Switch it over to use the
> > QAPI SocketAddress objects instead.
> > 
> > Signed-off-by: Daniel P. Berrange <berrange@redhat.com>
> > ---
> >  qemu-nbd.c | 102 +++++++++++++++++++++++--------------------------------------
> >  1 file changed, 38 insertions(+), 64 deletions(-)
> > 
> 
> > +static SocketAddress *nbd_build_socket_address(const char *sockpath,
> > +                                               const char *bindto,
> > +                                               const char *port)
> > +{
> > +    SocketAddress *saddr;
> > +
> > +    saddr = g_new0(SocketAddress, 1);
> > +    if (sockpath) {
> > +        saddr->kind = SOCKET_ADDRESS_KIND_UNIX;
> 
> More minor conflicts with my qapi cleanups.  Not a show-stopper.
> 
> > +        saddr->q_unix = g_new0(UnixSocketAddress, 1);
> > +        saddr->q_unix->path = g_strdup(sockpath);
> > +    } else {
> > +        saddr->kind = SOCKET_ADDRESS_KIND_INET;
> > +        saddr->inet = g_new0(InetSocketAddress, 1);
> > +        saddr->inet->host = g_strdup(bindto);
> > +        if (port) {
> > +            saddr->inet->port = g_strdup(port);
> > +        } else  {
> > +            saddr->inet->port = g_strdup_printf("%d", NBD_DEFAULT_PORT);
> 
> The qapi type is gross for requiring port as a string. But we have plans
> to clean that up, not a showstopper for this patch.

On the contrary - QAPI is correct in requiring this, as it lets you
provide a service name (as defined in /etc/services) instead of a
numeric port, and getaddrinfo() will look that up and convert to
numeric format.

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

Re: [Qemu-devel] [PATCH v1 2/2] qemu-nbd: convert to use the QAPI SocketAddress object

[Paolo Bonzini]

On 16/09/2015 16:13, Daniel P. Berrange wrote:
> > The qapi type is gross for requiring port as a string. But we have plans
> > to clean that up, not a showstopper for this patch.
>
> On the contrary - QAPI is correct in requiring this, as it lets you
> provide a service name (as defined in /etc/services) instead of a
> numeric port, and getaddrinfo() will look that up and convert to
> numeric format.

Was going to say the same. :)  I'll fix up patch 1 and apply both.

Paolo

Re: [Qemu-devel] [PATCH v1 2/2] qemu-nbd: convert to use the QAPI SocketAddress object

[Eric Blake]

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

On 09/16/2015 08:13 AM, Daniel P. Berrange wrote:
> On Wed, Sep 16, 2015 at 08:08:35AM -0600, Eric Blake wrote:
>> On 09/16/2015 07:52 AM, Daniel P. Berrange wrote:
>>> The qemu-nbd program currently uses a QemuOpts objects
>>> when setting up sockets. Switch it over to use the
>>> QAPI SocketAddress objects instead.
>>>

>>> +        if (port) {
>>> +            saddr->inet->port = g_strdup(port);
>>> +        } else  {
>>> +            saddr->inet->port = g_strdup_printf("%d", NBD_DEFAULT_PORT);
>>
>> The qapi type is gross for requiring port as a string. But we have plans
>> to clean that up, not a showstopper for this patch.
> 
> On the contrary - QAPI is correct in requiring this, as it lets you
> provide a service name (as defined in /etc/services) instead of a
> numeric port, and getaddrinfo() will look that up and convert to
> numeric format.

What it SHOULD be doing it taking an 'alternate' that allows both a
string (for getaddrinfo lookup) and an int (for direct port usage),
rather than overloading a string for both uses.  But as I said, that
should be a future cleanup, affecting more uses of the qapi type than
just this patch.

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


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