* [Qemu-devel] [PATCH v2 01/15] net: Improve error message for -net hubport a bit
2015-05-15 11:58 [Qemu-devel] [PATCH v2 00/15] net: Improve error reporting Markus Armbruster
@ 2015-05-15 11:58 ` Markus Armbruster
2015-05-15 11:58 ` [Qemu-devel] [PATCH v2 02/15] net: Permit incremental conversion of init functions to Error Markus Armbruster
` (14 subsequent siblings)
15 siblings, 0 replies; 20+ messages in thread
From: Markus Armbruster @ 2015-05-15 11:58 UTC (permalink / raw)
To: qemu-devel; +Cc: eblake, stefanha
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>
Reviewed-by: Eric Blake <eblake@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 7427f6a..d9aaeb5 100644
--- a/net/net.c
+++ b/net/net.c
@@ -882,6 +882,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
^ permalink raw reply related [flat|nested] 20+ messages in thread* [Qemu-devel] [PATCH v2 02/15] net: Permit incremental conversion of init functions to Error
2015-05-15 11:58 [Qemu-devel] [PATCH v2 00/15] net: Improve error reporting Markus Armbruster
2015-05-15 11:58 ` [Qemu-devel] [PATCH v2 01/15] net: Improve error message for -net hubport a bit Markus Armbruster
@ 2015-05-15 11:58 ` Markus Armbruster
2015-05-15 11:58 ` [Qemu-devel] [PATCH v2 03/15] net: Improve -net nic error reporting Markus Armbruster
` (13 subsequent siblings)
15 siblings, 0 replies; 20+ messages in thread
From: Markus Armbruster @ 2015-05-15 11:58 UTC (permalink / raw)
To: qemu-devel; +Cc: eblake, stefanha
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 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>
Reviewed-by: Eric Blake <eblake@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 d9aaeb5..3295741 100644
--- a/net/net.c
+++ b/net/net.c
@@ -740,8 +740,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;
@@ -809,7 +810,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,
@@ -902,10 +903,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
^ permalink raw reply related [flat|nested] 20+ messages in thread* [Qemu-devel] [PATCH v2 03/15] net: Improve -net nic error reporting
2015-05-15 11:58 [Qemu-devel] [PATCH v2 00/15] net: Improve error reporting Markus Armbruster
2015-05-15 11:58 ` [Qemu-devel] [PATCH v2 01/15] net: Improve error message for -net hubport a bit Markus Armbruster
2015-05-15 11:58 ` [Qemu-devel] [PATCH v2 02/15] net: Permit incremental conversion of init functions to Error Markus Armbruster
@ 2015-05-15 11:58 ` Markus Armbruster
2015-05-15 11:58 ` [Qemu-devel] [PATCH v2 04/15] net/dump: Improve -net/host_net_add dump " Markus Armbruster
` (12 subsequent siblings)
15 siblings, 0 replies; 20+ messages in thread
From: Markus Armbruster @ 2015-05-15 11:58 UTC (permalink / raw)
To: qemu-devel; +Cc: eblake, stefanha
When -net nic fails, it first reports a specific error, then a generic
one, like this:
$ qemu-system-x86_64 -net nic,netdev=nonexistent
qemu-system-x86_64: -net nic,netdev=nonexistent: netdev 'nonexistent' not found
qemu-system-x86_64: -net nic,netdev=nonexistent: Device 'nic' could not be initialized
Convert net_init_nic() to Error to get rid of the unwanted second
error message.
While there, tidy up an Overcapitalized Error Message.
Signed-off-by: Markus Armbruster <armbru@redhat.com>
Reviewed-by: Eric Blake <eblake@redhat.com>
---
net/net.c | 12 ++++++------
1 file changed, 6 insertions(+), 6 deletions(-)
diff --git a/net/net.c b/net/net.c
index 3295741..0fe5b8b 100644
--- a/net/net.c
+++ b/net/net.c
@@ -742,7 +742,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;
@@ -752,7 +751,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;
}
@@ -763,7 +762,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 {
@@ -780,19 +779,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
^ permalink raw reply related [flat|nested] 20+ messages in thread* [Qemu-devel] [PATCH v2 04/15] net/dump: Improve -net/host_net_add dump error reporting
2015-05-15 11:58 [Qemu-devel] [PATCH v2 00/15] net: Improve error reporting Markus Armbruster
` (2 preceding siblings ...)
2015-05-15 11:58 ` [Qemu-devel] [PATCH v2 03/15] net: Improve -net nic error reporting Markus Armbruster
@ 2015-05-15 11:58 ` Markus Armbruster
2015-05-15 11:58 ` [Qemu-devel] [PATCH v2 05/15] tap: net_tap_fd_init() can't fail, drop dead error handling Markus Armbruster
` (11 subsequent siblings)
15 siblings, 0 replies; 20+ messages in thread
From: Markus Armbruster @ 2015-05-15 11:58 UTC (permalink / raw)
To: qemu-devel; +Cc: eblake, stefanha
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.
Improve the error messages to include strerror(errno) where
appropriate.
Signed-off-by: Markus Armbruster <armbru@redhat.com>
Reviewed-by: Eric Blake <eblake@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
^ permalink raw reply related [flat|nested] 20+ messages in thread* [Qemu-devel] [PATCH v2 05/15] tap: net_tap_fd_init() can't fail, drop dead error handling
2015-05-15 11:58 [Qemu-devel] [PATCH v2 00/15] net: Improve error reporting Markus Armbruster
` (3 preceding siblings ...)
2015-05-15 11:58 ` [Qemu-devel] [PATCH v2 04/15] net/dump: Improve -net/host_net_add dump " Markus Armbruster
@ 2015-05-15 11:58 ` Markus Armbruster
2015-05-15 11:58 ` [Qemu-devel] [PATCH v2 06/15] tap: Improve -netdev/netdev_add/-net/... bridge error reporting Markus Armbruster
` (10 subsequent siblings)
15 siblings, 0 replies; 20+ messages in thread
From: Markus Armbruster @ 2015-05-15 11:58 UTC (permalink / raw)
To: qemu-devel; +Cc: eblake, stefanha
Signed-off-by: Markus Armbruster <armbru@redhat.com>
Reviewed-by: Eric Blake <eblake@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
^ permalink raw reply related [flat|nested] 20+ messages in thread* [Qemu-devel] [PATCH v2 06/15] tap: Improve -netdev/netdev_add/-net/... bridge error reporting
2015-05-15 11:58 [Qemu-devel] [PATCH v2 00/15] net: Improve error reporting Markus Armbruster
` (4 preceding siblings ...)
2015-05-15 11:58 ` [Qemu-devel] [PATCH v2 05/15] tap: net_tap_fd_init() can't fail, drop dead error handling Markus Armbruster
@ 2015-05-15 11:58 ` Markus Armbruster
2015-05-15 11:58 ` [Qemu-devel] [PATCH v2 07/15] tap: Convert tap_set_sndbuf() to Error Markus Armbruster
` (9 subsequent siblings)
15 siblings, 0 replies; 20+ messages in thread
From: Markus Armbruster @ 2015-05-15 11:58 UTC (permalink / raw)
To: qemu-devel; +Cc: eblake, stefanha
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>
Reviewed-by: Eric Blake <eblake@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
^ permalink raw reply related [flat|nested] 20+ messages in thread* [Qemu-devel] [PATCH v2 07/15] tap: Convert tap_set_sndbuf() to Error
2015-05-15 11:58 [Qemu-devel] [PATCH v2 00/15] net: Improve error reporting Markus Armbruster
` (5 preceding siblings ...)
2015-05-15 11:58 ` [Qemu-devel] [PATCH v2 06/15] tap: Improve -netdev/netdev_add/-net/... bridge error reporting Markus Armbruster
@ 2015-05-15 11:58 ` Markus Armbruster
2015-05-15 11:58 ` [Qemu-devel] [PATCH v2 08/15] tap: Convert net_init_tap_one() " Markus Armbruster
` (8 subsequent siblings)
15 siblings, 0 replies; 20+ messages in thread
From: Markus Armbruster @ 2015-05-15 11:58 UTC (permalink / raw)
To: qemu-devel; +Cc: eblake, stefanha
Signed-off-by: Markus Armbruster <armbru@redhat.com>
Reviewed-by: Eric Blake <eblake@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
^ permalink raw reply related [flat|nested] 20+ messages in thread* [Qemu-devel] [PATCH v2 08/15] tap: Convert net_init_tap_one() to Error
2015-05-15 11:58 [Qemu-devel] [PATCH v2 00/15] net: Improve error reporting Markus Armbruster
` (6 preceding siblings ...)
2015-05-15 11:58 ` [Qemu-devel] [PATCH v2 07/15] tap: Convert tap_set_sndbuf() to Error Markus Armbruster
@ 2015-05-15 11:58 ` Markus Armbruster
2015-05-19 12:52 ` Stefan Hajnoczi
2015-05-15 11:58 ` [Qemu-devel] [PATCH v2 09/15] tap: Convert launch_script() " Markus Armbruster
` (7 subsequent siblings)
15 siblings, 1 reply; 20+ messages in thread
From: Markus Armbruster @ 2015-05-15 11:58 UTC (permalink / raw)
To: qemu-devel; +Cc: eblake, stefanha
Signed-off-by: Markus Armbruster <armbru@redhat.com>
Reviewed-by: Eric Blake <eblake@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
^ permalink raw reply related [flat|nested] 20+ messages in thread* Re: [Qemu-devel] [PATCH v2 08/15] tap: Convert net_init_tap_one() to Error
2015-05-15 11:58 ` [Qemu-devel] [PATCH v2 08/15] tap: Convert net_init_tap_one() " Markus Armbruster
@ 2015-05-19 12:52 ` Stefan Hajnoczi
0 siblings, 0 replies; 20+ messages in thread
From: Stefan Hajnoczi @ 2015-05-19 12:52 UTC (permalink / raw)
To: Markus Armbruster; +Cc: eblake, qemu-devel, stefanha
[-- Attachment #1: Type: text/plain, Size: 1097 bytes --]
On Fri, May 15, 2015 at 01:58:56PM +0200, Markus Armbruster wrote:
> @@ -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;
%s must be removed since error_setg_errno() already prints the error
string and we have no format string argument.
If there are no other issues I'll do this when merging the patch.
[-- Attachment #2: Type: application/pgp-signature, Size: 473 bytes --]
^ permalink raw reply [flat|nested] 20+ messages in thread
* [Qemu-devel] [PATCH v2 09/15] tap: Convert launch_script() to Error
2015-05-15 11:58 [Qemu-devel] [PATCH v2 00/15] net: Improve error reporting Markus Armbruster
` (7 preceding siblings ...)
2015-05-15 11:58 ` [Qemu-devel] [PATCH v2 08/15] tap: Convert net_init_tap_one() " Markus Armbruster
@ 2015-05-15 11:58 ` Markus Armbruster
2015-05-15 11:58 ` [Qemu-devel] [PATCH v2 10/15] tap: Permit incremental conversion of tap_open() " Markus Armbruster
` (6 subsequent siblings)
15 siblings, 0 replies; 20+ messages in thread
From: Markus Armbruster @ 2015-05-15 11:58 UTC (permalink / raw)
To: qemu-devel; +Cc: eblake, stefanha
Fixes inappropriate use of stderr in monitor command handler.
While there, improve the messages some.
Signed-off-by: Markus Armbruster <armbru@redhat.com>
Reviewed-by: Eric Blake <eblake@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
^ permalink raw reply related [flat|nested] 20+ messages in thread* [Qemu-devel] [PATCH v2 10/15] tap: Permit incremental conversion of tap_open() to Error
2015-05-15 11:58 [Qemu-devel] [PATCH v2 00/15] net: Improve error reporting Markus Armbruster
` (8 preceding siblings ...)
2015-05-15 11:58 ` [Qemu-devel] [PATCH v2 09/15] tap: Convert launch_script() " Markus Armbruster
@ 2015-05-15 11:58 ` Markus Armbruster
2015-05-15 11:58 ` [Qemu-devel] [PATCH v2 11/15] tap-linux: Convert " Markus Armbruster
` (5 subsequent siblings)
15 siblings, 0 replies; 20+ messages in thread
From: Markus Armbruster @ 2015-05-15 11:58 UTC (permalink / raw)
To: qemu-devel; +Cc: eblake, stefanha
Convert the trivial ones immediately: tap-aix and tap-haiku.
Signed-off-by: Markus Armbruster <armbru@redhat.com>
Reviewed-by: Eric Blake <eblake@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
^ permalink raw reply related [flat|nested] 20+ messages in thread* [Qemu-devel] [PATCH v2 11/15] tap-linux: Convert tap_open() to Error
2015-05-15 11:58 [Qemu-devel] [PATCH v2 00/15] net: Improve error reporting Markus Armbruster
` (9 preceding siblings ...)
2015-05-15 11:58 ` [Qemu-devel] [PATCH v2 10/15] tap: Permit incremental conversion of tap_open() " Markus Armbruster
@ 2015-05-15 11:58 ` Markus Armbruster
2015-05-15 11:59 ` [Qemu-devel] [PATCH v2 12/15] tap-bsd: " Markus Armbruster
` (4 subsequent siblings)
15 siblings, 0 replies; 20+ messages in thread
From: Markus Armbruster @ 2015-05-15 11:58 UTC (permalink / raw)
To: qemu-devel; +Cc: eblake, stefanha
Signed-off-by: Markus Armbruster <armbru@redhat.com>
Reviewed-by: Eric Blake <eblake@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
^ permalink raw reply related [flat|nested] 20+ messages in thread* [Qemu-devel] [PATCH v2 12/15] tap-bsd: Convert tap_open() to Error
2015-05-15 11:58 [Qemu-devel] [PATCH v2 00/15] net: Improve error reporting Markus Armbruster
` (10 preceding siblings ...)
2015-05-15 11:58 ` [Qemu-devel] [PATCH v2 11/15] tap-linux: Convert " Markus Armbruster
@ 2015-05-15 11:59 ` Markus Armbruster
2015-05-15 14:36 ` Eric Blake
2015-05-15 11:59 ` [Qemu-devel] [PATCH v2 13/15] tap-solaris: " Markus Armbruster
` (3 subsequent siblings)
15 siblings, 1 reply; 20+ messages in thread
From: Markus Armbruster @ 2015-05-15 11:59 UTC (permalink / raw)
To: qemu-devel; +Cc: eblake, stefanha
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..5889920 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(errp, "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 no kernel support"
+ " for IFF_MULTI_QUEUE available");
goto error;
}
--
1.9.3
^ permalink raw reply related [flat|nested] 20+ messages in thread* [Qemu-devel] [PATCH v2 13/15] tap-solaris: Convert tap_open() to Error
2015-05-15 11:58 [Qemu-devel] [PATCH v2 00/15] net: Improve error reporting Markus Armbruster
` (11 preceding siblings ...)
2015-05-15 11:59 ` [Qemu-devel] [PATCH v2 12/15] tap-bsd: " Markus Armbruster
@ 2015-05-15 11:59 ` Markus Armbruster
2015-05-15 11:59 ` [Qemu-devel] [PATCH v2 14/15] tap: Finish conversion of " Markus Armbruster
` (2 subsequent siblings)
15 siblings, 0 replies; 20+ messages in thread
From: Markus Armbruster @ 2015-05-15 11:59 UTC (permalink / raw)
To: qemu-devel; +Cc: eblake, stefanha
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>
Reviewed-by: Eric Blake <eblake@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 *)𝔦
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
^ permalink raw reply related [flat|nested] 20+ messages in thread* [Qemu-devel] [PATCH v2 14/15] tap: Finish conversion of tap_open() to Error
2015-05-15 11:58 [Qemu-devel] [PATCH v2 00/15] net: Improve error reporting Markus Armbruster
` (12 preceding siblings ...)
2015-05-15 11:59 ` [Qemu-devel] [PATCH v2 13/15] tap-solaris: " Markus Armbruster
@ 2015-05-15 11:59 ` Markus Armbruster
2015-05-15 11:59 ` [Qemu-devel] [PATCH v2 15/15] tap: Improve -netdev/netdev_add/-net/... tap error reporting Markus Armbruster
2015-05-19 13:15 ` [Qemu-devel] [PATCH v2 00/15] net: Improve " Stefan Hajnoczi
15 siblings, 0 replies; 20+ messages in thread
From: Markus Armbruster @ 2015-05-15 11:59 UTC (permalink / raw)
To: qemu-devel; +Cc: eblake, stefanha
Signed-off-by: Markus Armbruster <armbru@redhat.com>
Reviewed-by: Eric Blake <eblake@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
^ permalink raw reply related [flat|nested] 20+ messages in thread* [Qemu-devel] [PATCH v2 15/15] tap: Improve -netdev/netdev_add/-net/... tap error reporting
2015-05-15 11:58 [Qemu-devel] [PATCH v2 00/15] net: Improve error reporting Markus Armbruster
` (13 preceding siblings ...)
2015-05-15 11:59 ` [Qemu-devel] [PATCH v2 14/15] tap: Finish conversion of " Markus Armbruster
@ 2015-05-15 11:59 ` Markus Armbruster
2015-05-15 14:38 ` Eric Blake
2015-05-19 13:15 ` [Qemu-devel] [PATCH v2 00/15] net: Improve " Stefan Hajnoczi
15 siblings, 1 reply; 20+ messages in thread
From: Markus Armbruster @ 2015-05-15 11:59 UTC (permalink / raw)
To: qemu-devel; +Cc: eblake, stefanha
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 makes the specific error the QMP error reply.
Signed-off-by: Markus Armbruster <armbru@redhat.com>
Reviewed-by: Eric Blake <eblake@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..319eac0 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
^ permalink raw reply related [flat|nested] 20+ messages in thread* Re: [Qemu-devel] [PATCH v2 15/15] tap: Improve -netdev/netdev_add/-net/... tap error reporting
2015-05-15 11:59 ` [Qemu-devel] [PATCH v2 15/15] tap: Improve -netdev/netdev_add/-net/... tap error reporting Markus Armbruster
@ 2015-05-15 14:38 ` Eric Blake
0 siblings, 0 replies; 20+ messages in thread
From: Eric Blake @ 2015-05-15 14:38 UTC (permalink / raw)
To: Markus Armbruster, qemu-devel; +Cc: stefanha
[-- Attachment #1: Type: text/plain, Size: 2106 bytes --]
On 05/15/2015 05:59 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 makes the specific error the QMP error reply.
>
> Signed-off-by: Markus Armbruster <armbru@redhat.com>
> Reviewed-by: Eric Blake <eblake@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=");
Still might be worth deleting the first 'and'; committer could do that
when preparing the pull request. But my R-b stands either way.
--
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 --]
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [Qemu-devel] [PATCH v2 00/15] net: Improve error reporting
2015-05-15 11:58 [Qemu-devel] [PATCH v2 00/15] net: Improve error reporting Markus Armbruster
` (14 preceding siblings ...)
2015-05-15 11:59 ` [Qemu-devel] [PATCH v2 15/15] tap: Improve -netdev/netdev_add/-net/... tap error reporting Markus Armbruster
@ 2015-05-19 13:15 ` Stefan Hajnoczi
15 siblings, 0 replies; 20+ messages in thread
From: Stefan Hajnoczi @ 2015-05-19 13:15 UTC (permalink / raw)
To: Markus Armbruster; +Cc: eblake, qemu-devel, stefanha
[-- Attachment #1: Type: text/plain, Size: 1901 bytes --]
On Fri, May 15, 2015 at 01:58:48PM +0200, Markus Armbruster wrote:
> v2:
> - Touch up a few commit messages [Eric]
> - Touch up a an error message in PATCH 03+15 [Eric]
> - Don't report possibly bogus errno in PATCH 12 [Eric]
>
> 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
>
>
Thanks, applied to my net tree:
https://github.com/stefanha/qemu/commits/net
Stefan
[-- Attachment #2: Type: application/pgp-signature, Size: 473 bytes --]
^ permalink raw reply [flat|nested] 20+ messages in thread