[Qemu-devel] [PATCH 00/15] net: Improve error reporting

[Qemu-devel] [PATCH 00/15] net: Improve error reporting

[Markus Armbruster]

Markus Armbruster (15):
  net: Improve error message for -net hubport a bit
  net: Permit incremental conversion of init functions to Error
  net: Improve -net nic error reporting
  net/dump: Improve -net/host_net_add dump error reporting
  tap: net_tap_fd_init() can't fail, drop dead error handling
  tap: Improve -netdev/netdev_add/-net/... bridge error reporting
  tap: Convert tap_set_sndbuf() to Error
  tap: Convert net_init_tap_one() to Error
  tap: Convert launch_script() to Error
  tap: Permit incremental conversion of tap_open() to Error
  tap-linux: Convert tap_open() to Error
  tap-bsd: Convert tap_open() to Error
  tap-solaris: Convert tap_open() to Error
  tap: Finish conversion of tap_open() to Error
  tap: Improve -netdev/netdev_add/-net/... tap error reporting

 net/clients.h     |  20 +++---
 net/dump.c        |  13 ++--
 net/hub.c         |   7 +-
 net/l2tpv3.c      |   5 +-
 net/net.c         |  30 ++++++---
 net/netmap.c      |   3 +-
 net/slirp.c       |   3 +-
 net/socket.c      |   3 +-
 net/tap-aix.c     |   7 +-
 net/tap-bsd.c     |  38 +++++------
 net/tap-haiku.c   |   7 +-
 net/tap-linux.c   |  24 +++----
 net/tap-solaris.c |  63 ++++++++---------
 net/tap-win32.c   |   3 +-
 net/tap.c         | 198 ++++++++++++++++++++++++++++++------------------------
 net/tap_int.h     |   4 +-
 net/vde.c         |   3 +-
 net/vhost-user.c  |   3 +-
 18 files changed, 231 insertions(+), 203 deletions(-)

-- 
1.9.3

[Qemu-devel] [PATCH 01/15] net: Improve error message for -net hubport a bit

[Markus Armbruster]

Type "hubport" is valid only with -netdev.  Unfortunately, that's
detected late and the error message doesn't explain why:

    $ qemu-system-i386 -net hubport,id=foo,hubid=0
    qemu-system-i386: -net hubport,id=foo,hubid=0: Device 'hubport' could not be initialized

Improve the error message to "Parameter 'type' expects a net type".

Not fixed: -net hubport without the parameters required by -netdev
hubport still asks for those parameters:

    $ qemu-system-i386 -net hubport
    qemu-system-i386: -net hubport: Parameter 'hubid' is missing

Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
 net/hub.c | 5 +----
 net/net.c | 5 +++++
 2 files changed, 6 insertions(+), 4 deletions(-)

diff --git a/net/hub.c b/net/hub.c
index 2b60ab9..261f8cc 100644
--- a/net/hub.c
+++ b/net/hub.c
@@ -286,12 +286,9 @@ int net_init_hubport(const NetClientOptions *opts, const char *name,
     const NetdevHubPortOptions *hubport;
 
     assert(opts->kind == NET_CLIENT_OPTIONS_KIND_HUBPORT);
+    assert(!peer);
     hubport = opts->hubport;
 
-    if (peer) {
-        return -EINVAL;
-    }
-
     net_hub_add_port(hubport->hubid, name);
     return 0;
 }
diff --git a/net/net.c b/net/net.c
index 0be084d..06affd6 100644
--- a/net/net.c
+++ b/net/net.c
@@ -875,6 +875,11 @@ static int net_client_init1(const void *object, int is_netdev, Error **errp)
     } else {
         u.net = object;
         opts = u.net->opts;
+        if (opts->kind == NET_CLIENT_OPTIONS_KIND_HUBPORT) {
+            error_set(errp, QERR_INVALID_PARAMETER_VALUE, "type",
+                      "a net type");
+            return -1;
+        }
         /* missing optional values have been initialized to "all bits zero" */
         name = u.net->has_id ? u.net->id : u.net->name;
     }
-- 
1.9.3

[Qemu-devel] [PATCH 02/15] net: Permit incremental conversion of init functions to Error

[Markus Armbruster]

Error reporting for netdev_add is broken: the net_client_init_fun[]
report the actual errors with (at best) error_report(), and their
caller net_client_init1() makes up a generic error on top.

For command line and HMP, this produces an mildly ugly error cascande.

In QMP, the actual errors go to stderr, and the generic error becomes
the command's error reply.

To fix this, we need to convert the net_client_init_fun[] to Error.

To permit fixing them one by one, add an Error ** parameter to the
net_client_init_fun[].  If the call fails without returning an Error,
make up the same generic Error as before.  But if it returns one, use
that instead.  Since none of them does so far, no functional change.

Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
 net/clients.h    | 20 ++++++++++----------
 net/dump.c       |  3 ++-
 net/hub.c        |  2 +-
 net/l2tpv3.c     |  5 ++---
 net/net.c        | 15 +++++++++------
 net/netmap.c     |  3 ++-
 net/slirp.c      |  3 ++-
 net/socket.c     |  3 ++-
 net/tap-win32.c  |  3 ++-
 net/tap.c        |  6 ++++--
 net/vde.c        |  3 ++-
 net/vhost-user.c |  3 ++-
 12 files changed, 40 insertions(+), 29 deletions(-)

diff --git a/net/clients.h b/net/clients.h
index 2e8feda..d47530e 100644
--- a/net/clients.h
+++ b/net/clients.h
@@ -28,38 +28,38 @@
 #include "qapi-types.h"
 
 int net_init_dump(const NetClientOptions *opts, const char *name,
-                  NetClientState *peer);
+                  NetClientState *peer, Error **errp);
 
 #ifdef CONFIG_SLIRP
 int net_init_slirp(const NetClientOptions *opts, const char *name,
-                   NetClientState *peer);
+                   NetClientState *peer, Error **errp);
 #endif
 
 int net_init_hubport(const NetClientOptions *opts, const char *name,
-                     NetClientState *peer);
+                     NetClientState *peer, Error **errp);
 
 int net_init_socket(const NetClientOptions *opts, const char *name,
-                    NetClientState *peer);
+                    NetClientState *peer, Error **errp);
 
 int net_init_tap(const NetClientOptions *opts, const char *name,
-                 NetClientState *peer);
+                 NetClientState *peer, Error **errp);
 
 int net_init_bridge(const NetClientOptions *opts, const char *name,
-                    NetClientState *peer);
+                    NetClientState *peer, Error **errp);
 
 int net_init_l2tpv3(const NetClientOptions *opts, const char *name,
-                    NetClientState *peer);
+                    NetClientState *peer, Error **errp);
 #ifdef CONFIG_VDE
 int net_init_vde(const NetClientOptions *opts, const char *name,
-                 NetClientState *peer);
+                 NetClientState *peer, Error **errp);
 #endif
 
 #ifdef CONFIG_NETMAP
 int net_init_netmap(const NetClientOptions *opts, const char *name,
-                    NetClientState *peer);
+                    NetClientState *peer, Error **errp);
 #endif
 
 int net_init_vhost_user(const NetClientOptions *opts, const char *name,
-                        NetClientState *peer);
+                        NetClientState *peer, Error **errp);
 
 #endif /* QEMU_NET_CLIENTS_H */
diff --git a/net/dump.c b/net/dump.c
index 9d3a09e..214e88a 100644
--- a/net/dump.c
+++ b/net/dump.c
@@ -146,8 +146,9 @@ static int net_dump_init(NetClientState *peer, const char *device,
 }
 
 int net_init_dump(const NetClientOptions *opts, const char *name,
-                  NetClientState *peer)
+                  NetClientState *peer, Error **errp)
 {
+    /* FIXME error_setg(errp, ...) on failure */
     int len;
     const char *file;
     char def_file[128];
diff --git a/net/hub.c b/net/hub.c
index 261f8cc..3047f12 100644
--- a/net/hub.c
+++ b/net/hub.c
@@ -281,7 +281,7 @@ int net_hub_id_for_client(NetClientState *nc, int *id)
 }
 
 int net_init_hubport(const NetClientOptions *opts, const char *name,
-                     NetClientState *peer)
+                     NetClientState *peer, Error **errp)
 {
     const NetdevHubPortOptions *hubport;
 
diff --git a/net/l2tpv3.c b/net/l2tpv3.c
index 8c598b0..ed395dc 100644
--- a/net/l2tpv3.c
+++ b/net/l2tpv3.c
@@ -536,10 +536,9 @@ static NetClientInfo net_l2tpv3_info = {
 
 int net_init_l2tpv3(const NetClientOptions *opts,
                     const char *name,
-                    NetClientState *peer)
+                    NetClientState *peer, Error **errp)
 {
-
-
+    /* FIXME error_setg(errp, ...) on failure */
     const NetdevL2TPv3Options *l2tpv3;
     NetL2TPV3State *s;
     NetClientState *nc;
diff --git a/net/net.c b/net/net.c
index 06affd6..5808c58 100644
--- a/net/net.c
+++ b/net/net.c
@@ -733,8 +733,9 @@ int qemu_find_nic_model(NICInfo *nd, const char * const *models,
 }
 
 static int net_init_nic(const NetClientOptions *opts, const char *name,
-                        NetClientState *peer)
+                        NetClientState *peer, Error **errp)
 {
+    /* FIXME error_setg(errp, ...) on failure */
     int idx;
     NICInfo *nd;
     const NetLegacyNicOptions *nic;
@@ -802,7 +803,7 @@ static int net_init_nic(const NetClientOptions *opts, const char *name,
 static int (* const net_client_init_fun[NET_CLIENT_OPTIONS_KIND_MAX])(
     const NetClientOptions *opts,
     const char *name,
-    NetClientState *peer) = {
+    NetClientState *peer, Error **errp) = {
         [NET_CLIENT_OPTIONS_KIND_NIC]       = net_init_nic,
 #ifdef CONFIG_SLIRP
         [NET_CLIENT_OPTIONS_KIND_USER]      = net_init_slirp,
@@ -895,10 +896,12 @@ static int net_client_init1(const void *object, int is_netdev, Error **errp)
             peer = net_hub_add_port(u.net->has_vlan ? u.net->vlan : 0, NULL);
         }
 
-        if (net_client_init_fun[opts->kind](opts, name, peer) < 0) {
-            /* TODO push error reporting into init() methods */
-            error_set(errp, QERR_DEVICE_INIT_FAILED,
-                      NetClientOptionsKind_lookup[opts->kind]);
+        if (net_client_init_fun[opts->kind](opts, name, peer, errp) < 0) {
+            /* FIXME drop when all init functions store an Error */
+            if (errp && !*errp) {
+                error_set(errp, QERR_DEVICE_INIT_FAILED,
+                          NetClientOptionsKind_lookup[opts->kind]);
+            }
             return -1;
         }
     }
diff --git a/net/netmap.c b/net/netmap.c
index 0c1772b..69300eb 100644
--- a/net/netmap.c
+++ b/net/netmap.c
@@ -446,8 +446,9 @@ static NetClientInfo net_netmap_info = {
  * ... -net netmap,ifname="..."
  */
 int net_init_netmap(const NetClientOptions *opts,
-        const char *name, NetClientState *peer)
+                    const char *name, NetClientState *peer, Error **errp)
 {
+    /* FIXME error_setg(errp, ...) on failure */
     const NetdevNetmapOptions *netmap_opts = opts->netmap;
     NetClientState *nc;
     NetmapPriv me;
diff --git a/net/slirp.c b/net/slirp.c
index 9bbed74..0e15cf6 100644
--- a/net/slirp.c
+++ b/net/slirp.c
@@ -737,8 +737,9 @@ static const char **slirp_dnssearch(const StringList *dnsname)
 }
 
 int net_init_slirp(const NetClientOptions *opts, const char *name,
-                   NetClientState *peer)
+                   NetClientState *peer, Error **errp)
 {
+    /* FIXME error_setg(errp, ...) on failure */
     struct slirp_config_str *config;
     char *vnet;
     int ret;
diff --git a/net/socket.c b/net/socket.c
index c30e03f..5a19aa1 100644
--- a/net/socket.c
+++ b/net/socket.c
@@ -693,8 +693,9 @@ static int net_socket_udp_init(NetClientState *peer,
 }
 
 int net_init_socket(const NetClientOptions *opts, const char *name,
-                    NetClientState *peer)
+                    NetClientState *peer, Error **errp)
 {
+    /* FIXME error_setg(errp, ...) on failure */
     Error *err = NULL;
     const NetdevSocketOptions *sock;
 
diff --git a/net/tap-win32.c b/net/tap-win32.c
index 8aee611..f6fc961 100644
--- a/net/tap-win32.c
+++ b/net/tap-win32.c
@@ -752,8 +752,9 @@ static int tap_win32_init(NetClientState *peer, const char *model,
 }
 
 int net_init_tap(const NetClientOptions *opts, const char *name,
-                 NetClientState *peer)
+                 NetClientState *peer, Error **errp)
 {
+    /* FIXME error_setg(errp, ...) on failure */
     const NetdevTapOptions *tap;
 
     assert(opts->kind == NET_CLIENT_OPTIONS_KIND_TAP);
diff --git a/net/tap.c b/net/tap.c
index 968df46..8f06cb7 100644
--- a/net/tap.c
+++ b/net/tap.c
@@ -531,8 +531,9 @@ static int net_bridge_run_helper(const char *helper, const char *bridge)
 }
 
 int net_init_bridge(const NetClientOptions *opts, const char *name,
-                    NetClientState *peer)
+                    NetClientState *peer, Error **errp)
 {
+    /* FIXME error_setg(errp, ...) on failure */
     const NetdevBridgeOptions *bridge;
     const char *helper, *br;
 
@@ -699,8 +700,9 @@ static int get_fds(char *str, char *fds[], int max)
 }
 
 int net_init_tap(const NetClientOptions *opts, const char *name,
-                 NetClientState *peer)
+                 NetClientState *peer, Error **errp)
 {
+    /* FIXME error_setg(errp, ...) on failure */
     const NetdevTapOptions *tap;
     int fd, vnet_hdr = 0, i = 0, queues;
     /* for the no-fd, no-helper case */
diff --git a/net/vde.c b/net/vde.c
index 2a619fb..dacaa64 100644
--- a/net/vde.c
+++ b/net/vde.c
@@ -110,8 +110,9 @@ static int net_vde_init(NetClientState *peer, const char *model,
 }
 
 int net_init_vde(const NetClientOptions *opts, const char *name,
-                 NetClientState *peer)
+                 NetClientState *peer, Error **errp)
 {
+    /* FIXME error_setg(errp, ...) on failure */
     const NetdevVdeOptions *vde;
 
     assert(opts->kind == NET_CLIENT_OPTIONS_KIND_VDE);
diff --git a/net/vhost-user.c b/net/vhost-user.c
index 1d86a2b..11899c5 100644
--- a/net/vhost-user.c
+++ b/net/vhost-user.c
@@ -223,8 +223,9 @@ static int net_vhost_check_net(QemuOpts *opts, void *opaque)
 }
 
 int net_init_vhost_user(const NetClientOptions *opts, const char *name,
-                        NetClientState *peer)
+                        NetClientState *peer, Error **errp)
 {
+    /* FIXME error_setg(errp, ...) on failure */
     const NetdevVhostUserOptions *vhost_user_opts;
     CharDriverState *chr;
 
-- 
1.9.3

[Qemu-devel] [PATCH 03/15] net: Improve -net nic error reporting

[Markus Armbruster]

When -net nic fails, it first reports a specific error, then a generic
one, like this:

    $ qemu-system-x86_64 -net nic,netdev=nonexistant
    qemu-system-x86_64: -net nic,netdev=nonexistant: netdev 'nonexistant' not found
    qemu-system-x86_64: -net nic,netdev=nonexistant: Device 'nic' could not be initialized

Convert net_init_nic() to Error to get rid of the unwanted second
error message.

Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
 net/net.c | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

diff --git a/net/net.c b/net/net.c
index 5808c58..70ec33a 100644
--- a/net/net.c
+++ b/net/net.c
@@ -735,7 +735,6 @@ int qemu_find_nic_model(NICInfo *nd, const char * const *models,
 static int net_init_nic(const NetClientOptions *opts, const char *name,
                         NetClientState *peer, Error **errp)
 {
-    /* FIXME error_setg(errp, ...) on failure */
     int idx;
     NICInfo *nd;
     const NetLegacyNicOptions *nic;
@@ -745,7 +744,7 @@ static int net_init_nic(const NetClientOptions *opts, const char *name,
 
     idx = nic_get_free_idx();
     if (idx == -1 || nb_nics >= MAX_NICS) {
-        error_report("Too Many NICs");
+        error_setg(errp, "Too Many NICs");
         return -1;
     }
 
@@ -756,7 +755,7 @@ static int net_init_nic(const NetClientOptions *opts, const char *name,
     if (nic->has_netdev) {
         nd->netdev = qemu_find_netdev(nic->netdev);
         if (!nd->netdev) {
-            error_report("netdev '%s' not found", nic->netdev);
+            error_setg(errp, "netdev '%s' not found", nic->netdev);
             return -1;
         }
     } else {
@@ -773,19 +772,20 @@ static int net_init_nic(const NetClientOptions *opts, const char *name,
 
     if (nic->has_macaddr &&
         net_parse_macaddr(nd->macaddr.a, nic->macaddr) < 0) {
-        error_report("invalid syntax for ethernet address");
+        error_setg(errp, "invalid syntax for ethernet address");
         return -1;
     }
     if (nic->has_macaddr &&
         is_multicast_ether_addr(nd->macaddr.a)) {
-        error_report("NIC cannot have multicast MAC address (odd 1st byte)");
+        error_setg(errp,
+                   "NIC cannot have multicast MAC address (odd 1st byte)");
         return -1;
     }
     qemu_macaddr_default_if_unset(&nd->macaddr);
 
     if (nic->has_vectors) {
         if (nic->vectors > 0x7ffffff) {
-            error_report("invalid # of vectors: %"PRIu32, nic->vectors);
+            error_setg(errp, "invalid # of vectors: %"PRIu32, nic->vectors);
             return -1;
         }
         nd->nvectors = nic->vectors;
-- 
1.9.3

[Qemu-devel] [PATCH 04/15] net/dump: Improve -net/host_net_add dump error reporting

[Markus Armbruster]

When -net dump fails, it first reports a specific error, then a
generic one, like this:

    $ qemu-system-x86_64 -net dump,id=foo,file=/eperm
    qemu-system-x86_64: -net dump,id=foo,file=/eperm: -net dump: can't open /eperm
    qemu-system-x86_64: -net dump,id=foo,file=/eperm: Device 'dump' could not be initialized

Convert net_init_tap() to Error.  This suppresses the unwanted second
message.

Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
 net/dump.c | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

diff --git a/net/dump.c b/net/dump.c
index 214e88a..02c8064 100644
--- a/net/dump.c
+++ b/net/dump.c
@@ -101,7 +101,8 @@ static NetClientInfo net_dump_info = {
 };
 
 static int net_dump_init(NetClientState *peer, const char *device,
-                         const char *name, const char *filename, int len)
+                         const char *name, const char *filename, int len,
+                         Error **errp)
 {
     struct pcap_file_hdr hdr;
     NetClientState *nc;
@@ -111,7 +112,7 @@ static int net_dump_init(NetClientState *peer, const char *device,
 
     fd = open(filename, O_CREAT | O_TRUNC | O_WRONLY | O_BINARY, 0644);
     if (fd < 0) {
-        error_report("-net dump: can't open %s", filename);
+        error_setg_errno(errp, errno, "-net dump: can't open %s", filename);
         return -1;
     }
 
@@ -124,7 +125,7 @@ static int net_dump_init(NetClientState *peer, const char *device,
     hdr.linktype = 1;
 
     if (write(fd, &hdr, sizeof(hdr)) < sizeof(hdr)) {
-        error_report("-net dump write error: %s", strerror(errno));
+        error_setg_errno(errp, errno, "-net dump write error");
         close(fd);
         return -1;
     }
@@ -148,7 +149,6 @@ static int net_dump_init(NetClientState *peer, const char *device,
 int net_init_dump(const NetClientOptions *opts, const char *name,
                   NetClientState *peer, Error **errp)
 {
-    /* FIXME error_setg(errp, ...) on failure */
     int len;
     const char *file;
     char def_file[128];
@@ -174,7 +174,7 @@ int net_init_dump(const NetClientOptions *opts, const char *name,
 
     if (dump->has_len) {
         if (dump->len > INT_MAX) {
-            error_report("invalid length: %"PRIu64, dump->len);
+            error_setg(errp, "invalid length: %"PRIu64, dump->len);
             return -1;
         }
         len = dump->len;
@@ -182,5 +182,5 @@ int net_init_dump(const NetClientOptions *opts, const char *name,
         len = 65536;
     }
 
-    return net_dump_init(peer, "dump", name, file, len);
+    return net_dump_init(peer, "dump", name, file, len, errp);
 }
-- 
1.9.3

[Qemu-devel] [PATCH 05/15] tap: net_tap_fd_init() can't fail, drop dead error handling

[Markus Armbruster]

Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
 net/tap.c | 14 +-------------
 1 file changed, 1 insertion(+), 13 deletions(-)

diff --git a/net/tap.c b/net/tap.c
index 8f06cb7..adb1022 100644
--- a/net/tap.c
+++ b/net/tap.c
@@ -536,7 +536,6 @@ int net_init_bridge(const NetClientOptions *opts, const char *name,
     /* FIXME error_setg(errp, ...) on failure */
     const NetdevBridgeOptions *bridge;
     const char *helper, *br;
-
     TAPState *s;
     int fd, vnet_hdr;
 
@@ -552,14 +551,8 @@ int net_init_bridge(const NetClientOptions *opts, const char *name,
     }
 
     fcntl(fd, F_SETFL, O_NONBLOCK);
-
     vnet_hdr = tap_probe_vnet_hdr(fd);
-
     s = net_tap_fd_init(peer, "bridge", name, fd, vnet_hdr);
-    if (!s) {
-        close(fd);
-        return -1;
-    }
 
     snprintf(s->nc.info_str, sizeof(s->nc.info_str), "helper=%s,br=%s", helper,
              br);
@@ -607,14 +600,9 @@ static int net_init_tap_one(const NetdevTapOptions *tap, NetClientState *peer,
                             int vnet_hdr, int fd)
 {
     Error *err = NULL;
-    TAPState *s;
+    TAPState *s = net_tap_fd_init(peer, model, name, fd, vnet_hdr);
     int vhostfd;
 
-    s = net_tap_fd_init(peer, model, name, fd, vnet_hdr);
-    if (!s) {
-        return -1;
-    }
-
     if (tap_set_sndbuf(s->fd, tap) < 0) {
         return -1;
     }
-- 
1.9.3

[Qemu-devel] [PATCH 06/15] tap: Improve -netdev/netdev_add/-net/... bridge error reporting

[Markus Armbruster]

When -netdev bridge fails, it first reports a specific error, then a
generic one, like this:

    $ qemu-system-x86_64 -netdev bridge,id=foo
    failed to launch bridge helper
    qemu-system-x86_64: -netdev bridge,id=foo: Device 'bridge' could not be initialized

The first message goes to stderr.  Wrong for HMP, because errors need
to go to the monitor there.

The second message goes to stderr for -netdev, to the monitor for HMP
netdev_add, and becomes the error reply for QMP netdev_add.

Convert net_bridge_run_helper() to Error, and propagate its errors
through net_init_bridge().  This ensures the error gets reported where
the user is, and suppresses the unwanted second message.

While there, improve the error messages a bit.

The above example becomes:

    $ qemu-system-x86_64 -netdev bridge,id=foo
    qemu-system-x86_64: -netdev bridge,id=foo: bridge helper failed

net_init_tap() also uses net_bridge_run_helper().  Propagate its
errors there as well.  Improves reporting these errors with -netdev
tap & friends.

Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
 net/tap.c | 30 +++++++++++++++++++-----------
 1 file changed, 19 insertions(+), 11 deletions(-)

diff --git a/net/tap.c b/net/tap.c
index adb1022..23c81fa 100644
--- a/net/tap.c
+++ b/net/tap.c
@@ -437,7 +437,8 @@ static int recv_fd(int c)
     return len;
 }
 
-static int net_bridge_run_helper(const char *helper, const char *bridge)
+static int net_bridge_run_helper(const char *helper, const char *bridge,
+                                 Error **errp)
 {
     sigset_t oldmask, mask;
     int pid, status;
@@ -450,11 +451,16 @@ static int net_bridge_run_helper(const char *helper, const char *bridge)
     sigprocmask(SIG_BLOCK, &mask, &oldmask);
 
     if (socketpair(PF_UNIX, SOCK_STREAM, 0, sv) == -1) {
+        error_setg_errno(errp, errno, "socketpair() failed");
         return -1;
     }
 
     /* try to launch bridge helper */
     pid = fork();
+    if (pid < 0) {
+        error_setg_errno(errp, errno, "Can't fork bridge helper");
+        return -1;
+    }
     if (pid == 0) {
         int open_max = sysconf(_SC_OPEN_MAX), i;
         char fd_buf[6+10];
@@ -502,14 +508,16 @@ static int net_bridge_run_helper(const char *helper, const char *bridge)
         }
         _exit(1);
 
-    } else if (pid > 0) {
+    } else {
         int fd;
+        int saved_errno;
 
         close(sv[1]);
 
         do {
             fd = recv_fd(sv[0]);
         } while (fd == -1 && errno == EINTR);
+        saved_errno = errno;
 
         close(sv[0]);
 
@@ -518,22 +526,21 @@ static int net_bridge_run_helper(const char *helper, const char *bridge)
         }
         sigprocmask(SIG_SETMASK, &oldmask, NULL);
         if (fd < 0) {
-            fprintf(stderr, "failed to recv file descriptor\n");
+            error_setg_errno(errp, saved_errno,
+                             "failed to recv file descriptor");
             return -1;
         }
-
-        if (WIFEXITED(status) && WEXITSTATUS(status) == 0) {
-            return fd;
+        if (!WIFEXITED(status) || WEXITSTATUS(status) != 0) {
+            error_setg(errp, "bridge helper failed");
+            return -1;
         }
+        return fd;
     }
-    fprintf(stderr, "failed to launch bridge helper\n");
-    return -1;
 }
 
 int net_init_bridge(const NetClientOptions *opts, const char *name,
                     NetClientState *peer, Error **errp)
 {
-    /* FIXME error_setg(errp, ...) on failure */
     const NetdevBridgeOptions *bridge;
     const char *helper, *br;
     TAPState *s;
@@ -545,7 +552,7 @@ int net_init_bridge(const NetClientOptions *opts, const char *name,
     helper = bridge->has_helper ? bridge->helper : DEFAULT_BRIDGE_HELPER;
     br     = bridge->has_br     ? bridge->br     : DEFAULT_BRIDGE_INTERFACE;
 
-    fd = net_bridge_run_helper(helper, br);
+    fd = net_bridge_run_helper(helper, br, errp);
     if (fd == -1) {
         return -1;
     }
@@ -792,7 +799,8 @@ int net_init_tap(const NetClientOptions *opts, const char *name,
             return -1;
         }
 
-        fd = net_bridge_run_helper(tap->helper, DEFAULT_BRIDGE_INTERFACE);
+        fd = net_bridge_run_helper(tap->helper, DEFAULT_BRIDGE_INTERFACE,
+                                   errp);
         if (fd == -1) {
             return -1;
         }
-- 
1.9.3

[Qemu-devel] [PATCH 07/15] tap: Convert tap_set_sndbuf() to Error

[Markus Armbruster]

Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
 net/tap-aix.c     | 3 +--
 net/tap-bsd.c     | 3 +--
 net/tap-haiku.c   | 3 +--
 net/tap-linux.c   | 6 ++----
 net/tap-solaris.c | 3 +--
 net/tap.c         | 4 +++-
 net/tap_int.h     | 2 +-
 7 files changed, 10 insertions(+), 14 deletions(-)

diff --git a/net/tap-aix.c b/net/tap-aix.c
index 804d164..0a3d461 100644
--- a/net/tap-aix.c
+++ b/net/tap-aix.c
@@ -32,9 +32,8 @@ int tap_open(char *ifname, int ifname_size, int *vnet_hdr,
     return -1;
 }
 
-int tap_set_sndbuf(int fd, const NetdevTapOptions *tap)
+void tap_set_sndbuf(int fd, const NetdevTapOptions *tap, Error **errp)
 {
-    return 0;
 }
 
 int tap_probe_vnet_hdr(int fd)
diff --git a/net/tap-bsd.c b/net/tap-bsd.c
index bf91bd0..53cdd9f 100644
--- a/net/tap-bsd.c
+++ b/net/tap-bsd.c
@@ -176,9 +176,8 @@ error:
 }
 #endif /* __FreeBSD__ */
 
-int tap_set_sndbuf(int fd, const NetdevTapOptions *tap)
+void tap_set_sndbuf(int fd, const NetdevTapOptions *tap, Error **errp)
 {
-    return 0;
 }
 
 int tap_probe_vnet_hdr(int fd)
diff --git a/net/tap-haiku.c b/net/tap-haiku.c
index e5ce436..0905b28 100644
--- a/net/tap-haiku.c
+++ b/net/tap-haiku.c
@@ -32,9 +32,8 @@ int tap_open(char *ifname, int ifname_size, int *vnet_hdr,
     return -1;
 }
 
-int tap_set_sndbuf(int fd, const NetdevTapOptions *tap)
+void tap_set_sndbuf(int fd, const NetdevTapOptions *tap, Error **errp)
 {
-    return 0;
 }
 
 int tap_probe_vnet_hdr(int fd)
diff --git a/net/tap-linux.c b/net/tap-linux.c
index 812bf2d..6fa2744 100644
--- a/net/tap-linux.c
+++ b/net/tap-linux.c
@@ -126,7 +126,7 @@ int tap_open(char *ifname, int ifname_size, int *vnet_hdr,
  */
 #define TAP_DEFAULT_SNDBUF 0
 
-int tap_set_sndbuf(int fd, const NetdevTapOptions *tap)
+void tap_set_sndbuf(int fd, const NetdevTapOptions *tap, Error **errp)
 {
     int sndbuf;
 
@@ -139,10 +139,8 @@ int tap_set_sndbuf(int fd, const NetdevTapOptions *tap)
     }
 
     if (ioctl(fd, TUNSETSNDBUF, &sndbuf) == -1 && tap->has_sndbuf) {
-        error_report("TUNSETSNDBUF ioctl failed: %s", strerror(errno));
-        return -1;
+        error_setg_errno(errp, errno, "TUNSETSNDBUF ioctl failed");
     }
-    return 0;
 }
 
 int tap_probe_vnet_hdr(int fd)
diff --git a/net/tap-solaris.c b/net/tap-solaris.c
index 9c7278f..7839323 100644
--- a/net/tap-solaris.c
+++ b/net/tap-solaris.c
@@ -198,9 +198,8 @@ int tap_open(char *ifname, int ifname_size, int *vnet_hdr,
     return fd;
 }
 
-int tap_set_sndbuf(int fd, const NetdevTapOptions *tap)
+void tap_set_sndbuf(int fd, const NetdevTapOptions *tap, Error **errp)
 {
-    return 0;
 }
 
 int tap_probe_vnet_hdr(int fd)
diff --git a/net/tap.c b/net/tap.c
index 23c81fa..d54222d 100644
--- a/net/tap.c
+++ b/net/tap.c
@@ -610,7 +610,9 @@ static int net_init_tap_one(const NetdevTapOptions *tap, NetClientState *peer,
     TAPState *s = net_tap_fd_init(peer, model, name, fd, vnet_hdr);
     int vhostfd;
 
-    if (tap_set_sndbuf(s->fd, tap) < 0) {
+    tap_set_sndbuf(s->fd, tap, &err);
+    if (err) {
+        error_report_err(err);
         return -1;
     }
 
diff --git a/net/tap_int.h b/net/tap_int.h
index 79afdf2..6df271f 100644
--- a/net/tap_int.h
+++ b/net/tap_int.h
@@ -34,7 +34,7 @@ int tap_open(char *ifname, int ifname_size, int *vnet_hdr,
 
 ssize_t tap_read_packet(int tapfd, uint8_t *buf, int maxlen);
 
-int tap_set_sndbuf(int fd, const NetdevTapOptions *tap);
+void tap_set_sndbuf(int fd, const NetdevTapOptions *tap, Error **errp);
 int tap_probe_vnet_hdr(int fd);
 int tap_probe_vnet_hdr_len(int fd, int len);
 int tap_probe_has_ufo(int fd);
-- 
1.9.3

[Qemu-devel] [PATCH 08/15] tap: Convert net_init_tap_one() to Error

[Markus Armbruster]

Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
 net/tap.c | 70 ++++++++++++++++++++++++++++++++++-----------------------------
 1 file changed, 38 insertions(+), 32 deletions(-)

diff --git a/net/tap.c b/net/tap.c
index d54222d..d1f5644 100644
--- a/net/tap.c
+++ b/net/tap.c
@@ -600,11 +600,11 @@ static int net_tap_init(const NetdevTapOptions *tap, int *vnet_hdr,
 
 #define MAX_TAP_QUEUES 1024
 
-static int net_init_tap_one(const NetdevTapOptions *tap, NetClientState *peer,
-                            const char *model, const char *name,
-                            const char *ifname, const char *script,
-                            const char *downscript, const char *vhostfdname,
-                            int vnet_hdr, int fd)
+static void net_init_tap_one(const NetdevTapOptions *tap, NetClientState *peer,
+                             const char *model, const char *name,
+                             const char *ifname, const char *script,
+                             const char *downscript, const char *vhostfdname,
+                             int vnet_hdr, int fd, Error **errp)
 {
     Error *err = NULL;
     TAPState *s = net_tap_fd_init(peer, model, name, fd, vnet_hdr);
@@ -612,8 +612,8 @@ static int net_init_tap_one(const NetdevTapOptions *tap, NetClientState *peer,
 
     tap_set_sndbuf(s->fd, tap, &err);
     if (err) {
-        error_report_err(err);
-        return -1;
+        error_propagate(errp, err);
+        return;
     }
 
     if (tap->has_fd || tap->has_fds) {
@@ -644,30 +644,28 @@ static int net_init_tap_one(const NetdevTapOptions *tap, NetClientState *peer,
         if (tap->has_vhostfd || tap->has_vhostfds) {
             vhostfd = monitor_fd_param(cur_mon, vhostfdname, &err);
             if (vhostfd == -1) {
-                error_report_err(err);
-                return -1;
+                error_propagate(errp, err);
+                return;
             }
         } else {
             vhostfd = open("/dev/vhost-net", O_RDWR);
             if (vhostfd < 0) {
-                error_report("tap: open vhost char device failed: %s",
-                           strerror(errno));
-                return -1;
+                error_setg_errno(errp, errno,
+                                 "tap: open vhost char device failed: %s");
+                return;
             }
         }
         options.opaque = (void *)(uintptr_t)vhostfd;
 
         s->vhost_net = vhost_net_init(&options);
         if (!s->vhost_net) {
-            error_report("vhost-net requested but could not be initialized");
-            return -1;
+            error_setg(errp,
+                       "vhost-net requested but could not be initialized");
+            return;
         }
     } else if (tap->has_vhostfd || tap->has_vhostfds) {
-        error_report("vhostfd= is not valid without vhost");
-        return -1;
+        error_setg(errp, "vhostfd= is not valid without vhost");
     }
-
-    return 0;
 }
 
 static int get_fds(char *str, char *fds[], int max)
@@ -741,9 +739,11 @@ int net_init_tap(const NetClientOptions *opts, const char *name,
 
         vnet_hdr = tap_probe_vnet_hdr(fd);
 
-        if (net_init_tap_one(tap, peer, "tap", name, NULL,
-                             script, downscript,
-                             vhostfdname, vnet_hdr, fd)) {
+        net_init_tap_one(tap, peer, "tap", name, NULL,
+                         script, downscript,
+                         vhostfdname, vnet_hdr, fd, &err);
+        if (err) {
+            error_report_err(err);
             return -1;
         }
     } else if (tap->has_fds) {
@@ -786,10 +786,12 @@ int net_init_tap(const NetClientOptions *opts, const char *name,
                 return -1;
             }
 
-            if (net_init_tap_one(tap, peer, "tap", name, ifname,
-                                 script, downscript,
-                                 tap->has_vhostfds ? vhost_fds[i] : NULL,
-                                 vnet_hdr, fd)) {
+            net_init_tap_one(tap, peer, "tap", name, ifname,
+                             script, downscript,
+                             tap->has_vhostfds ? vhost_fds[i] : NULL,
+                             vnet_hdr, fd, &err);
+            if (err) {
+                error_report_err(err);
                 return -1;
             }
         }
@@ -810,9 +812,11 @@ int net_init_tap(const NetClientOptions *opts, const char *name,
         fcntl(fd, F_SETFL, O_NONBLOCK);
         vnet_hdr = tap_probe_vnet_hdr(fd);
 
-        if (net_init_tap_one(tap, peer, "bridge", name, ifname,
-                             script, downscript, vhostfdname,
-                             vnet_hdr, fd)) {
+        net_init_tap_one(tap, peer, "bridge", name, ifname,
+                         script, downscript, vhostfdname,
+                         vnet_hdr, fd, &err);
+        if (err) {
+            error_report_err(err);
             close(fd);
             return -1;
         }
@@ -846,10 +850,12 @@ int net_init_tap(const NetClientOptions *opts, const char *name,
                 }
             }
 
-            if (net_init_tap_one(tap, peer, "tap", name, ifname,
-                                 i >= 1 ? "no" : script,
-                                 i >= 1 ? "no" : downscript,
-                                 vhostfdname, vnet_hdr, fd)) {
+            net_init_tap_one(tap, peer, "tap", name, ifname,
+                             i >= 1 ? "no" : script,
+                             i >= 1 ? "no" : downscript,
+                             vhostfdname, vnet_hdr, fd, &err);
+            if (err) {
+                error_report_err(err);
                 close(fd);
                 return -1;
             }
-- 
1.9.3

[Qemu-devel] [PATCH 09/15] tap: Convert launch_script() to Error

[Markus Armbruster]

Fixes inappropriate use of stderr in monitor command handler.

While there, improve the messages some.

Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
 net/tap.c | 40 ++++++++++++++++++++++++++++------------
 1 file changed, 28 insertions(+), 12 deletions(-)

diff --git a/net/tap.c b/net/tap.c
index d1f5644..08bfc51 100644
--- a/net/tap.c
+++ b/net/tap.c
@@ -59,7 +59,8 @@ typedef struct TAPState {
     unsigned host_vnet_hdr_len;
 } TAPState;
 
-static int launch_script(const char *setup_script, const char *ifname, int fd);
+static void launch_script(const char *setup_script, const char *ifname,
+                          int fd, Error **errp);
 
 static int tap_can_send(void *opaque);
 static void tap_send(void *opaque);
@@ -288,6 +289,7 @@ static void tap_set_offload(NetClientState *nc, int csum, int tso4,
 static void tap_cleanup(NetClientState *nc)
 {
     TAPState *s = DO_UPCAST(TAPState, nc, nc);
+    Error **err = NULL;
 
     if (s->vhost_net) {
         vhost_net_cleanup(s->vhost_net);
@@ -296,8 +298,12 @@ static void tap_cleanup(NetClientState *nc)
 
     qemu_purge_queued_packets(nc);
 
-    if (s->down_script[0])
-        launch_script(s->down_script, s->down_script_arg, s->fd);
+    if (s->down_script[0]) {
+        launch_script(s->down_script, s->down_script_arg, s->fd, &err);
+        if (err) {
+            error_report_err(err);
+        }
+    }
 
     tap_read_poll(s, false);
     tap_write_poll(s, false);
@@ -368,7 +374,8 @@ static TAPState *net_tap_fd_init(NetClientState *peer,
     return s;
 }
 
-static int launch_script(const char *setup_script, const char *ifname, int fd)
+static void launch_script(const char *setup_script, const char *ifname,
+                          int fd, Error **errp)
 {
     int pid, status;
     char *args[3];
@@ -376,6 +383,11 @@ static int launch_script(const char *setup_script, const char *ifname, int fd)
 
     /* try to launch network script */
     pid = fork();
+    if (pid < 0) {
+        error_setg_errno(errp, errno, "could not launch network script %s",
+                         setup_script);
+        return;
+    }
     if (pid == 0) {
         int open_max = sysconf(_SC_OPEN_MAX), i;
 
@@ -390,17 +402,17 @@ static int launch_script(const char *setup_script, const char *ifname, int fd)
         *parg = NULL;
         execv(setup_script, args);
         _exit(1);
-    } else if (pid > 0) {
+    } else {
         while (waitpid(pid, &status, 0) != pid) {
             /* loop */
         }
 
         if (WIFEXITED(status) && WEXITSTATUS(status) == 0) {
-            return 0;
+            return;
         }
+        error_setg(errp, "network script %s failed with status %d",
+                   setup_script, status);
     }
-    fprintf(stderr, "%s: could not launch network script\n", setup_script);
-    return -1;
 }
 
 static int recv_fd(int c)
@@ -571,6 +583,7 @@ static int net_tap_init(const NetdevTapOptions *tap, int *vnet_hdr,
                         const char *setup_script, char *ifname,
                         size_t ifname_sz, int mq_required)
 {
+    Error **err = NULL;
     int fd, vnet_hdr_required;
 
     if (tap->has_vnet_hdr) {
@@ -589,10 +602,13 @@ static int net_tap_init(const NetdevTapOptions *tap, int *vnet_hdr,
 
     if (setup_script &&
         setup_script[0] != '\0' &&
-        strcmp(setup_script, "no") != 0 &&
-        launch_script(setup_script, ifname, fd)) {
-        close(fd);
-        return -1;
+        strcmp(setup_script, "no") != 0) {
+        launch_script(setup_script, ifname, fd, &err);
+        if (err) {
+            error_report_err(err);
+            close(fd);
+            return -1;
+        }
     }
 
     return fd;
-- 
1.9.3

[Qemu-devel] [PATCH 10/15] tap: Permit incremental conversion of tap_open() to Error

[Markus Armbruster]

Convert the trivial ones immediately: tap-aix and tap-haiku.

Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
 net/tap-aix.c     |  4 ++--
 net/tap-bsd.c     |  6 ++++--
 net/tap-haiku.c   |  4 ++--
 net/tap-linux.c   |  3 ++-
 net/tap-solaris.c |  3 ++-
 net/tap.c         | 13 +++++++++----
 net/tap_int.h     |  2 +-
 7 files changed, 22 insertions(+), 13 deletions(-)

diff --git a/net/tap-aix.c b/net/tap-aix.c
index 0a3d461..18fdbf3 100644
--- a/net/tap-aix.c
+++ b/net/tap-aix.c
@@ -26,9 +26,9 @@
 #include <stdio.h>
 
 int tap_open(char *ifname, int ifname_size, int *vnet_hdr,
-             int vnet_hdr_required, int mq_required)
+             int vnet_hdr_required, int mq_required, Error **errp)
 {
-    fprintf(stderr, "no tap on AIX\n");
+    error_setg(errp, "no tap on AIX");
     return -1;
 }
 
diff --git a/net/tap-bsd.c b/net/tap-bsd.c
index 53cdd9f..bbbe446 100644
--- a/net/tap-bsd.c
+++ b/net/tap-bsd.c
@@ -35,8 +35,9 @@
 
 #ifndef __FreeBSD__
 int tap_open(char *ifname, int ifname_size, int *vnet_hdr,
-             int vnet_hdr_required, int mq_required)
+             int vnet_hdr_required, int mq_required, Error **errp)
 {
+    /* FIXME error_setg(errp, ...) on failure */
     int fd;
 #ifdef TAPGIFNAME
     struct ifreq ifr;
@@ -114,8 +115,9 @@ int tap_open(char *ifname, int ifname_size, int *vnet_hdr,
 #define PATH_NET_TAP "/dev/tap"
 
 int tap_open(char *ifname, int ifname_size, int *vnet_hdr,
-             int vnet_hdr_required, int mq_required)
+             int vnet_hdr_required, int mq_required, Error **errp)
 {
+    /* FIXME error_setg(errp, ...) on failure */
     int fd, s, ret;
     struct ifreq ifr;
 
diff --git a/net/tap-haiku.c b/net/tap-haiku.c
index 0905b28..d18590c 100644
--- a/net/tap-haiku.c
+++ b/net/tap-haiku.c
@@ -26,9 +26,9 @@
 #include <stdio.h>
 
 int tap_open(char *ifname, int ifname_size, int *vnet_hdr,
-             int vnet_hdr_required, int mq_required)
+             int vnet_hdr_required, int mq_required, Error **errp)
 {
-    fprintf(stderr, "no tap on Haiku\n");
+    error_setg(errp, "no tap on Haiku");
     return -1;
 }
 
diff --git a/net/tap-linux.c b/net/tap-linux.c
index 6fa2744..be18382 100644
--- a/net/tap-linux.c
+++ b/net/tap-linux.c
@@ -37,8 +37,9 @@
 #define PATH_NET_TUN "/dev/net/tun"
 
 int tap_open(char *ifname, int ifname_size, int *vnet_hdr,
-             int vnet_hdr_required, int mq_required)
+             int vnet_hdr_required, int mq_required, Error **errp)
 {
+    /* FIXME error_setg(errp, ...) on failure */
     struct ifreq ifr;
     int fd, ret;
     int len = sizeof(struct virtio_net_hdr);
diff --git a/net/tap-solaris.c b/net/tap-solaris.c
index 7839323..079046b 100644
--- a/net/tap-solaris.c
+++ b/net/tap-solaris.c
@@ -174,8 +174,9 @@ static int tap_alloc(char *dev, size_t dev_size)
 }
 
 int tap_open(char *ifname, int ifname_size, int *vnet_hdr,
-             int vnet_hdr_required, int mq_required)
+             int vnet_hdr_required, int mq_required, Error **errp)
 {
+    /* FIXME error_setg(errp, ...) on failure */
     char  dev[10]="";
     int fd;
     if( (fd = tap_alloc(dev, sizeof(dev))) < 0 ){
diff --git a/net/tap.c b/net/tap.c
index 08bfc51..348b786 100644
--- a/net/tap.c
+++ b/net/tap.c
@@ -581,7 +581,7 @@ int net_init_bridge(const NetClientOptions *opts, const char *name,
 
 static int net_tap_init(const NetdevTapOptions *tap, int *vnet_hdr,
                         const char *setup_script, char *ifname,
-                        size_t ifname_sz, int mq_required)
+                        size_t ifname_sz, int mq_required, Error **errp)
 {
     Error **err = NULL;
     int fd, vnet_hdr_required;
@@ -595,8 +595,12 @@ static int net_tap_init(const NetdevTapOptions *tap, int *vnet_hdr,
     }
 
     TFR(fd = tap_open(ifname, ifname_sz, vnet_hdr, vnet_hdr_required,
-                      mq_required));
+                      mq_required, errp));
     if (fd < 0) {
+        /* FIXME drop when all tap_open() store an Error */
+        if (errp && !*errp) {
+            error_setg(errp, "can't open tap device");
+        }
         return -1;
     }
 
@@ -605,7 +609,7 @@ static int net_tap_init(const NetdevTapOptions *tap, int *vnet_hdr,
         strcmp(setup_script, "no") != 0) {
         launch_script(setup_script, ifname, fd, &err);
         if (err) {
-            error_report_err(err);
+            error_propagate(errp, err);
             close(fd);
             return -1;
         }
@@ -853,8 +857,9 @@ int net_init_tap(const NetClientOptions *opts, const char *name,
 
         for (i = 0; i < queues; i++) {
             fd = net_tap_init(tap, &vnet_hdr, i >= 1 ? "no" : script,
-                              ifname, sizeof ifname, queues > 1);
+                              ifname, sizeof ifname, queues > 1, &err);
             if (fd == -1) {
+                error_report_err(err);
                 return -1;
             }
 
diff --git a/net/tap_int.h b/net/tap_int.h
index 6df271f..d12a409 100644
--- a/net/tap_int.h
+++ b/net/tap_int.h
@@ -30,7 +30,7 @@
 #include "qapi-types.h"
 
 int tap_open(char *ifname, int ifname_size, int *vnet_hdr,
-             int vnet_hdr_required, int mq_required);
+             int vnet_hdr_required, int mq_required, Error **errp);
 
 ssize_t tap_read_packet(int tapfd, uint8_t *buf, int maxlen);
 
-- 
1.9.3

[Qemu-devel] [PATCH 11/15] tap-linux: Convert tap_open() to Error

[Markus Armbruster]

Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
 net/tap-linux.c | 17 +++++++++--------
 1 file changed, 9 insertions(+), 8 deletions(-)

diff --git a/net/tap-linux.c b/net/tap-linux.c
index be18382..6c3caef 100644
--- a/net/tap-linux.c
+++ b/net/tap-linux.c
@@ -39,7 +39,6 @@
 int tap_open(char *ifname, int ifname_size, int *vnet_hdr,
              int vnet_hdr_required, int mq_required, Error **errp)
 {
-    /* FIXME error_setg(errp, ...) on failure */
     struct ifreq ifr;
     int fd, ret;
     int len = sizeof(struct virtio_net_hdr);
@@ -47,7 +46,7 @@ int tap_open(char *ifname, int ifname_size, int *vnet_hdr,
 
     TFR(fd = open(PATH_NET_TUN, O_RDWR));
     if (fd < 0) {
-        error_report("could not open %s: %m", PATH_NET_TUN);
+        error_setg_errno(errp, errno, "could not open %s", PATH_NET_TUN);
         return -1;
     }
     memset(&ifr, 0, sizeof(ifr));
@@ -71,8 +70,8 @@ int tap_open(char *ifname, int ifname_size, int *vnet_hdr,
         }
 
         if (vnet_hdr_required && !*vnet_hdr) {
-            error_report("vnet_hdr=1 requested, but no kernel "
-                         "support for IFF_VNET_HDR available");
+            error_setg(errp, "vnet_hdr=1 requested, but no kernel "
+                       "support for IFF_VNET_HDR available");
             close(fd);
             return -1;
         }
@@ -87,8 +86,8 @@ int tap_open(char *ifname, int ifname_size, int *vnet_hdr,
 
     if (mq_required) {
         if (!(features & IFF_MULTI_QUEUE)) {
-            error_report("multiqueue required, but no kernel "
-                         "support for IFF_MULTI_QUEUE available");
+            error_setg(errp, "multiqueue required, but no kernel "
+                       "support for IFF_MULTI_QUEUE available");
             close(fd);
             return -1;
         } else {
@@ -103,9 +102,11 @@ int tap_open(char *ifname, int ifname_size, int *vnet_hdr,
     ret = ioctl(fd, TUNSETIFF, (void *) &ifr);
     if (ret != 0) {
         if (ifname[0] != '\0') {
-            error_report("could not configure %s (%s): %m", PATH_NET_TUN, ifr.ifr_name);
+            error_setg_errno(errp, errno, "could not configure %s (%s)",
+                             PATH_NET_TUN, ifr.ifr_name);
         } else {
-            error_report("could not configure %s: %m", PATH_NET_TUN);
+            error_setg_errno(errp, errno, "could not configure %s",
+                             PATH_NET_TUN);
         }
         close(fd);
         return -1;
-- 
1.9.3

[Qemu-devel] [PATCH 12/15] tap-bsd: Convert tap_open() to Error

[Markus Armbruster]

Fixes inappropriate use of stderr in monitor command handler.

While there, improve some of the messages a bit.

Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
 net/tap-bsd.c | 33 ++++++++++++++-------------------
 1 file changed, 14 insertions(+), 19 deletions(-)

diff --git a/net/tap-bsd.c b/net/tap-bsd.c
index bbbe446..8d5c776 100644
--- a/net/tap-bsd.c
+++ b/net/tap-bsd.c
@@ -37,7 +37,6 @@
 int tap_open(char *ifname, int ifname_size, int *vnet_hdr,
              int vnet_hdr_required, int mq_required, Error **errp)
 {
-    /* FIXME error_setg(errp, ...) on failure */
     int fd;
 #ifdef TAPGIFNAME
     struct ifreq ifr;
@@ -72,23 +71,19 @@ int tap_open(char *ifname, int ifname_size, int *vnet_hdr,
         }
     }
     if (fd < 0) {
-        error_report("warning: could not open %s (%s): no virtual network emulation",
-                   dname, strerror(errno));
+        error_setg_errno(errp, errno, "could not open %s", dname);
         return -1;
     }
 
 #ifdef TAPGIFNAME
     if (ioctl(fd, TAPGIFNAME, (void *)&ifr) < 0) {
-        fprintf(stderr, "warning: could not get tap name: %s\n",
-            strerror(errno));
+        error_setg_errno(errp, errno, "could not get tap name");
         return -1;
     }
     pstrcpy(ifname, ifname_size, ifr.ifr_name);
 #else
     if (fstat(fd, &s) < 0) {
-        fprintf(stderr,
-            "warning: could not stat /dev/tap: no virtual network emulation: %s\n",
-            strerror(errno));
+        error_setg_errno(errp, errno, "could not stat %s", dname);
         return -1;
     }
     dev = devname(s.st_rdev, S_IFCHR);
@@ -100,8 +95,8 @@ int tap_open(char *ifname, int ifname_size, int *vnet_hdr,
         *vnet_hdr = 0;
 
         if (vnet_hdr_required && !*vnet_hdr) {
-            error_report("vnet_hdr=1 requested, but no kernel "
-                         "support for IFF_VNET_HDR available");
+            error_setg(errp, "vnet_hdr=1 requested, but no kernel "
+                       "support for IFF_VNET_HDR available");
             close(fd);
             return -1;
         }
@@ -117,13 +112,12 @@ int tap_open(char *ifname, int ifname_size, int *vnet_hdr,
 int tap_open(char *ifname, int ifname_size, int *vnet_hdr,
              int vnet_hdr_required, int mq_required, Error **errp)
 {
-    /* FIXME error_setg(errp, ...) on failure */
     int fd, s, ret;
     struct ifreq ifr;
 
     TFR(fd = open(PATH_NET_TAP, O_RDWR));
     if (fd < 0) {
-        error_report("could not open %s: %s", PATH_NET_TAP, strerror(errno));
+        error_setg_errno(errp, errno, "could not open %s", PATH_NET_TAP);
         return -1;
     }
 
@@ -131,7 +125,7 @@ int tap_open(char *ifname, int ifname_size, int *vnet_hdr,
 
     ret = ioctl(fd, TAPGIFNAME, (void *)&ifr);
     if (ret < 0) {
-        error_report("could not get tap interface name");
+        error_setg_errno(errp, errno, "could not get tap interface name");
         goto error;
     }
 
@@ -139,14 +133,15 @@ int tap_open(char *ifname, int ifname_size, int *vnet_hdr,
         /* User requested the interface to have a specific name */
         s = socket(AF_LOCAL, SOCK_DGRAM, 0);
         if (s < 0) {
-            error_report("could not open socket to set interface name");
+            error_setg_errno(errp, errno,
+                             "could not open socket to set interface name");
             goto error;
         }
         ifr.ifr_data = ifname;
         ret = ioctl(s, SIOCSIFNAME, (void *)&ifr);
         close(s);
         if (ret < 0) {
-            error_report("could not set tap interface name");
+            error_setg_errno(errp, errno, "could not set tap interface name");
             goto error;
         }
     } else {
@@ -158,14 +153,14 @@ int tap_open(char *ifname, int ifname_size, int *vnet_hdr,
         *vnet_hdr = 0;
 
         if (vnet_hdr_required && !*vnet_hdr) {
-            error_report("vnet_hdr=1 requested, but no kernel "
-                         "support for IFF_VNET_HDR available");
+            error_setg(errp, "vnet_hdr=1 requested, but no kernel "
+                       "support for IFF_VNET_HDR available");
             goto error;
         }
     }
     if (mq_required) {
-        error_report("mq_required requested, but not kernel support"
-                     "for IFF_MULTI_QUEUE available");
+        error_setg(errp, "mq_required requested, but not kernel support"
+                   "for IFF_MULTI_QUEUE available");
         goto error;
     }
 
-- 
1.9.3

[Qemu-devel] [PATCH 13/15] tap-solaris: Convert tap_open() to Error

[Markus Armbruster]

Fixes inappropriate use of syslog().

Not fixed: leaks on error paths, suspicious non-fatal errors.  FIXMEs
added instead.

Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
 net/tap-solaris.c | 59 ++++++++++++++++++++++++++++---------------------------
 1 file changed, 30 insertions(+), 29 deletions(-)

diff --git a/net/tap-solaris.c b/net/tap-solaris.c
index 079046b..90b2fd1 100644
--- a/net/tap-solaris.c
+++ b/net/tap-solaris.c
@@ -36,7 +36,6 @@
 #include <netinet/udp.h>
 #include <netinet/tcp.h>
 #include <net/if.h>
-#include <syslog.h>
 #include <stropts.h>
 #include "qemu/error-report.h"
 
@@ -56,8 +55,10 @@ ssize_t tap_read_packet(int tapfd, uint8_t *buf, int maxlen)
  * Allocate TAP device, returns opened fd.
  * Stores dev name in the first arg(must be large enough).
  */
-static int tap_alloc(char *dev, size_t dev_size)
+static int tap_alloc(char *dev, size_t dev_size, Error **errp)
 {
+    /* FIXME leaks like a sieve on error paths */
+    /* FIXME suspicious: many errors are reported, then ignored */
     int tap_fd, if_fd, ppa = -1;
     static int ip_fd = 0;
     char *ptr;
@@ -83,14 +84,14 @@ static int tap_alloc(char *dev, size_t dev_size)
 
     TFR(ip_fd = open("/dev/udp", O_RDWR, 0));
     if (ip_fd < 0) {
-       syslog(LOG_ERR, "Can't open /dev/ip (actually /dev/udp)");
-       return -1;
+        error_setg(errp, "Can't open /dev/ip (actually /dev/udp)");
+        return -1;
     }
 
     TFR(tap_fd = open("/dev/tap", O_RDWR, 0));
     if (tap_fd < 0) {
-       syslog(LOG_ERR, "Can't open /dev/tap");
-       return -1;
+        error_setg(errp, "Can't open /dev/tap");
+        return -1;
     }
 
     /* Assign a new PPA and get its unit number. */
@@ -99,20 +100,20 @@ static int tap_alloc(char *dev, size_t dev_size)
     strioc_ppa.ic_len = sizeof(ppa);
     strioc_ppa.ic_dp = (char *)&ppa;
     if ((ppa = ioctl (tap_fd, I_STR, &strioc_ppa)) < 0)
-       syslog (LOG_ERR, "Can't assign new interface");
+        error_report("Can't assign new interface");
 
     TFR(if_fd = open("/dev/tap", O_RDWR, 0));
     if (if_fd < 0) {
-       syslog(LOG_ERR, "Can't open /dev/tap (2)");
-       return -1;
+        error_setg(errp, "Can't open /dev/tap (2)");
+        return -1;
     }
     if(ioctl(if_fd, I_PUSH, "ip") < 0){
-       syslog(LOG_ERR, "Can't push IP module");
-       return -1;
+        error_setg(errp, "Can't push IP module");
+        return -1;
     }
 
     if (ioctl(if_fd, SIOCGLIFFLAGS, &ifr) < 0)
-	syslog(LOG_ERR, "Can't get flags\n");
+        error_report("Can't get flags");
 
     snprintf (actual_name, 32, "tap%d", ppa);
     pstrcpy(ifr.lifr_name, sizeof(ifr.lifr_name), actual_name);
@@ -121,22 +122,22 @@ static int tap_alloc(char *dev, size_t dev_size)
     /* Assign ppa according to the unit number returned by tun device */
 
     if (ioctl (if_fd, SIOCSLIFNAME, &ifr) < 0)
-        syslog (LOG_ERR, "Can't set PPA %d", ppa);
+        error_report("Can't set PPA %d", ppa);
     if (ioctl(if_fd, SIOCGLIFFLAGS, &ifr) <0)
-        syslog (LOG_ERR, "Can't get flags\n");
+        error_report("Can't get flags");
     /* Push arp module to if_fd */
     if (ioctl (if_fd, I_PUSH, "arp") < 0)
-        syslog (LOG_ERR, "Can't push ARP module (2)");
+        error_report("Can't push ARP module (2)");
 
     /* Push arp module to ip_fd */
     if (ioctl (ip_fd, I_POP, NULL) < 0)
-        syslog (LOG_ERR, "I_POP failed\n");
+        error_report("I_POP failed");
     if (ioctl (ip_fd, I_PUSH, "arp") < 0)
-        syslog (LOG_ERR, "Can't push ARP module (3)\n");
+        error_report("Can't push ARP module (3)");
     /* Open arp_fd */
     TFR(arp_fd = open ("/dev/tap", O_RDWR, 0));
     if (arp_fd < 0)
-       syslog (LOG_ERR, "Can't open %s\n", "/dev/tap");
+        error_report("Can't open %s", "/dev/tap");
 
     /* Set ifname to arp */
     strioc_if.ic_cmd = SIOCSLIFNAME;
@@ -144,16 +145,16 @@ static int tap_alloc(char *dev, size_t dev_size)
     strioc_if.ic_len = sizeof(ifr);
     strioc_if.ic_dp = (char *)&ifr;
     if (ioctl(arp_fd, I_STR, &strioc_if) < 0){
-        syslog (LOG_ERR, "Can't set ifname to arp\n");
+        error_report("Can't set ifname to arp");
     }
 
     if((ip_muxid = ioctl(ip_fd, I_LINK, if_fd)) < 0){
-       syslog(LOG_ERR, "Can't link TAP device to IP");
-       return -1;
+        error_setg(errp, "Can't link TAP device to IP");
+        return -1;
     }
 
     if ((arp_muxid = ioctl (ip_fd, link_type, arp_fd)) < 0)
-        syslog (LOG_ERR, "Can't link TAP device to ARP");
+        error_report("Can't link TAP device to ARP");
 
     close (if_fd);
 
@@ -166,7 +167,7 @@ static int tap_alloc(char *dev, size_t dev_size)
     {
       ioctl (ip_fd, I_PUNLINK , arp_muxid);
       ioctl (ip_fd, I_PUNLINK, ip_muxid);
-      syslog (LOG_ERR, "Can't set multiplexor id");
+      error_report("Can't set multiplexor id");
     }
 
     snprintf(dev, dev_size, "tap%d", ppa);
@@ -176,12 +177,12 @@ static int tap_alloc(char *dev, size_t dev_size)
 int tap_open(char *ifname, int ifname_size, int *vnet_hdr,
              int vnet_hdr_required, int mq_required, Error **errp)
 {
-    /* FIXME error_setg(errp, ...) on failure */
     char  dev[10]="";
     int fd;
-    if( (fd = tap_alloc(dev, sizeof(dev))) < 0 ){
-       fprintf(stderr, "Cannot allocate TAP device\n");
-       return -1;
+
+    fd = tap_alloc(dev, sizeof(dev), errp);
+    if (fd < 0) {
+        return -1;
     }
     pstrcpy(ifname, ifname_size, dev);
     if (*vnet_hdr) {
@@ -189,8 +190,8 @@ int tap_open(char *ifname, int ifname_size, int *vnet_hdr,
         *vnet_hdr = 0;
 
         if (vnet_hdr_required && !*vnet_hdr) {
-            error_report("vnet_hdr=1 requested, but no kernel "
-                         "support for IFF_VNET_HDR available");
+            error_setg(errp, "vnet_hdr=1 requested, but no kernel "
+                       "support for IFF_VNET_HDR available");
             close(fd);
             return -1;
         }
-- 
1.9.3

[Qemu-devel] [PATCH 14/15] tap: Finish conversion of tap_open() to Error

[Markus Armbruster]

Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
 net/tap.c | 4 ----
 1 file changed, 4 deletions(-)

diff --git a/net/tap.c b/net/tap.c
index 348b786..f7db9dc 100644
--- a/net/tap.c
+++ b/net/tap.c
@@ -597,10 +597,6 @@ static int net_tap_init(const NetdevTapOptions *tap, int *vnet_hdr,
     TFR(fd = tap_open(ifname, ifname_sz, vnet_hdr, vnet_hdr_required,
                       mq_required, errp));
     if (fd < 0) {
-        /* FIXME drop when all tap_open() store an Error */
-        if (errp && !*errp) {
-            error_setg(errp, "can't open tap device");
-        }
         return -1;
     }
 
-- 
1.9.3

[Qemu-devel] [PATCH 15/15] tap: Improve -netdev/netdev_add/-net/... tap error reporting

[Markus Armbruster]

When -netdev tap fails, it first reports a specific error, then a
generic one, like this:

    $ qemu-system-x86_64 -netdev tap,id=foo
    qemu-system-x86_64: -netdev tap,id=foo: could not configure /dev/net/tun: Operation not permitted
    qemu-system-x86_64: -netdev tap,id=foo: Device 'tap' could not be initialized

With the command line, the messages go to stderr.  In HMP, they go to
the monitor.  In QMP, the second one becomes the error reply, and the
first one goes to stderr.

Convert net_init_tap() to Error.  This suppresses the unwanted second
message, and and makes the specific error the QMP error reply.

Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
 net/tap.c | 45 ++++++++++++++++++++++-----------------------
 1 file changed, 22 insertions(+), 23 deletions(-)

diff --git a/net/tap.c b/net/tap.c
index f7db9dc..8d80bc8 100644
--- a/net/tap.c
+++ b/net/tap.c
@@ -713,7 +713,6 @@ static int get_fds(char *str, char *fds[], int max)
 int net_init_tap(const NetClientOptions *opts, const char *name,
                  NetClientState *peer, Error **errp)
 {
-    /* FIXME error_setg(errp, ...) on failure */
     const NetdevTapOptions *tap;
     int fd, vnet_hdr = 0, i = 0, queues;
     /* for the no-fd, no-helper case */
@@ -731,7 +730,7 @@ int net_init_tap(const NetClientOptions *opts, const char *name,
     /* QEMU vlans does not support multiqueue tap, in this case peer is set.
      * For -netdev, peer is always NULL. */
     if (peer && (tap->has_queues || tap->has_fds || tap->has_vhostfds)) {
-        error_report("Multiqueue tap cannot be used with QEMU vlans");
+        error_setg(errp, "Multiqueue tap cannot be used with QEMU vlans");
         return -1;
     }
 
@@ -739,15 +738,15 @@ int net_init_tap(const NetClientOptions *opts, const char *name,
         if (tap->has_ifname || tap->has_script || tap->has_downscript ||
             tap->has_vnet_hdr || tap->has_helper || tap->has_queues ||
             tap->has_fds || tap->has_vhostfds) {
-            error_report("ifname=, script=, downscript=, vnet_hdr=, "
-                         "helper=, queues=, fds=, and vhostfds= "
-                         "are invalid with fd=");
+            error_setg(errp, "ifname=, script=, downscript=, vnet_hdr=, "
+                       "helper=, queues=, fds=, and vhostfds= "
+                       "are invalid with fd=");
             return -1;
         }
 
         fd = monitor_fd_param(cur_mon, tap->fd, &err);
         if (fd == -1) {
-            error_report_err(err);
+            error_propagate(errp, err);
             return -1;
         }
 
@@ -759,7 +758,7 @@ int net_init_tap(const NetClientOptions *opts, const char *name,
                          script, downscript,
                          vhostfdname, vnet_hdr, fd, &err);
         if (err) {
-            error_report_err(err);
+            error_propagate(errp, err);
             return -1;
         }
     } else if (tap->has_fds) {
@@ -770,9 +769,9 @@ int net_init_tap(const NetClientOptions *opts, const char *name,
         if (tap->has_ifname || tap->has_script || tap->has_downscript ||
             tap->has_vnet_hdr || tap->has_helper || tap->has_queues ||
             tap->has_vhostfd) {
-            error_report("ifname=, script=, downscript=, vnet_hdr=, "
-                         "helper=, queues=, and vhostfd= "
-                         "are invalid with fds=");
+            error_setg(errp, "ifname=, script=, downscript=, vnet_hdr=, "
+                       "helper=, queues=, and vhostfd= "
+                       "are invalid with fds=");
             return -1;
         }
 
@@ -780,8 +779,8 @@ int net_init_tap(const NetClientOptions *opts, const char *name,
         if (tap->has_vhostfds) {
             nvhosts = get_fds(tap->vhostfds, vhost_fds, MAX_TAP_QUEUES);
             if (nfds != nvhosts) {
-                error_report("The number of fds passed does not match the "
-                             "number of vhostfds passed");
+                error_setg(errp, "The number of fds passed does not match "
+                           "the number of vhostfds passed");
                 return -1;
             }
         }
@@ -789,7 +788,7 @@ int net_init_tap(const NetClientOptions *opts, const char *name,
         for (i = 0; i < nfds; i++) {
             fd = monitor_fd_param(cur_mon, fds[i], &err);
             if (fd == -1) {
-                error_report_err(err);
+                error_propagate(errp, err);
                 return -1;
             }
 
@@ -798,7 +797,8 @@ int net_init_tap(const NetClientOptions *opts, const char *name,
             if (i == 0) {
                 vnet_hdr = tap_probe_vnet_hdr(fd);
             } else if (vnet_hdr != tap_probe_vnet_hdr(fd)) {
-                error_report("vnet_hdr not consistent across given tap fds");
+                error_setg(errp,
+                           "vnet_hdr not consistent across given tap fds");
                 return -1;
             }
 
@@ -807,15 +807,15 @@ int net_init_tap(const NetClientOptions *opts, const char *name,
                              tap->has_vhostfds ? vhost_fds[i] : NULL,
                              vnet_hdr, fd, &err);
             if (err) {
-                error_report_err(err);
+                error_propagate(errp, err);
                 return -1;
             }
         }
     } else if (tap->has_helper) {
         if (tap->has_ifname || tap->has_script || tap->has_downscript ||
             tap->has_vnet_hdr || tap->has_queues || tap->has_vhostfds) {
-            error_report("ifname=, script=, downscript=, and vnet_hdr= "
-                         "queues=, and vhostfds= are invalid with helper=");
+            error_setg(errp, "ifname=, script=, downscript=, and vnet_hdr= "
+                       "queues=, and vhostfds= are invalid with helper=");
             return -1;
         }
 
@@ -832,13 +832,13 @@ int net_init_tap(const NetClientOptions *opts, const char *name,
                          script, downscript, vhostfdname,
                          vnet_hdr, fd, &err);
         if (err) {
-            error_report_err(err);
+            error_propagate(errp, err);
             close(fd);
             return -1;
         }
     } else {
         if (tap->has_vhostfds) {
-            error_report("vhostfds= is invalid if fds= wasn't specified");
+            error_setg(errp, "vhostfds= is invalid if fds= wasn't specified");
             return -1;
         }
         script = tap->has_script ? tap->script : DEFAULT_NETWORK_SCRIPT;
@@ -853,15 +853,14 @@ int net_init_tap(const NetClientOptions *opts, const char *name,
 
         for (i = 0; i < queues; i++) {
             fd = net_tap_init(tap, &vnet_hdr, i >= 1 ? "no" : script,
-                              ifname, sizeof ifname, queues > 1, &err);
+                              ifname, sizeof ifname, queues > 1, errp);
             if (fd == -1) {
-                error_report_err(err);
                 return -1;
             }
 
             if (queues > 1 && i == 0 && !tap->has_ifname) {
                 if (tap_fd_get_ifname(fd, ifname)) {
-                    error_report("Fail to get ifname");
+                    error_setg(errp, "Fail to get ifname");
                     close(fd);
                     return -1;
                 }
@@ -872,7 +871,7 @@ int net_init_tap(const NetClientOptions *opts, const char *name,
                              i >= 1 ? "no" : downscript,
                              vhostfdname, vnet_hdr, fd, &err);
             if (err) {
-                error_report_err(err);
+                error_propagate(errp, err);
                 close(fd);
                 return -1;
             }
-- 
1.9.3

Re: [Qemu-devel] [PATCH 01/15] net: Improve error message for -net hubport a bit

[Eric Blake]

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

On 05/12/2015 06:02 AM, Markus Armbruster wrote:
> Type "hubport" is valid only with -netdev.  Unfortunately, that's
> detected late and the error message doesn't explain why:
> 
>     $ qemu-system-i386 -net hubport,id=foo,hubid=0
>     qemu-system-i386: -net hubport,id=foo,hubid=0: Device 'hubport' could not be initialized
> 
> Improve the error message to "Parameter 'type' expects a net type".
> 
> Not fixed: -net hubport without the parameters required by -netdev
> hubport still asks for those parameters:
> 
>     $ qemu-system-i386 -net hubport
>     qemu-system-i386: -net hubport: Parameter 'hubid' is missing
> 
> Signed-off-by: Markus Armbruster <armbru@redhat.com>
> ---
>  net/hub.c | 5 +----
>  net/net.c | 5 +++++
>  2 files changed, 6 insertions(+), 4 deletions(-)
> 

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 02/15] net: Permit incremental conversion of init functions to Error

[Eric Blake]

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

On 05/12/2015 06:02 AM, Markus Armbruster wrote:
> Error reporting for netdev_add is broken: the net_client_init_fun[]
> report the actual errors with (at best) error_report(), and their
> caller net_client_init1() makes up a generic error on top.
> 
> For command line and HMP, this produces an mildly ugly error cascande.

s/cascande/cascade/

> 
> In QMP, the actual errors go to stderr, and the generic error becomes
> the command's error reply.
> 
> To fix this, we need to convert the net_client_init_fun[] to Error.
> 
> To permit fixing them one by one, add an Error ** parameter to the
> net_client_init_fun[].  If the call fails without returning an Error,
> make up the same generic Error as before.  But if it returns one, use
> that instead.  Since none of them does so far, no functional change.
> 
> Signed-off-by: Markus Armbruster <armbru@redhat.com>
> ---

> @@ -802,7 +803,7 @@ static int net_init_nic(const NetClientOptions *opts, const char *name,
>  static int (* const net_client_init_fun[NET_CLIENT_OPTIONS_KIND_MAX])(
>      const NetClientOptions *opts,
>      const char *name,
> -    NetClientState *peer) = {
> +    NetClientState *peer, Error **errp) = {
>          [NET_CLIENT_OPTIONS_KIND_NIC]       = net_init_nic,

Not this patch, but would this be easier to read with the use of a
typedef for the function pointer?

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 03/15] net: Improve -net nic error reporting

[Eric Blake]

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

On 05/12/2015 06:02 AM, Markus Armbruster wrote:
> When -net nic fails, it first reports a specific error, then a generic
> one, like this:
> 
>     $ qemu-system-x86_64 -net nic,netdev=nonexistant
>     qemu-system-x86_64: -net nic,netdev=nonexistant: netdev 'nonexistant' not found
>     qemu-system-x86_64: -net nic,netdev=nonexistant: Device 'nic' could not be initialized

s/nonexistant/nonexistent/g

> 
> Convert net_init_nic() to Error to get rid of the unwanted second
> error message.
> 
> Signed-off-by: Markus Armbruster <armbru@redhat.com>
> ---
>  net/net.c | 12 ++++++------
>  1 file changed, 6 insertions(+), 6 deletions(-)
> 

> @@ -745,7 +744,7 @@ static int net_init_nic(const NetClientOptions *opts, const char *name,
>  
>      idx = nic_get_free_idx();
>      if (idx == -1 || nb_nics >= MAX_NICS) {
> -        error_report("Too Many NICs");
> +        error_setg(errp, "Too Many NICs");

worth s/Many/many/ while touching this?

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 04/15] net/dump: Improve -net/host_net_add dump error reporting

[Eric Blake]

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

On 05/12/2015 06:02 AM, Markus Armbruster wrote:
> When -net dump fails, it first reports a specific error, then a
> generic one, like this:
> 
>     $ qemu-system-x86_64 -net dump,id=foo,file=/eperm
>     qemu-system-x86_64: -net dump,id=foo,file=/eperm: -net dump: can't open /eperm
>     qemu-system-x86_64: -net dump,id=foo,file=/eperm: Device 'dump' could not be initialized
> 
> Convert net_init_tap() to Error.  This suppresses the unwanted second
> message.
> 
> Signed-off-by: Markus Armbruster <armbru@redhat.com>
> ---
>  net/dump.c | 12 ++++++------
>  1 file changed, 6 insertions(+), 6 deletions(-)
> 

> @@ -111,7 +112,7 @@ static int net_dump_init(NetClientState *peer, const char *device,
>  
>      fd = open(filename, O_CREAT | O_TRUNC | O_WRONLY | O_BINARY, 0644);
>      if (fd < 0) {
> -        error_report("-net dump: can't open %s", filename);
> +        error_setg_errno(errp, errno, "-net dump: can't open %s", filename);

It also adds the strerror() text into the output of this message.  Might
be worth mentioning that in the commit message.

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 05/15] tap: net_tap_fd_init() can't fail, drop dead error handling

[Eric Blake]

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

On 05/12/2015 06:02 AM, Markus Armbruster wrote:
> Signed-off-by: Markus Armbruster <armbru@redhat.com>
> ---
>  net/tap.c | 14 +-------------
>  1 file changed, 1 insertion(+), 13 deletions(-)
> 

> @@ -552,14 +551,8 @@ int net_init_bridge(const NetClientOptions *opts, const char *name,
>      }
>  
>      fcntl(fd, F_SETFL, O_NONBLOCK);

Well, fcntl() could fail.  And don't we have the helper function
util/oslib-posix.c:qemu_set_nonblock() for doing this correctly?  (Then
again, that helper also ignores failures).

But the raw use of fcntl here is pre-existing and you didn't touch it,
so it doesn't affect my review of 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 06/15] tap: Improve -netdev/netdev_add/-net/... bridge error reporting

[Eric Blake]

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

On 05/12/2015 06:02 AM, Markus Armbruster wrote:
> When -netdev bridge fails, it first reports a specific error, then a
> generic one, like this:
> 
>     $ qemu-system-x86_64 -netdev bridge,id=foo
>     failed to launch bridge helper
>     qemu-system-x86_64: -netdev bridge,id=foo: Device 'bridge' could not be initialized
> 
> The first message goes to stderr.  Wrong for HMP, because errors need
> to go to the monitor there.
> 
> The second message goes to stderr for -netdev, to the monitor for HMP
> netdev_add, and becomes the error reply for QMP netdev_add.
> 
> Convert net_bridge_run_helper() to Error, and propagate its errors
> through net_init_bridge().  This ensures the error gets reported where
> the user is, and suppresses the unwanted second message.
> 
> While there, improve the error messages a bit.
> 
> The above example becomes:
> 
>     $ qemu-system-x86_64 -netdev bridge,id=foo
>     qemu-system-x86_64: -netdev bridge,id=foo: bridge helper failed
> 
> net_init_tap() also uses net_bridge_run_helper().  Propagate its
> errors there as well.  Improves reporting these errors with -netdev
> tap & friends.
> 
> Signed-off-by: Markus Armbruster <armbru@redhat.com>
> ---
>  net/tap.c | 30 +++++++++++++++++++-----------
>  1 file changed, 19 insertions(+), 11 deletions(-)
> 

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 07/15] tap: Convert tap_set_sndbuf() to Error

[Eric Blake]

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

On 05/12/2015 06:02 AM, Markus Armbruster wrote:
> Signed-off-by: Markus Armbruster <armbru@redhat.com>
> ---
>  net/tap-aix.c     | 3 +--
>  net/tap-bsd.c     | 3 +--
>  net/tap-haiku.c   | 3 +--
>  net/tap-linux.c   | 6 ++----
>  net/tap-solaris.c | 3 +--
>  net/tap.c         | 4 +++-
>  net/tap_int.h     | 2 +-
>  7 files changed, 10 insertions(+), 14 deletions(-)
> 

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 08/15] tap: Convert net_init_tap_one() to Error

[Eric Blake]

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

On 05/12/2015 06:03 AM, Markus Armbruster wrote:
> Signed-off-by: Markus Armbruster <armbru@redhat.com>
> ---
>  net/tap.c | 70 ++++++++++++++++++++++++++++++++++-----------------------------
>  1 file changed, 38 insertions(+), 32 deletions(-)
> 

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 09/15] tap: Convert launch_script() to Error

[Eric Blake]

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

On 05/12/2015 06:03 AM, Markus Armbruster wrote:
> Fixes inappropriate use of stderr in monitor command handler.
> 
> While there, improve the messages some.
> 
> Signed-off-by: Markus Armbruster <armbru@redhat.com>
> ---
>  net/tap.c | 40 ++++++++++++++++++++++++++++------------
>  1 file changed, 28 insertions(+), 12 deletions(-)
> 

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 10/15] tap: Permit incremental conversion of tap_open() to Error

[Eric Blake]

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

On 05/12/2015 06:03 AM, Markus Armbruster wrote:
> Convert the trivial ones immediately: tap-aix and tap-haiku.
> 
> Signed-off-by: Markus Armbruster <armbru@redhat.com>
> ---
>  net/tap-aix.c     |  4 ++--
>  net/tap-bsd.c     |  6 ++++--
>  net/tap-haiku.c   |  4 ++--
>  net/tap-linux.c   |  3 ++-
>  net/tap-solaris.c |  3 ++-
>  net/tap.c         | 13 +++++++++----
>  net/tap_int.h     |  2 +-
>  7 files changed, 22 insertions(+), 13 deletions(-)
> 

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 11/15] tap-linux: Convert tap_open() to Error

[Eric Blake]

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

On 05/12/2015 06:03 AM, Markus Armbruster wrote:
> Signed-off-by: Markus Armbruster <armbru@redhat.com>
> ---
>  net/tap-linux.c | 17 +++++++++--------
>  1 file changed, 9 insertions(+), 8 deletions(-)
> 

> @@ -47,7 +46,7 @@ int tap_open(char *ifname, int ifname_size, int *vnet_hdr,
>  
>      TFR(fd = open(PATH_NET_TUN, O_RDWR));
>      if (fd < 0) {
> -        error_report("could not open %s: %m", PATH_NET_TUN);
> +        error_setg_errno(errp, errno, "could not open %s", PATH_NET_TUN);

Oh nice - gets rid of non-portable uses of %m (of course, the file is
only compiled on Linux where %m is supported, so it is not a bug fix).

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 12/15] tap-bsd: Convert tap_open() to Error

[Eric Blake]

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

On 05/12/2015 06:03 AM, Markus Armbruster wrote:
> Fixes inappropriate use of stderr in monitor command handler.
> 
> While there, improve some of the messages a bit.
> 
> Signed-off-by: Markus Armbruster <armbru@redhat.com>
> ---
>  net/tap-bsd.c | 33 ++++++++++++++-------------------
>  1 file changed, 14 insertions(+), 19 deletions(-)
> 

> @@ -139,14 +133,15 @@ int tap_open(char *ifname, int ifname_size, int *vnet_hdr,
>          /* User requested the interface to have a specific name */
>          s = socket(AF_LOCAL, SOCK_DGRAM, 0);
>          if (s < 0) {
> -            error_report("could not open socket to set interface name");
> +            error_setg_errno(errp, errno,
> +                             "could not open socket to set interface name");
>              goto error;
>          }
>          ifr.ifr_data = ifname;
>          ret = ioctl(s, SIOCSIFNAME, (void *)&ifr);
>          close(s);
>          if (ret < 0) {
> -            error_report("could not set tap interface name");
> +            error_setg_errno(errp, errno, "could not set tap interface name");

Bad. close() may have clobbered errno before you get to report it.

>              goto error;
>          }
>      } else {
> @@ -158,14 +153,14 @@ int tap_open(char *ifname, int ifname_size, int *vnet_hdr,
>          *vnet_hdr = 0;
>  
>          if (vnet_hdr_required && !*vnet_hdr) {
> -            error_report("vnet_hdr=1 requested, but no kernel "
> -                         "support for IFF_VNET_HDR available");
> +            error_setg(errp, "vnet_hdr=1 requested, but no kernel "
> +                       "support for IFF_VNET_HDR available");
>              goto error;
>          }
>      }
>      if (mq_required) {
> -        error_report("mq_required requested, but not kernel support"
> -                     "for IFF_MULTI_QUEUE available");
> +        error_setg(errp, "mq_required requested, but not kernel support"

As long as you are touching this, s/not/no/

> +                   "for IFF_MULTI_QUEUE available");
>          goto error;
>      }
>  
> 

-- 
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 13/15] tap-solaris: Convert tap_open() to Error

[Eric Blake]

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

On 05/12/2015 06:03 AM, Markus Armbruster wrote:
> Fixes inappropriate use of syslog().
> 
> Not fixed: leaks on error paths, suspicious non-fatal errors.  FIXMEs
> added instead.

At least you're admitting where the code is still bad.

> 
> Signed-off-by: Markus Armbruster <armbru@redhat.com>
> ---
>  net/tap-solaris.c | 59 ++++++++++++++++++++++++++++---------------------------
>  1 file changed, 30 insertions(+), 29 deletions(-)
> 

> @@ -99,20 +100,20 @@ static int tap_alloc(char *dev, size_t dev_size)
>      strioc_ppa.ic_len = sizeof(ppa);
>      strioc_ppa.ic_dp = (char *)&ppa;
>      if ((ppa = ioctl (tap_fd, I_STR, &strioc_ppa)) < 0)
> -       syslog (LOG_ERR, "Can't assign new interface");
> +        error_report("Can't assign new interface");

I see you're fixing spacing while at it, as well.

>  
>      TFR(if_fd = open("/dev/tap", O_RDWR, 0));
>      if (if_fd < 0) {
> -       syslog(LOG_ERR, "Can't open /dev/tap (2)");
> -       return -1;
> +        error_setg(errp, "Can't open /dev/tap (2)");
> +        return -1;
>      }
>      if(ioctl(if_fd, I_PUSH, "ip") < 0){
> -       syslog(LOG_ERR, "Can't push IP module");

Should you add the space after 'if' while touching this?

> -       return -1;
> +        error_setg(errp, "Can't push IP module");
> +        return -1;
>      }
>  
>      if (ioctl(if_fd, SIOCGLIFFLAGS, &ifr) < 0)
> -	syslog(LOG_ERR, "Can't get flags\n");
> +        error_report("Can't get flags");

What about adding missing {} while touching this file?  Hmm - there's
enough cruft that it may involve a separate patch just to clean up
style. For this patch, I'm not going to hold up review on style.

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 14/15] tap: Finish conversion of tap_open() to Error

[Eric Blake]

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

On 05/12/2015 06:03 AM, Markus Armbruster wrote:
> Signed-off-by: Markus Armbruster <armbru@redhat.com>
> ---
>  net/tap.c | 4 ----
>  1 file changed, 4 deletions(-)

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 15/15] tap: Improve -netdev/netdev_add/-net/... tap error reporting

[Eric Blake]

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

On 05/12/2015 06:03 AM, Markus Armbruster wrote:
> When -netdev tap fails, it first reports a specific error, then a
> generic one, like this:
> 
>     $ qemu-system-x86_64 -netdev tap,id=foo
>     qemu-system-x86_64: -netdev tap,id=foo: could not configure /dev/net/tun: Operation not permitted
>     qemu-system-x86_64: -netdev tap,id=foo: Device 'tap' could not be initialized
> 
> With the command line, the messages go to stderr.  In HMP, they go to
> the monitor.  In QMP, the second one becomes the error reply, and the
> first one goes to stderr.
> 
> Convert net_init_tap() to Error.  This suppresses the unwanted second
> message, and and makes the specific error the QMP error reply.

s/and and/and/

> 
> Signed-off-by: Markus Armbruster <armbru@redhat.com>
> ---
>  net/tap.c | 45 ++++++++++++++++++++++-----------------------
>  1 file changed, 22 insertions(+), 23 deletions(-)
> 


> @@ -807,15 +807,15 @@ int net_init_tap(const NetClientOptions *opts, const char *name,
>                               tap->has_vhostfds ? vhost_fds[i] : NULL,
>                               vnet_hdr, fd, &err);
>              if (err) {
> -                error_report_err(err);
> +                error_propagate(errp, err);
>                  return -1;
>              }
>          }
>      } else if (tap->has_helper) {
>          if (tap->has_ifname || tap->has_script || tap->has_downscript ||
>              tap->has_vnet_hdr || tap->has_queues || tap->has_vhostfds) {
> -            error_report("ifname=, script=, downscript=, and vnet_hdr= "
> -                         "queues=, and vhostfds= are invalid with helper=");
> +            error_setg(errp, "ifname=, script=, downscript=, and vnet_hdr= "
> +                       "queues=, and vhostfds= are invalid with helper=");

As long as you are touching this, s/and vnet_hdr=/vnet_hdr=,/

Minor enough that I'm okay with fixing it, and adding:
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 03/15] net: Improve -net nic error reporting

[Markus Armbruster]

Eric Blake <eblake@redhat.com> writes:

> On 05/12/2015 06:02 AM, Markus Armbruster wrote:
>> When -net nic fails, it first reports a specific error, then a generic
>> one, like this:
>> 
>>     $ qemu-system-x86_64 -net nic,netdev=nonexistant
>>     qemu-system-x86_64: -net nic,netdev=nonexistant: netdev
>> nonexistant' not found
>>     qemu-system-x86_64: -net nic,netdev=nonexistant: Device 'nic'
>> could not be initialized
>
> s/nonexistant/nonexistent/g

Fixing...

>> 
>> Convert net_init_nic() to Error to get rid of the unwanted second
>> error message.
>> 
>> Signed-off-by: Markus Armbruster <armbru@redhat.com>
>> ---
>>  net/net.c | 12 ++++++------
>>  1 file changed, 6 insertions(+), 6 deletions(-)
>> 
>
>> @@ -745,7 +744,7 @@ static int net_init_nic(const NetClientOptions *opts, const char *name,
>>  
>>      idx = nic_get_free_idx();
>>      if (idx == -1 || nb_nics >= MAX_NICS) {
>> -        error_report("Too Many NICs");
>> +        error_setg(errp, "Too Many NICs");
>
> worth s/Many/many/ while touching this?

Yup.

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

Thanks!

Re: [Qemu-devel] [PATCH 05/15] tap: net_tap_fd_init() can't fail, drop dead error handling

[Markus Armbruster]

Eric Blake <eblake@redhat.com> writes:

> On 05/12/2015 06:02 AM, Markus Armbruster wrote:
>> Signed-off-by: Markus Armbruster <armbru@redhat.com>
>> ---
>>  net/tap.c | 14 +-------------
>>  1 file changed, 1 insertion(+), 13 deletions(-)
>> 
>
>> @@ -552,14 +551,8 @@ int net_init_bridge(const NetClientOptions *opts, const char *name,
>>      }
>>  
>>      fcntl(fd, F_SETFL, O_NONBLOCK);
>
> Well, fcntl() could fail.  And don't we have the helper function
> util/oslib-posix.c:qemu_set_nonblock() for doing this correctly?  (Then
> again, that helper also ignores failures).
>
> But the raw use of fcntl here is pre-existing and you didn't touch it,
> so it doesn't affect my review of this patch.

I guess using the helper makes some sense here, but it's outside this
series' scope.

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

Thanks!

Re: [Qemu-devel] [PATCH 12/15] tap-bsd: Convert tap_open() to Error

[Markus Armbruster]

Eric Blake <eblake@redhat.com> writes:

> On 05/12/2015 06:03 AM, Markus Armbruster wrote:
>> Fixes inappropriate use of stderr in monitor command handler.
>> 
>> While there, improve some of the messages a bit.
>> 
>> Signed-off-by: Markus Armbruster <armbru@redhat.com>
>> ---
>>  net/tap-bsd.c | 33 ++++++++++++++-------------------
>>  1 file changed, 14 insertions(+), 19 deletions(-)
>> 
>
>> @@ -139,14 +133,15 @@ int tap_open(char *ifname, int ifname_size, int *vnet_hdr,
>>          /* User requested the interface to have a specific name */
>>          s = socket(AF_LOCAL, SOCK_DGRAM, 0);
>>          if (s < 0) {
>> -            error_report("could not open socket to set interface name");
>> +            error_setg_errno(errp, errno,
>> +                             "could not open socket to set interface name");
>>              goto error;
>>          }
>>          ifr.ifr_data = ifname;
>>          ret = ioctl(s, SIOCSIFNAME, (void *)&ifr);
>>          close(s);
>>          if (ret < 0) {
>> -            error_report("could not set tap interface name");
>> +            error_setg_errno(errp, errno, "could not set tap interface name");
>
> Bad. close() may have clobbered errno before you get to report it.

You're right.  I'll dumb this down again.

>>              goto error;
>>          }
>>      } else {
>> @@ -158,14 +153,14 @@ int tap_open(char *ifname, int ifname_size, int *vnet_hdr,
>>          *vnet_hdr = 0;
>>  
>>          if (vnet_hdr_required && !*vnet_hdr) {
>> -            error_report("vnet_hdr=1 requested, but no kernel "
>> -                         "support for IFF_VNET_HDR available");
>> +            error_setg(errp, "vnet_hdr=1 requested, but no kernel "
>> +                       "support for IFF_VNET_HDR available");
>>              goto error;
>>          }
>>      }
>>      if (mq_required) {
>> -        error_report("mq_required requested, but not kernel support"
>> -                     "for IFF_MULTI_QUEUE available");
>> +        error_setg(errp, "mq_required requested, but not kernel support"
>
> As long as you are touching this, s/not/no/

Fixing...

>> +                   "for IFF_MULTI_QUEUE available");
>>          goto error;
>>      }

Re: [Qemu-devel] [PATCH 13/15] tap-solaris: Convert tap_open() to Error

[Markus Armbruster]

Eric Blake <eblake@redhat.com> writes:

> On 05/12/2015 06:03 AM, Markus Armbruster wrote:
>> Fixes inappropriate use of syslog().
>> 
>> Not fixed: leaks on error paths, suspicious non-fatal errors.  FIXMEs
>> added instead.
>
> At least you're admitting where the code is still bad.

Actually, git-rm felt pretty tempting.

>> Signed-off-by: Markus Armbruster <armbru@redhat.com>
>> ---
>>  net/tap-solaris.c | 59 ++++++++++++++++++++++++++++---------------------------
>>  1 file changed, 30 insertions(+), 29 deletions(-)
>> 
>
>> @@ -99,20 +100,20 @@ static int tap_alloc(char *dev, size_t dev_size)
>>      strioc_ppa.ic_len = sizeof(ppa);
>>      strioc_ppa.ic_dp = (char *)&ppa;
>>      if ((ppa = ioctl (tap_fd, I_STR, &strioc_ppa)) < 0)
>> -       syslog (LOG_ERR, "Can't assign new interface");
>> +        error_report("Can't assign new interface");
>
> I see you're fixing spacing while at it, as well.
>
>>  
>>      TFR(if_fd = open("/dev/tap", O_RDWR, 0));
>>      if (if_fd < 0) {
>> -       syslog(LOG_ERR, "Can't open /dev/tap (2)");
>> -       return -1;
>> +        error_setg(errp, "Can't open /dev/tap (2)");
>> +        return -1;
>>      }
>>      if(ioctl(if_fd, I_PUSH, "ip") < 0){
>> -       syslog(LOG_ERR, "Can't push IP module");
>
> Should you add the space after 'if' while touching this?

Fixing up just enough to make checkpatch happy.  Also keeps git-blame
useful even without -w.

>> -       return -1;
>> +        error_setg(errp, "Can't push IP module");
>> +        return -1;
>>      }
>>  
>>      if (ioctl(if_fd, SIOCGLIFFLAGS, &ifr) < 0)
>> -	syslog(LOG_ERR, "Can't get flags\n");
>> +        error_report("Can't get flags");
>
> What about adding missing {} while touching this file?  Hmm - there's
> enough cruft that it may involve a separate patch just to clean up
> style. For this patch, I'm not going to hold up review on style.
>
> Reviewed-by: Eric Blake <eblake@redhat.com>

Thanks!

Re: [Qemu-devel] [PATCH 15/15] tap: Improve -netdev/netdev_add/-net/... tap error reporting

[Markus Armbruster]

Eric Blake <eblake@redhat.com> writes:

> On 05/12/2015 06:03 AM, Markus Armbruster wrote:
>> When -netdev tap fails, it first reports a specific error, then a
>> generic one, like this:
>> 
>>     $ qemu-system-x86_64 -netdev tap,id=foo
>>     qemu-system-x86_64: -netdev tap,id=foo: could not configure
>> /dev/net/tun: Operation not permitted
>>     qemu-system-x86_64: -netdev tap,id=foo: Device 'tap' could not
>> be initialized
>> 
>> With the command line, the messages go to stderr.  In HMP, they go to
>> the monitor.  In QMP, the second one becomes the error reply, and the
>> first one goes to stderr.
>> 
>> Convert net_init_tap() to Error.  This suppresses the unwanted second
>> message, and and makes the specific error the QMP error reply.
>
> s/and and/and/

Fixing...

>> Signed-off-by: Markus Armbruster <armbru@redhat.com>
>> ---
>>  net/tap.c | 45 ++++++++++++++++++++++-----------------------
>>  1 file changed, 22 insertions(+), 23 deletions(-)
>> 
>
>
>> @@ -807,15 +807,15 @@ int net_init_tap(const NetClientOptions *opts, const char *name,
>>                               tap->has_vhostfds ? vhost_fds[i] : NULL,
>>                               vnet_hdr, fd, &err);
>>              if (err) {
>> -                error_report_err(err);
>> +                error_propagate(errp, err);
>>                  return -1;
>>              }
>>          }
>>      } else if (tap->has_helper) {
>>          if (tap->has_ifname || tap->has_script || tap->has_downscript ||
>>              tap->has_vnet_hdr || tap->has_queues || tap->has_vhostfds) {
>> -            error_report("ifname=, script=, downscript=, and vnet_hdr= "
>> -                         "queues=, and vhostfds= are invalid with helper=");
>> +            error_setg(errp, "ifname=, script=, downscript=, and vnet_hdr= "
>> +                       "queues=, and vhostfds= are invalid with helper=");
>
> As long as you are touching this, s/and vnet_hdr=/vnet_hdr=,/

Yes.

> Minor enough that I'm okay with fixing it, and adding:
> Reviewed-by: Eric Blake <eblake@redhat.com>

Thanks!