* [Qemu-devel] [PATCH v5 1/4] net/socket: Don't treat odd socket type as SOCK_STREAM
2017-06-27 3:24 [Qemu-devel] [PATCH v5 0/4] Improve error reporting Mao Zhongyi
@ 2017-06-27 3:24 ` Mao Zhongyi
2017-06-27 3:24 ` [Qemu-devel] [PATCH v5 2/4] net/socket: Convert error message to Error Mao Zhongyi
` (2 subsequent siblings)
3 siblings, 0 replies; 13+ messages in thread
From: Mao Zhongyi @ 2017-06-27 3:24 UTC (permalink / raw)
To: qemu-devel; +Cc: jasowang, armbru, berrange
In net_socket_fd_init(), the 'default' case is odd: it warns,
then continues as if the socket type was SOCK_STREAM. The
comment explains "this could be a eg. a pty", but that makes
no sense. If @fd really was a pty, getsockopt() would fail
with ENOTSOCK. If @fd was a socket, but neither SOCK_DGRAM nor
SOCK_STREAM. It should not be treated as if it was SOCK_STREAM.
Turn this case into an Error. If there is a genuine reason to
support something like SOCK_RAW, it should be explicitly
handled.
Cc: jasowang@redhat.com
Cc: armbru@redhat.com
Cc: berrange@redhat.com
Cc: armbru@redhat.com
Suggested-by: Markus Armbruster <armbru@redhat.com>
Suggested-by: Daniel P. Berrange <berrange@redhat.com>
Signed-off-by: Mao Zhongyi <maozy.fnst@cn.fujitsu.com>
Reviewed-by: Markus Armbruster <armbru@redhat.com>
---
net/socket.c | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/net/socket.c b/net/socket.c
index dcae1ae..53765bd 100644
--- a/net/socket.c
+++ b/net/socket.c
@@ -449,9 +449,9 @@ static NetSocketState *net_socket_fd_init(NetClientState *peer,
case SOCK_STREAM:
return net_socket_fd_init_stream(peer, model, name, fd, is_connected);
default:
- /* who knows ... this could be a eg. a pty, do warn and continue as stream */
- fprintf(stderr, "qemu: warning: socket type=%d for fd=%d is not SOCK_DGRAM or SOCK_STREAM\n", so_type, fd);
- return net_socket_fd_init_stream(peer, model, name, fd, is_connected);
+ error_report("qemu: error: socket type=%d for fd=%d is not"
+ " SOCK_DGRAM or SOCK_STREAM", so_type, fd);
+ closesocket(fd);
}
return NULL;
}
--
2.9.4
^ permalink raw reply related [flat|nested] 13+ messages in thread* [Qemu-devel] [PATCH v5 2/4] net/socket: Convert error message to Error
2017-06-27 3:24 [Qemu-devel] [PATCH v5 0/4] Improve error reporting Mao Zhongyi
2017-06-27 3:24 ` [Qemu-devel] [PATCH v5 1/4] net/socket: Don't treat odd socket type as SOCK_STREAM Mao Zhongyi
@ 2017-06-27 3:24 ` Mao Zhongyi
2017-06-27 7:43 ` Markus Armbruster
2017-06-27 3:24 ` [Qemu-devel] [PATCH v5 3/4] net/net: Convert parse_host_port() " Mao Zhongyi
2017-06-27 3:24 ` [Qemu-devel] [PATCH v5 4/4] net/socket: Improve -net socket error reporting Mao Zhongyi
3 siblings, 1 reply; 13+ messages in thread
From: Mao Zhongyi @ 2017-06-27 3:24 UTC (permalink / raw)
To: qemu-devel; +Cc: jasowang, armbru
Currently, net_socket_mcast_create(), net_socket_fd_init_dgram() and
net_socket_fd_init() use the function such as fprintf(), perror() to
report an error message.
Now, convert these functions to Error.
Cc: jasowang@redhat.com
Cc: armbru@redhat.com
Signed-off-by: Mao Zhongyi <maozy.fnst@cn.fujitsu.com>
---
net/socket.c | 76 ++++++++++++++++++++++++++++++++++++++----------------------
1 file changed, 48 insertions(+), 28 deletions(-)
diff --git a/net/socket.c b/net/socket.c
index 53765bd..e136f87 100644
--- a/net/socket.c
+++ b/net/socket.c
@@ -209,7 +209,9 @@ static void net_socket_send_dgram(void *opaque)
}
}
-static int net_socket_mcast_create(struct sockaddr_in *mcastaddr, struct in_addr *localaddr)
+static int net_socket_mcast_create(struct sockaddr_in *mcastaddr,
+ struct in_addr *localaddr,
+ Error **errp)
{
struct ip_mreq imr;
int fd;
@@ -221,8 +223,8 @@ static int net_socket_mcast_create(struct sockaddr_in *mcastaddr, struct in_addr
#endif
if (!IN_MULTICAST(ntohl(mcastaddr->sin_addr.s_addr))) {
- fprintf(stderr, "qemu: error: specified mcastaddr \"%s\" (0x%08x) "
- "does not contain a multicast address\n",
+ error_setg(errp, "qemu: error: specified mcastaddr %s (0x%08x) "
+ "does not contain a multicast address",
inet_ntoa(mcastaddr->sin_addr),
(int)ntohl(mcastaddr->sin_addr.s_addr));
return -1;
@@ -230,7 +232,8 @@ static int net_socket_mcast_create(struct sockaddr_in *mcastaddr, struct in_addr
}
fd = qemu_socket(PF_INET, SOCK_DGRAM, 0);
if (fd < 0) {
- perror("socket(PF_INET, SOCK_DGRAM)");
+ error_setg_errno(errp, errno, "Create socket(PF_INET, SOCK_DGRAM)"
+ " failed");
return -1;
}
@@ -242,13 +245,15 @@ static int net_socket_mcast_create(struct sockaddr_in *mcastaddr, struct in_addr
val = 1;
ret = qemu_setsockopt(fd, SOL_SOCKET, SO_REUSEADDR, &val, sizeof(val));
if (ret < 0) {
- perror("setsockopt(SOL_SOCKET, SO_REUSEADDR)");
+ error_setg_errno(errp, errno, "Set the socket fd=%d attribute"
+ " SO_REUSEADDR failed", fd);
goto fail;
}
ret = bind(fd, (struct sockaddr *)mcastaddr, sizeof(*mcastaddr));
if (ret < 0) {
- perror("bind");
+ error_setg_errno(errp, errno, "Bind ip=%s to fd=%d failed",
+ inet_ntoa(mcastaddr->sin_addr), fd);
goto fail;
}
@@ -263,7 +268,8 @@ static int net_socket_mcast_create(struct sockaddr_in *mcastaddr, struct in_addr
ret = qemu_setsockopt(fd, IPPROTO_IP, IP_ADD_MEMBERSHIP,
&imr, sizeof(struct ip_mreq));
if (ret < 0) {
- perror("setsockopt(IP_ADD_MEMBERSHIP)");
+ error_setg_errno(errp, errno, "Add socket fd=%d to multicast group %s"
+ " failed", fd, inet_ntoa(imr.imr_multiaddr));
goto fail;
}
@@ -272,7 +278,8 @@ static int net_socket_mcast_create(struct sockaddr_in *mcastaddr, struct in_addr
ret = qemu_setsockopt(fd, IPPROTO_IP, IP_MULTICAST_LOOP,
&loop, sizeof(loop));
if (ret < 0) {
- perror("setsockopt(SOL_IP, IP_MULTICAST_LOOP)");
+ error_setg_errno(errp, errno, "Force multicast message to loopback"
+ " failed");
goto fail;
}
@@ -281,7 +288,8 @@ static int net_socket_mcast_create(struct sockaddr_in *mcastaddr, struct in_addr
ret = qemu_setsockopt(fd, IPPROTO_IP, IP_MULTICAST_IF,
localaddr, sizeof(*localaddr));
if (ret < 0) {
- perror("setsockopt(IP_MULTICAST_IF)");
+ error_setg_errno(errp, errno, "Set the default network send "
+ "interfaec failed");
goto fail;
}
}
@@ -320,7 +328,8 @@ static NetClientInfo net_dgram_socket_info = {
static NetSocketState *net_socket_fd_init_dgram(NetClientState *peer,
const char *model,
const char *name,
- int fd, int is_connected)
+ int fd, int is_connected,
+ Error **errp)
{
struct sockaddr_in saddr;
int newfd;
@@ -337,14 +346,13 @@ static NetSocketState *net_socket_fd_init_dgram(NetClientState *peer,
if (getsockname(fd, (struct sockaddr *) &saddr, &saddr_len) == 0) {
/* must be bound */
if (saddr.sin_addr.s_addr == 0) {
- fprintf(stderr, "qemu: error: init_dgram: fd=%d unbound, "
- "cannot setup multicast dst addr\n", fd);
+ error_setg(errp, "qemu: error: init_dgram: fd=%d unbound, "
+ "cannot setup multicast dst addr", fd);
goto err;
}
/* clone dgram socket */
- newfd = net_socket_mcast_create(&saddr, NULL);
+ newfd = net_socket_mcast_create(&saddr, NULL, errp);
if (newfd < 0) {
- /* error already reported by net_socket_mcast_create() */
goto err;
}
/* clone newfd to fd, close newfd */
@@ -352,8 +360,8 @@ static NetSocketState *net_socket_fd_init_dgram(NetClientState *peer,
close(newfd);
} else {
- fprintf(stderr,
- "qemu: error: init_dgram: fd=%d failed getsockname(): %s\n",
+ error_setg(errp,
+ "qemu: error: init_dgram: fd=%d failed getsockname(): %s",
fd, strerror(errno));
goto err;
}
@@ -432,24 +440,26 @@ static NetSocketState *net_socket_fd_init_stream(NetClientState *peer,
static NetSocketState *net_socket_fd_init(NetClientState *peer,
const char *model, const char *name,
- int fd, int is_connected)
+ int fd, int is_connected,
+ Error **errp)
{
int so_type = -1, optlen=sizeof(so_type);
if(getsockopt(fd, SOL_SOCKET, SO_TYPE, (char *)&so_type,
(socklen_t *)&optlen)< 0) {
- fprintf(stderr, "qemu: error: getsockopt(SO_TYPE) for fd=%d failed\n",
+ error_setg(errp, "qemu: error: getsockopt(SO_TYPE) for fd=%d failed",
fd);
closesocket(fd);
return NULL;
}
switch(so_type) {
case SOCK_DGRAM:
- return net_socket_fd_init_dgram(peer, model, name, fd, is_connected);
+ return net_socket_fd_init_dgram(peer, model, name, fd, is_connected,
+ errp);
case SOCK_STREAM:
return net_socket_fd_init_stream(peer, model, name, fd, is_connected);
default:
- error_report("qemu: error: socket type=%d for fd=%d is not"
+ error_setg(errp, "qemu: error: socket type=%d for fd=%d is not"
" SOCK_DGRAM or SOCK_STREAM", so_type, fd);
closesocket(fd);
}
@@ -536,6 +546,7 @@ static int net_socket_connect_init(NetClientState *peer,
NetSocketState *s;
int fd, connected, ret;
struct sockaddr_in saddr;
+ Error *err = NULL;
if (parse_host_port(&saddr, host_str) < 0)
return -1;
@@ -567,9 +578,11 @@ static int net_socket_connect_init(NetClientState *peer,
break;
}
}
- s = net_socket_fd_init(peer, model, name, fd, connected);
- if (!s)
+ s = net_socket_fd_init(peer, model, name, fd, connected, &err);
+ if (!s) {
+ error_report_err(err);
return -1;
+ }
snprintf(s->nc.info_str, sizeof(s->nc.info_str),
"socket: connect to %s:%d",
inet_ntoa(saddr.sin_addr), ntohs(saddr.sin_port));
@@ -586,6 +599,7 @@ static int net_socket_mcast_init(NetClientState *peer,
int fd;
struct sockaddr_in saddr;
struct in_addr localaddr, *param_localaddr;
+ Error *err = NULL;
if (parse_host_port(&saddr, host_str) < 0)
return -1;
@@ -598,13 +612,17 @@ static int net_socket_mcast_init(NetClientState *peer,
param_localaddr = NULL;
}
- fd = net_socket_mcast_create(&saddr, param_localaddr);
- if (fd < 0)
+ fd = net_socket_mcast_create(&saddr, param_localaddr, &err);
+ if (fd < 0) {
+ error_report_err(err);
return -1;
+ }
- s = net_socket_fd_init(peer, model, name, fd, 0);
- if (!s)
+ s = net_socket_fd_init(peer, model, name, fd, 0, &err);
+ if (!s) {
+ error_report_err(err);
return -1;
+ }
s->dgram_dst = saddr;
@@ -624,6 +642,7 @@ static int net_socket_udp_init(NetClientState *peer,
NetSocketState *s;
int fd, ret;
struct sockaddr_in laddr, raddr;
+ Error *err = NULL;
if (parse_host_port(&laddr, lhost) < 0) {
return -1;
@@ -652,8 +671,9 @@ static int net_socket_udp_init(NetClientState *peer,
}
qemu_set_nonblock(fd);
- s = net_socket_fd_init(peer, model, name, fd, 0);
+ s = net_socket_fd_init(peer, model, name, fd, 0, &err);
if (!s) {
+ error_report_err(err);
return -1;
}
@@ -696,7 +716,7 @@ int net_init_socket(const Netdev *netdev, const char *name,
return -1;
}
qemu_set_nonblock(fd);
- if (!net_socket_fd_init(peer, "socket", name, fd, 1)) {
+ if (!net_socket_fd_init(peer, "socket", name, fd, 1, errp)) {
return -1;
}
return 0;
--
2.9.4
^ permalink raw reply related [flat|nested] 13+ messages in thread* Re: [Qemu-devel] [PATCH v5 2/4] net/socket: Convert error message to Error
2017-06-27 3:24 ` [Qemu-devel] [PATCH v5 2/4] net/socket: Convert error message to Error Mao Zhongyi
@ 2017-06-27 7:43 ` Markus Armbruster
2017-06-28 4:07 ` Mao Zhongyi
0 siblings, 1 reply; 13+ messages in thread
From: Markus Armbruster @ 2017-06-27 7:43 UTC (permalink / raw)
To: Mao Zhongyi; +Cc: qemu-devel, jasowang
Suggest "net/socket: Convert several helper functions to Error".
Mao Zhongyi <maozy.fnst@cn.fujitsu.com> writes:
> Currently, net_socket_mcast_create(), net_socket_fd_init_dgram() and
> net_socket_fd_init() use the function such as fprintf(), perror() to
> report an error message.
>
> Now, convert these functions to Error.
>
> Cc: jasowang@redhat.com
> Cc: armbru@redhat.com
> Signed-off-by: Mao Zhongyi <maozy.fnst@cn.fujitsu.com>
> ---
> net/socket.c | 76 ++++++++++++++++++++++++++++++++++++++----------------------
> 1 file changed, 48 insertions(+), 28 deletions(-)
>
> diff --git a/net/socket.c b/net/socket.c
> index 53765bd..e136f87 100644
> --- a/net/socket.c
> +++ b/net/socket.c
> @@ -209,7 +209,9 @@ static void net_socket_send_dgram(void *opaque)
> }
> }
>
> -static int net_socket_mcast_create(struct sockaddr_in *mcastaddr, struct in_addr *localaddr)
> +static int net_socket_mcast_create(struct sockaddr_in *mcastaddr,
> + struct in_addr *localaddr,
> + Error **errp)
> {
> struct ip_mreq imr;
> int fd;
> @@ -221,8 +223,8 @@ static int net_socket_mcast_create(struct sockaddr_in *mcastaddr, struct in_addr
> #endif
>
> if (!IN_MULTICAST(ntohl(mcastaddr->sin_addr.s_addr))) {
> - fprintf(stderr, "qemu: error: specified mcastaddr \"%s\" (0x%08x) "
> - "does not contain a multicast address\n",
> + error_setg(errp, "qemu: error: specified mcastaddr %s (0x%08x) "
> + "does not contain a multicast address",
Please drop the "qemu: error: " prefix. More of the same below.
> inet_ntoa(mcastaddr->sin_addr),
> (int)ntohl(mcastaddr->sin_addr.s_addr));
> return -1;
> @@ -230,7 +232,8 @@ static int net_socket_mcast_create(struct sockaddr_in *mcastaddr, struct in_addr
> }
> fd = qemu_socket(PF_INET, SOCK_DGRAM, 0);
> if (fd < 0) {
> - perror("socket(PF_INET, SOCK_DGRAM)");
> + error_setg_errno(errp, errno, "Create socket(PF_INET, SOCK_DGRAM)"
> + " failed");
I'm not a native speaker, but "Create FOO failed" doesn't feel right to
me. Where's the subject? "Create" is a verb. "Creation of FOO failed"
has a subject, but doesn't feel right. What about "can't create
datagram socket"?
Same for many of the other new error messages.
> return -1;
> }
>
> @@ -242,13 +245,15 @@ static int net_socket_mcast_create(struct sockaddr_in *mcastaddr, struct in_addr
> val = 1;
> ret = qemu_setsockopt(fd, SOL_SOCKET, SO_REUSEADDR, &val, sizeof(val));
> if (ret < 0) {
> - perror("setsockopt(SOL_SOCKET, SO_REUSEADDR)");
> + error_setg_errno(errp, errno, "Set the socket fd=%d attribute"
> + " SO_REUSEADDR failed", fd);
Why would the user want to know the value of @fd here?
Same for many of the other new error messages.
> goto fail;
> }
>
> ret = bind(fd, (struct sockaddr *)mcastaddr, sizeof(*mcastaddr));
> if (ret < 0) {
> - perror("bind");
> + error_setg_errno(errp, errno, "Bind ip=%s to fd=%d failed",
> + inet_ntoa(mcastaddr->sin_addr), fd);
> goto fail;
> }
>
> @@ -263,7 +268,8 @@ static int net_socket_mcast_create(struct sockaddr_in *mcastaddr, struct in_addr
> ret = qemu_setsockopt(fd, IPPROTO_IP, IP_ADD_MEMBERSHIP,
> &imr, sizeof(struct ip_mreq));
> if (ret < 0) {
> - perror("setsockopt(IP_ADD_MEMBERSHIP)");
> + error_setg_errno(errp, errno, "Add socket fd=%d to multicast group %s"
> + " failed", fd, inet_ntoa(imr.imr_multiaddr));
> goto fail;
> }
>
> @@ -272,7 +278,8 @@ static int net_socket_mcast_create(struct sockaddr_in *mcastaddr, struct in_addr
> ret = qemu_setsockopt(fd, IPPROTO_IP, IP_MULTICAST_LOOP,
> &loop, sizeof(loop));
> if (ret < 0) {
> - perror("setsockopt(SOL_IP, IP_MULTICAST_LOOP)");
> + error_setg_errno(errp, errno, "Force multicast message to loopback"
> + " failed");
> goto fail;
> }
>
> @@ -281,7 +288,8 @@ static int net_socket_mcast_create(struct sockaddr_in *mcastaddr, struct in_addr
> ret = qemu_setsockopt(fd, IPPROTO_IP, IP_MULTICAST_IF,
> localaddr, sizeof(*localaddr));
> if (ret < 0) {
> - perror("setsockopt(IP_MULTICAST_IF)");
> + error_setg_errno(errp, errno, "Set the default network send "
> + "interfaec failed");
s/interfaec/interface/
> goto fail;
> }
> }
> @@ -320,7 +328,8 @@ static NetClientInfo net_dgram_socket_info = {
> static NetSocketState *net_socket_fd_init_dgram(NetClientState *peer,
> const char *model,
> const char *name,
> - int fd, int is_connected)
> + int fd, int is_connected,
> + Error **errp)
> {
> struct sockaddr_in saddr;
> int newfd;
> @@ -337,14 +346,13 @@ static NetSocketState *net_socket_fd_init_dgram(NetClientState *peer,
> if (getsockname(fd, (struct sockaddr *) &saddr, &saddr_len) == 0) {
> /* must be bound */
> if (saddr.sin_addr.s_addr == 0) {
> - fprintf(stderr, "qemu: error: init_dgram: fd=%d unbound, "
> - "cannot setup multicast dst addr\n", fd);
> + error_setg(errp, "qemu: error: init_dgram: fd=%d unbound, "
> + "cannot setup multicast dst addr", fd);
> goto err;
> }
> /* clone dgram socket */
> - newfd = net_socket_mcast_create(&saddr, NULL);
> + newfd = net_socket_mcast_create(&saddr, NULL, errp);
> if (newfd < 0) {
> - /* error already reported by net_socket_mcast_create() */
> goto err;
> }
> /* clone newfd to fd, close newfd */
> @@ -352,8 +360,8 @@ static NetSocketState *net_socket_fd_init_dgram(NetClientState *peer,
> close(newfd);
>
> } else {
> - fprintf(stderr,
> - "qemu: error: init_dgram: fd=%d failed getsockname(): %s\n",
> + error_setg(errp,
> + "qemu: error: init_dgram: fd=%d failed getsockname(): %s",
> fd, strerror(errno));
> goto err;
> }
> @@ -432,24 +440,26 @@ static NetSocketState *net_socket_fd_init_stream(NetClientState *peer,
>
> static NetSocketState *net_socket_fd_init(NetClientState *peer,
> const char *model, const char *name,
> - int fd, int is_connected)
> + int fd, int is_connected,
> + Error **errp)
> {
> int so_type = -1, optlen=sizeof(so_type);
>
> if(getsockopt(fd, SOL_SOCKET, SO_TYPE, (char *)&so_type,
> (socklen_t *)&optlen)< 0) {
> - fprintf(stderr, "qemu: error: getsockopt(SO_TYPE) for fd=%d failed\n",
> + error_setg(errp, "qemu: error: getsockopt(SO_TYPE) for fd=%d failed",
> fd);
> closesocket(fd);
> return NULL;
> }
> switch(so_type) {
> case SOCK_DGRAM:
> - return net_socket_fd_init_dgram(peer, model, name, fd, is_connected);
> + return net_socket_fd_init_dgram(peer, model, name, fd, is_connected,
> + errp);
> case SOCK_STREAM:
> return net_socket_fd_init_stream(peer, model, name, fd, is_connected);
> default:
> - error_report("qemu: error: socket type=%d for fd=%d is not"
> + error_setg(errp, "qemu: error: socket type=%d for fd=%d is not"
> " SOCK_DGRAM or SOCK_STREAM", so_type, fd);
> closesocket(fd);
> }
> @@ -536,6 +546,7 @@ static int net_socket_connect_init(NetClientState *peer,
> NetSocketState *s;
> int fd, connected, ret;
> struct sockaddr_in saddr;
> + Error *err = NULL;
>
> if (parse_host_port(&saddr, host_str) < 0)
> return -1;
> @@ -567,9 +578,11 @@ static int net_socket_connect_init(NetClientState *peer,
> break;
> }
> }
> - s = net_socket_fd_init(peer, model, name, fd, connected);
> - if (!s)
> + s = net_socket_fd_init(peer, model, name, fd, connected, &err);
> + if (!s) {
> + error_report_err(err);
> return -1;
> + }
> snprintf(s->nc.info_str, sizeof(s->nc.info_str),
> "socket: connect to %s:%d",
> inet_ntoa(saddr.sin_addr), ntohs(saddr.sin_port));
> @@ -586,6 +599,7 @@ static int net_socket_mcast_init(NetClientState *peer,
> int fd;
> struct sockaddr_in saddr;
> struct in_addr localaddr, *param_localaddr;
> + Error *err = NULL;
>
> if (parse_host_port(&saddr, host_str) < 0)
> return -1;
> @@ -598,13 +612,17 @@ static int net_socket_mcast_init(NetClientState *peer,
> param_localaddr = NULL;
> }
>
> - fd = net_socket_mcast_create(&saddr, param_localaddr);
> - if (fd < 0)
> + fd = net_socket_mcast_create(&saddr, param_localaddr, &err);
> + if (fd < 0) {
> + error_report_err(err);
> return -1;
> + }
>
> - s = net_socket_fd_init(peer, model, name, fd, 0);
> - if (!s)
> + s = net_socket_fd_init(peer, model, name, fd, 0, &err);
> + if (!s) {
> + error_report_err(err);
> return -1;
> + }
>
> s->dgram_dst = saddr;
>
> @@ -624,6 +642,7 @@ static int net_socket_udp_init(NetClientState *peer,
> NetSocketState *s;
> int fd, ret;
> struct sockaddr_in laddr, raddr;
> + Error *err = NULL;
>
> if (parse_host_port(&laddr, lhost) < 0) {
> return -1;
> @@ -652,8 +671,9 @@ static int net_socket_udp_init(NetClientState *peer,
> }
> qemu_set_nonblock(fd);
>
> - s = net_socket_fd_init(peer, model, name, fd, 0);
> + s = net_socket_fd_init(peer, model, name, fd, 0, &err);
> if (!s) {
> + error_report_err(err);
> return -1;
> }
>
> @@ -696,7 +716,7 @@ int net_init_socket(const Netdev *netdev, const char *name,
> return -1;
> }
> qemu_set_nonblock(fd);
> - if (!net_socket_fd_init(peer, "socket", name, fd, 1)) {
> + if (!net_socket_fd_init(peer, "socket", name, fd, 1, errp)) {
> return -1;
> }
> return 0;
Looks good to me otherwise.
^ permalink raw reply [flat|nested] 13+ messages in thread* Re: [Qemu-devel] [PATCH v5 2/4] net/socket: Convert error message to Error
2017-06-27 7:43 ` Markus Armbruster
@ 2017-06-28 4:07 ` Mao Zhongyi
0 siblings, 0 replies; 13+ messages in thread
From: Mao Zhongyi @ 2017-06-28 4:07 UTC (permalink / raw)
To: Markus Armbruster; +Cc: qemu-devel, jasowang
Hi, Markus
On 06/27/2017 03:43 PM, Markus Armbruster wrote:
> Suggest "net/socket: Convert several helper functions to Error".
OK, I see.
>
> Mao Zhongyi <maozy.fnst@cn.fujitsu.com> writes:
>
>> Currently, net_socket_mcast_create(), net_socket_fd_init_dgram() and
>> net_socket_fd_init() use the function such as fprintf(), perror() to
>> report an error message.
>>
>> Now, convert these functions to Error.
>>
>> Cc: jasowang@redhat.com
>> Cc: armbru@redhat.com
>> Signed-off-by: Mao Zhongyi <maozy.fnst@cn.fujitsu.com>
>> ---
>> net/socket.c | 76 ++++++++++++++++++++++++++++++++++++++----------------------
>> 1 file changed, 48 insertions(+), 28 deletions(-)
>>
>> diff --git a/net/socket.c b/net/socket.c
>> index 53765bd..e136f87 100644
>> --- a/net/socket.c
>> +++ b/net/socket.c
>> @@ -209,7 +209,9 @@ static void net_socket_send_dgram(void *opaque)
>> }
>> }
>>
>> -static int net_socket_mcast_create(struct sockaddr_in *mcastaddr, struct in_addr *localaddr)
>> +static int net_socket_mcast_create(struct sockaddr_in *mcastaddr,
>> + struct in_addr *localaddr,
>> + Error **errp)
>> {
>> struct ip_mreq imr;
>> int fd;
>> @@ -221,8 +223,8 @@ static int net_socket_mcast_create(struct sockaddr_in *mcastaddr, struct in_addr
>> #endif
>>
>> if (!IN_MULTICAST(ntohl(mcastaddr->sin_addr.s_addr))) {
>> - fprintf(stderr, "qemu: error: specified mcastaddr \"%s\" (0x%08x) "
>> - "does not contain a multicast address\n",
>> + error_setg(errp, "qemu: error: specified mcastaddr %s (0x%08x) "
>> + "does not contain a multicast address",
>
> Please drop the "qemu: error: " prefix. More of the same below.
OK, I will
>
>> inet_ntoa(mcastaddr->sin_addr),
>> (int)ntohl(mcastaddr->sin_addr.s_addr));
>> return -1;
>> @@ -230,7 +232,8 @@ static int net_socket_mcast_create(struct sockaddr_in *mcastaddr, struct in_addr
>> }
>> fd = qemu_socket(PF_INET, SOCK_DGRAM, 0);
>> if (fd < 0) {
>> - perror("socket(PF_INET, SOCK_DGRAM)");
>> + error_setg_errno(errp, errno, ")"
>> + " failed");
>
> I'm not a native speaker, but "Create FOO failed" doesn't feel right to
> me. Where's the subject? "Create" is a verb. "Creation of FOO failed"
> has a subject, but doesn't feel right. What about "can't create
> datagram socket"?
>
Thanks for your sharing.
My English is really poor, I will continue to improve.
> Same for many of the other new error messages.
>
>> return -1;
>> }
>>
>> @@ -242,13 +245,15 @@ static int net_socket_mcast_create(struct sockaddr_in *mcastaddr, struct in_addr
>> val = 1;
>> ret = qemu_setsockopt(fd, SOL_SOCKET, SO_REUSEADDR, &val, sizeof(val));
>> if (ret < 0) {
>> - perror("setsockopt(SOL_SOCKET, SO_REUSEADDR)");
>> + error_setg_errno(errp, errno, "Set the socket fd=%d attribute"
>> + " SO_REUSEADDR failed", fd);
>
> Why would the user want to know the value of @fd here?
>
> Same for many of the other new error messages.
It really doesn't make sense. I will drop it.
>
>> goto fail;
>> }
>>
>> ret = bind(fd, (struct sockaddr *)mcastaddr, sizeof(*mcastaddr));
>> if (ret < 0) {
>> - perror("bind");
>> + error_setg_errno(errp, errno, "Bind ip=%s to fd=%d failed",
>> + inet_ntoa(mcastaddr->sin_addr), fd);
>> goto fail;
>> }
>>
>> @@ -263,7 +268,8 @@ static int net_socket_mcast_create(struct sockaddr_in *mcastaddr, struct in_addr
>> ret = qemu_setsockopt(fd, IPPROTO_IP, IP_ADD_MEMBERSHIP,
>> &imr, sizeof(struct ip_mreq));
>> if (ret < 0) {
>> - perror("setsockopt(IP_ADD_MEMBERSHIP)");
>> + error_setg_errno(errp, errno, "Add socket fd=%d to multicast group %s"
>> + " failed", fd, inet_ntoa(imr.imr_multiaddr));
>> goto fail;
>> }
>>
>> @@ -272,7 +278,8 @@ static int net_socket_mcast_create(struct sockaddr_in *mcastaddr, struct in_addr
>> ret = qemu_setsockopt(fd, IPPROTO_IP, IP_MULTICAST_LOOP,
>> &loop, sizeof(loop));
>> if (ret < 0) {
>> - perror("setsockopt(SOL_IP, IP_MULTICAST_LOOP)");
>> + error_setg_errno(errp, errno, "Force multicast message to loopback"
>> + " failed");
>> goto fail;
>> }
>>
>> @@ -281,7 +288,8 @@ static int net_socket_mcast_create(struct sockaddr_in *mcastaddr, struct in_addr
>> ret = qemu_setsockopt(fd, IPPROTO_IP, IP_MULTICAST_IF,
>> localaddr, sizeof(*localaddr));
>> if (ret < 0) {
>> - perror("setsockopt(IP_MULTICAST_IF)");
>> + error_setg_errno(errp, errno, "Set the default network send "
>> + "interfaec failed");
>
> s/interfaec/interface/
OK, I see.
Thanks,
Mao
>
>> goto fail;
>> }
>> }
>> @@ -320,7 +328,8 @@ static NetClientInfo net_dgram_socket_info = {
>> static NetSocketState *net_socket_fd_init_dgram(NetClientState *peer,
>> const char *model,
>> const char *name,
>> - int fd, int is_connected)
>> + int fd, int is_connected,
>> + Error **errp)
>> {
>> struct sockaddr_in saddr;
>> int newfd;
>> @@ -337,14 +346,13 @@ static NetSocketState *net_socket_fd_init_dgram(NetClientState *peer,
>> if (getsockname(fd, (struct sockaddr *) &saddr, &saddr_len) == 0) {
>> /* must be bound */
>> if (saddr.sin_addr.s_addr == 0) {
>> - fprintf(stderr, "qemu: error: init_dgram: fd=%d unbound, "
>> - "cannot setup multicast dst addr\n", fd);
>> + error_setg(errp, "qemu: error: init_dgram: fd=%d unbound, "
>> + "cannot setup multicast dst addr", fd);
>> goto err;
>> }
>> /* clone dgram socket */
>> - newfd = net_socket_mcast_create(&saddr, NULL);
>> + newfd = net_socket_mcast_create(&saddr, NULL, errp);
>> if (newfd < 0) {
>> - /* error already reported by net_socket_mcast_create() */
>> goto err;
>> }
>> /* clone newfd to fd, close newfd */
>> @@ -352,8 +360,8 @@ static NetSocketState *net_socket_fd_init_dgram(NetClientState *peer,
>> close(newfd);
>>
>> } else {
>> - fprintf(stderr,
>> - "qemu: error: init_dgram: fd=%d failed getsockname(): %s\n",
>> + error_setg(errp,
>> + "qemu: error: init_dgram: fd=%d failed getsockname(): %s",
>> fd, strerror(errno));
>> goto err;
>> }
>> @@ -432,24 +440,26 @@ static NetSocketState *net_socket_fd_init_stream(NetClientState *peer,
>>
>> static NetSocketState *net_socket_fd_init(NetClientState *peer,
>> const char *model, const char *name,
>> - int fd, int is_connected)
>> + int fd, int is_connected,
>> + Error **errp)
>> {
>> int so_type = -1, optlen=sizeof(so_type);
>>
>> if(getsockopt(fd, SOL_SOCKET, SO_TYPE, (char *)&so_type,
>> (socklen_t *)&optlen)< 0) {
>> - fprintf(stderr, "qemu: error: getsockopt(SO_TYPE) for fd=%d failed\n",
>> + error_setg(errp, "qemu: error: getsockopt(SO_TYPE) for fd=%d failed",
>> fd);
>> closesocket(fd);
>> return NULL;
>> }
>> switch(so_type) {
>> case SOCK_DGRAM:
>> - return net_socket_fd_init_dgram(peer, model, name, fd, is_connected);
>> + return net_socket_fd_init_dgram(peer, model, name, fd, is_connected,
>> + errp);
>> case SOCK_STREAM:
>> return net_socket_fd_init_stream(peer, model, name, fd, is_connected);
>> default:
>> - error_report("qemu: error: socket type=%d for fd=%d is not"
>> + error_setg(errp, "qemu: error: socket type=%d for fd=%d is not"
>> " SOCK_DGRAM or SOCK_STREAM", so_type, fd);
>> closesocket(fd);
>> }
>> @@ -536,6 +546,7 @@ static int net_socket_connect_init(NetClientState *peer,
>> NetSocketState *s;
>> int fd, connected, ret;
>> struct sockaddr_in saddr;
>> + Error *err = NULL;
>>
>> if (parse_host_port(&saddr, host_str) < 0)
>> return -1;
>> @@ -567,9 +578,11 @@ static int net_socket_connect_init(NetClientState *peer,
>> break;
>> }
>> }
>> - s = net_socket_fd_init(peer, model, name, fd, connected);
>> - if (!s)
>> + s = net_socket_fd_init(peer, model, name, fd, connected, &err);
>> + if (!s) {
>> + error_report_err(err);
>> return -1;
>> + }
>> snprintf(s->nc.info_str, sizeof(s->nc.info_str),
>> "socket: connect to %s:%d",
>> inet_ntoa(saddr.sin_addr), ntohs(saddr.sin_port));
>> @@ -586,6 +599,7 @@ static int net_socket_mcast_init(NetClientState *peer,
>> int fd;
>> struct sockaddr_in saddr;
>> struct in_addr localaddr, *param_localaddr;
>> + Error *err = NULL;
>>
>> if (parse_host_port(&saddr, host_str) < 0)
>> return -1;
>> @@ -598,13 +612,17 @@ static int net_socket_mcast_init(NetClientState *peer,
>> param_localaddr = NULL;
>> }
>>
>> - fd = net_socket_mcast_create(&saddr, param_localaddr);
>> - if (fd < 0)
>> + fd = net_socket_mcast_create(&saddr, param_localaddr, &err);
>> + if (fd < 0) {
>> + error_report_err(err);
>> return -1;
>> + }
>>
>> - s = net_socket_fd_init(peer, model, name, fd, 0);
>> - if (!s)
>> + s = net_socket_fd_init(peer, model, name, fd, 0, &err);
>> + if (!s) {
>> + error_report_err(err);
>> return -1;
>> + }
>>
>> s->dgram_dst = saddr;
>>
>> @@ -624,6 +642,7 @@ static int net_socket_udp_init(NetClientState *peer,
>> NetSocketState *s;
>> int fd, ret;
>> struct sockaddr_in laddr, raddr;
>> + Error *err = NULL;
>>
>> if (parse_host_port(&laddr, lhost) < 0) {
>> return -1;
>> @@ -652,8 +671,9 @@ static int net_socket_udp_init(NetClientState *peer,
>> }
>> qemu_set_nonblock(fd);
>>
>> - s = net_socket_fd_init(peer, model, name, fd, 0);
>> + s = net_socket_fd_init(peer, model, name, fd, 0, &err);
>> if (!s) {
>> + error_report_err(err);
>> return -1;
>> }
>>
>> @@ -696,7 +716,7 @@ int net_init_socket(const Netdev *netdev, const char *name,
>> return -1;
>> }
>> qemu_set_nonblock(fd);
>> - if (!net_socket_fd_init(peer, "socket", name, fd, 1)) {
>> + if (!net_socket_fd_init(peer, "socket", name, fd, 1, errp)) {
>> return -1;
>> }
>> return 0;
>
> Looks good to me otherwise.
>
>
>
^ permalink raw reply [flat|nested] 13+ messages in thread
* [Qemu-devel] [PATCH v5 3/4] net/net: Convert parse_host_port() to Error
2017-06-27 3:24 [Qemu-devel] [PATCH v5 0/4] Improve error reporting Mao Zhongyi
2017-06-27 3:24 ` [Qemu-devel] [PATCH v5 1/4] net/socket: Don't treat odd socket type as SOCK_STREAM Mao Zhongyi
2017-06-27 3:24 ` [Qemu-devel] [PATCH v5 2/4] net/socket: Convert error message to Error Mao Zhongyi
@ 2017-06-27 3:24 ` Mao Zhongyi
2017-06-27 8:04 ` Markus Armbruster
2017-06-27 3:24 ` [Qemu-devel] [PATCH v5 4/4] net/socket: Improve -net socket error reporting Mao Zhongyi
3 siblings, 1 reply; 13+ messages in thread
From: Mao Zhongyi @ 2017-06-27 3:24 UTC (permalink / raw)
To: qemu-devel; +Cc: berrange, kraxel, pbonzini, jasowang, armbru
Cc: berrange@redhat.com
Cc: kraxel@redhat.com
Cc: pbonzini@redhat.com
Cc: jasowang@redhat.com
Cc: armbru@redhat.com
Signed-off-by: Mao Zhongyi <maozy.fnst@cn.fujitsu.com>
---
include/qemu/sockets.h | 3 ++-
net/net.c | 22 +++++++++++++++++-----
net/socket.c | 19 ++++++++++++++-----
3 files changed, 33 insertions(+), 11 deletions(-)
diff --git a/include/qemu/sockets.h b/include/qemu/sockets.h
index 5c326db..78e2b30 100644
--- a/include/qemu/sockets.h
+++ b/include/qemu/sockets.h
@@ -53,7 +53,8 @@ void socket_listen_cleanup(int fd, Error **errp);
int socket_dgram(SocketAddress *remote, SocketAddress *local, Error **errp);
/* Old, ipv4 only bits. Don't use for new code. */
-int parse_host_port(struct sockaddr_in *saddr, const char *str);
+int parse_host_port(struct sockaddr_in *saddr, const char *str,
+ Error **errp);
int socket_init(void);
/**
diff --git a/net/net.c b/net/net.c
index 6235aab..884e3ac 100644
--- a/net/net.c
+++ b/net/net.c
@@ -100,7 +100,8 @@ static int get_str_sep(char *buf, int buf_size, const char **pp, int sep)
return 0;
}
-int parse_host_port(struct sockaddr_in *saddr, const char *str)
+int parse_host_port(struct sockaddr_in *saddr, const char *str,
+ Error **errp)
{
char buf[512];
struct hostent *he;
@@ -108,24 +109,35 @@ int parse_host_port(struct sockaddr_in *saddr, const char *str)
int port;
p = str;
- if (get_str_sep(buf, sizeof(buf), &p, ':') < 0)
+ if (get_str_sep(buf, sizeof(buf), &p, ':') < 0) {
+ error_setg(errp, "The address should contain ':', for example: "
+ "xxxx=230.0.0.1:1234");
return -1;
+ }
saddr->sin_family = AF_INET;
if (buf[0] == '\0') {
saddr->sin_addr.s_addr = 0;
} else {
if (qemu_isdigit(buf[0])) {
- if (!inet_aton(buf, &saddr->sin_addr))
+ if (!inet_aton(buf, &saddr->sin_addr)) {
+ error_setg(errp, "Host address '%s' is not a valid "
+ "IPv4 address", buf);
return -1;
+ }
} else {
- if ((he = gethostbyname(buf)) == NULL)
+ he = gethostbyname(buf);
+ if (he == NULL) {
+ error_setg(errp, "Specified hostname is error.");
return - 1;
+ }
saddr->sin_addr = *(struct in_addr *)he->h_addr;
}
}
port = strtol(p, (char **)&r, 0);
- if (r == p)
+ if (r == p) {
+ error_setg(errp, "The port number is illegal");
return -1;
+ }
saddr->sin_port = htons(port);
return 0;
}
diff --git a/net/socket.c b/net/socket.c
index e136f87..a875205 100644
--- a/net/socket.c
+++ b/net/socket.c
@@ -501,9 +501,12 @@ static int net_socket_listen_init(NetClientState *peer,
NetSocketState *s;
struct sockaddr_in saddr;
int fd, ret;
+ Error *err = NULL;
- if (parse_host_port(&saddr, host_str) < 0)
+ if (parse_host_port(&saddr, host_str, &err) < 0) {
+ error_report_err(err);
return -1;
+ }
fd = qemu_socket(PF_INET, SOCK_STREAM, 0);
if (fd < 0) {
@@ -548,8 +551,10 @@ static int net_socket_connect_init(NetClientState *peer,
struct sockaddr_in saddr;
Error *err = NULL;
- if (parse_host_port(&saddr, host_str) < 0)
+ if (parse_host_port(&saddr, host_str, &err) < 0) {
+ error_report_err(err);
return -1;
+ }
fd = qemu_socket(PF_INET, SOCK_STREAM, 0);
if (fd < 0) {
@@ -601,8 +606,10 @@ static int net_socket_mcast_init(NetClientState *peer,
struct in_addr localaddr, *param_localaddr;
Error *err = NULL;
- if (parse_host_port(&saddr, host_str) < 0)
+ if (parse_host_port(&saddr, host_str, &err) < 0) {
+ error_report_err(err);
return -1;
+ }
if (localaddr_str != NULL) {
if (inet_aton(localaddr_str, &localaddr) == 0)
@@ -644,11 +651,13 @@ static int net_socket_udp_init(NetClientState *peer,
struct sockaddr_in laddr, raddr;
Error *err = NULL;
- if (parse_host_port(&laddr, lhost) < 0) {
+ if (parse_host_port(&laddr, lhost, &err) < 0) {
+ error_report_err(err);
return -1;
}
- if (parse_host_port(&raddr, rhost) < 0) {
+ if (parse_host_port(&raddr, rhost, &err) < 0) {
+ error_report_err(err);
return -1;
}
--
2.9.4
^ permalink raw reply related [flat|nested] 13+ messages in thread* Re: [Qemu-devel] [PATCH v5 3/4] net/net: Convert parse_host_port() to Error
2017-06-27 3:24 ` [Qemu-devel] [PATCH v5 3/4] net/net: Convert parse_host_port() " Mao Zhongyi
@ 2017-06-27 8:04 ` Markus Armbruster
2017-06-28 4:08 ` Mao Zhongyi
0 siblings, 1 reply; 13+ messages in thread
From: Markus Armbruster @ 2017-06-27 8:04 UTC (permalink / raw)
To: Mao Zhongyi; +Cc: qemu-devel, pbonzini, jasowang, kraxel
Mao Zhongyi <maozy.fnst@cn.fujitsu.com> writes:
> Cc: berrange@redhat.com
> Cc: kraxel@redhat.com
> Cc: pbonzini@redhat.com
> Cc: jasowang@redhat.com
> Cc: armbru@redhat.com
> Signed-off-by: Mao Zhongyi <maozy.fnst@cn.fujitsu.com>
> ---
> include/qemu/sockets.h | 3 ++-
> net/net.c | 22 +++++++++++++++++-----
> net/socket.c | 19 ++++++++++++++-----
> 3 files changed, 33 insertions(+), 11 deletions(-)
>
> diff --git a/include/qemu/sockets.h b/include/qemu/sockets.h
> index 5c326db..78e2b30 100644
> --- a/include/qemu/sockets.h
> +++ b/include/qemu/sockets.h
> @@ -53,7 +53,8 @@ void socket_listen_cleanup(int fd, Error **errp);
> int socket_dgram(SocketAddress *remote, SocketAddress *local, Error **errp);
>
> /* Old, ipv4 only bits. Don't use for new code. */
> -int parse_host_port(struct sockaddr_in *saddr, const char *str);
> +int parse_host_port(struct sockaddr_in *saddr, const char *str,
> + Error **errp);
> int socket_init(void);
>
> /**
> diff --git a/net/net.c b/net/net.c
> index 6235aab..884e3ac 100644
> --- a/net/net.c
> +++ b/net/net.c
> @@ -100,7 +100,8 @@ static int get_str_sep(char *buf, int buf_size, const char **pp, int sep)
> return 0;
> }
>
> -int parse_host_port(struct sockaddr_in *saddr, const char *str)
> +int parse_host_port(struct sockaddr_in *saddr, const char *str,
> + Error **errp)
> {
> char buf[512];
> struct hostent *he;
> @@ -108,24 +109,35 @@ int parse_host_port(struct sockaddr_in *saddr, const char *str)
> int port;
>
> p = str;
> - if (get_str_sep(buf, sizeof(buf), &p, ':') < 0)
> + if (get_str_sep(buf, sizeof(buf), &p, ':') < 0) {
> + error_setg(errp, "The address should contain ':', for example: "
> + "xxxx=230.0.0.1:1234");
Suggest "Host address '%s' should ..." like you do in the next error message.
The xxxx= is confusing. Do we need an example here? The other error
messages in this function apparently don't.
What about "host address '%s' doesn't contain ':' separating host from
port" or "can't find ':' separating host from port in host address
'%s'"?
> return -1;
> + }
> saddr->sin_family = AF_INET;
> if (buf[0] == '\0') {
> saddr->sin_addr.s_addr = 0;
> } else {
> if (qemu_isdigit(buf[0])) {
> - if (!inet_aton(buf, &saddr->sin_addr))
> + if (!inet_aton(buf, &saddr->sin_addr)) {
> + error_setg(errp, "Host address '%s' is not a valid "
> + "IPv4 address", buf);
> return -1;
> + }
> } else {
> - if ((he = gethostbyname(buf)) == NULL)
> + he = gethostbyname(buf);
> + if (he == NULL) {
> + error_setg(errp, "Specified hostname is error.");
No. Suggest "can't resolve host address '%s'. This error message still
lacks detail, but I'm not sure hstrerror() is sufficiently portable.
Outside this patch's scope: gethostbyname() is obsolete. Applications
should use getaddrinfo() instead. Comes with gai_strerror().
> return - 1;
> + }
> saddr->sin_addr = *(struct in_addr *)he->h_addr;
> }
> }
> port = strtol(p, (char **)&r, 0);
> - if (r == p)
> + if (r == p) {
> + error_setg(errp, "The port number is illegal");
Suggest "Port number '%s' is invalid".
> return -1;
> + }
> saddr->sin_port = htons(port);
> return 0;
> }
> diff --git a/net/socket.c b/net/socket.c
> index e136f87..a875205 100644
> --- a/net/socket.c
> +++ b/net/socket.c
> @@ -501,9 +501,12 @@ static int net_socket_listen_init(NetClientState *peer,
> NetSocketState *s;
> struct sockaddr_in saddr;
> int fd, ret;
> + Error *err = NULL;
>
> - if (parse_host_port(&saddr, host_str) < 0)
> + if (parse_host_port(&saddr, host_str, &err) < 0) {
> + error_report_err(err);
> return -1;
> + }
>
> fd = qemu_socket(PF_INET, SOCK_STREAM, 0);
> if (fd < 0) {
> @@ -548,8 +551,10 @@ static int net_socket_connect_init(NetClientState *peer,
> struct sockaddr_in saddr;
> Error *err = NULL;
>
> - if (parse_host_port(&saddr, host_str) < 0)
> + if (parse_host_port(&saddr, host_str, &err) < 0) {
> + error_report_err(err);
> return -1;
> + }
>
> fd = qemu_socket(PF_INET, SOCK_STREAM, 0);
> if (fd < 0) {
> @@ -601,8 +606,10 @@ static int net_socket_mcast_init(NetClientState *peer,
> struct in_addr localaddr, *param_localaddr;
> Error *err = NULL;
>
> - if (parse_host_port(&saddr, host_str) < 0)
> + if (parse_host_port(&saddr, host_str, &err) < 0) {
> + error_report_err(err);
> return -1;
> + }
>
> if (localaddr_str != NULL) {
> if (inet_aton(localaddr_str, &localaddr) == 0)
> @@ -644,11 +651,13 @@ static int net_socket_udp_init(NetClientState *peer,
> struct sockaddr_in laddr, raddr;
> Error *err = NULL;
>
> - if (parse_host_port(&laddr, lhost) < 0) {
> + if (parse_host_port(&laddr, lhost, &err) < 0) {
> + error_report_err(err);
> return -1;
> }
>
> - if (parse_host_port(&raddr, rhost) < 0) {
> + if (parse_host_port(&raddr, rhost, &err) < 0) {
> + error_report_err(err);
> return -1;
> }
Looks good otherwise.
^ permalink raw reply [flat|nested] 13+ messages in thread* Re: [Qemu-devel] [PATCH v5 3/4] net/net: Convert parse_host_port() to Error
2017-06-27 8:04 ` Markus Armbruster
@ 2017-06-28 4:08 ` Mao Zhongyi
2017-06-28 5:51 ` Markus Armbruster
0 siblings, 1 reply; 13+ messages in thread
From: Mao Zhongyi @ 2017-06-28 4:08 UTC (permalink / raw)
To: Markus Armbruster; +Cc: qemu-devel, pbonzini, jasowang, kraxel
Hi, Markus
On 06/27/2017 04:04 PM, Markus Armbruster wrote:
> Mao Zhongyi <maozy.fnst@cn.fujitsu.com> writes:
>
>> Cc: berrange@redhat.com
>> Cc: kraxel@redhat.com
>> Cc: pbonzini@redhat.com
>> Cc: jasowang@redhat.com
>> Cc: armbru@redhat.com
>> Signed-off-by: Mao Zhongyi <maozy.fnst@cn.fujitsu.com>
>> ---
>> include/qemu/sockets.h | 3 ++-
>> net/net.c | 22 +++++++++++++++++-----
>> net/socket.c | 19 ++++++++++++++-----
>> 3 files changed, 33 insertions(+), 11 deletions(-)
>>
>> diff --git a/include/qemu/sockets.h b/include/qemu/sockets.h
>> index 5c326db..78e2b30 100644
>> --- a/include/qemu/sockets.h
>> +++ b/include/qemu/sockets.h
>> @@ -53,7 +53,8 @@ void socket_listen_cleanup(int fd, Error **errp);
>> int socket_dgram(SocketAddress *remote, SocketAddress *local, Error **errp);
>>
>> /* Old, ipv4 only bits. Don't use for new code. */
>> -int parse_host_port(struct sockaddr_in *saddr, const char *str);
>> +int parse_host_port(struct sockaddr_in *saddr, const char *str,
>> + Error **errp);
>> int socket_init(void);
>>
>> /**
>> diff --git a/net/net.c b/net/net.c
>> index 6235aab..884e3ac 100644
>> --- a/net/net.c
>> +++ b/net/net.c
>> @@ -100,7 +100,8 @@ static int get_str_sep(char *buf, int buf_size, const char **pp, int sep)
>> return 0;
>> }
>>
>> -int parse_host_port(struct sockaddr_in *saddr, const char *str)
>> +int parse_host_port(struct sockaddr_in *saddr, const char *str,
>> + Error **errp)
>> {
>> char buf[512];
>> struct hostent *he;
>> @@ -108,24 +109,35 @@ int parse_host_port(struct sockaddr_in *saddr, const char *str)
>> int port;
>>
>> p = str;
>> - if (get_str_sep(buf, sizeof(buf), &p, ':') < 0)
>> + if (get_str_sep(buf, sizeof(buf), &p, ':') < 0) {
>> + error_setg(errp, "The address should contain ':', for example: "
>> + "xxxx=230.0.0.1:1234");
>
> Suggest "Host address '%s' should ..." like you do in the next error message.
>
> The xxxx= is confusing. Do we need an example here? The other error
> messages in this function apparently don't.
>
> What about "host address '%s' doesn't contain ':' separating host from
> port" or "can't find ':' separating host from port in host address
> '%s'"?
>
Yes, my description message is really confusing.
Both of your prompt message are well understood.
Thank you very much.
>
>> return -1;
>> + }
>> saddr->sin_family = AF_INET;
>> if (buf[0] == '\0') {
>> saddr->sin_addr.s_addr = 0;
>> } else {
>> if (qemu_isdigit(buf[0])) {
>> - if (!inet_aton(buf, &saddr->sin_addr))
>> + if (!inet_aton(buf, &saddr->sin_addr)) {
>> + error_setg(errp, "Host address '%s' is not a valid "
>> + "IPv4 address", buf);
>> return -1;
>> + }
>> } else {
>> - if ((he = gethostbyname(buf)) == NULL)
>> + he = gethostbyname(buf);
>> + if (he == NULL) {
>> + error_setg(errp, "Specified hostname is error.");
>
> No. Suggest "can't resolve host address '%s'. This error message still
> lacks detail, but I'm not sure hstrerror() is sufficiently portable.
>
The gethostbyname() return a null pointer if an error occurs, and the h_errno
variable holds an error number. herror() and hstrerror() can prints the error
message associated with the current value of h_errno, but hstrerror() returns
the string type is good for passing the error message to Error. So I'd prefer
the hstrerror.
As for the portability of hstrerror(), sorry, I'm also not sure, but in this
case I tested, it's OK. so I want to use hstrerror() for a while, if there are
any problem that can be fixed later. Do you think it can be done?
> Outside this patch's scope: gethostbyname() is obsolete. Applications
> should use getaddrinfo() instead. Comes with gai_strerror().
Can I try to fix it later?
Thanks,
Mao
>
>> return - 1;
>> + }
>> saddr->sin_addr = *(struct in_addr *)he->h_addr;
>> }
>> }
>> port = strtol(p, (char **)&r, 0);
>> - if (r == p)
>> + if (r == p) {
>> + error_setg(errp, "The port number is illegal");
>
> Suggest "Port number '%s' is invalid".
>
>> return -1;
>> + }
>> saddr->sin_port = htons(port);
>> return 0;
>> }
>> diff --git a/net/socket.c b/net/socket.c
>> index e136f87..a875205 100644
>> --- a/net/socket.c
>> +++ b/net/socket.c
>> @@ -501,9 +501,12 @@ static int net_socket_listen_init(NetClientState *peer,
>> NetSocketState *s;
>> struct sockaddr_in saddr;
>> int fd, ret;
>> + Error *err = NULL;
>>
>> - if (parse_host_port(&saddr, host_str) < 0)
>> + if (parse_host_port(&saddr, host_str, &err) < 0) {
>> + error_report_err(err);
>> return -1;
>> + }
>>
>> fd = qemu_socket(PF_INET, SOCK_STREAM, 0);
>> if (fd < 0) {
>> @@ -548,8 +551,10 @@ static int net_socket_connect_init(NetClientState *peer,
>> struct sockaddr_in saddr;
>> Error *err = NULL;
>>
>> - if (parse_host_port(&saddr, host_str) < 0)
>> + if (parse_host_port(&saddr, host_str, &err) < 0) {
>> + error_report_err(err);
>> return -1;
>> + }
>>
>> fd = qemu_socket(PF_INET, SOCK_STREAM, 0);
>> if (fd < 0) {
>> @@ -601,8 +606,10 @@ static int net_socket_mcast_init(NetClientState *peer,
>> struct in_addr localaddr, *param_localaddr;
>> Error *err = NULL;
>>
>> - if (parse_host_port(&saddr, host_str) < 0)
>> + if (parse_host_port(&saddr, host_str, &err) < 0) {
>> + error_report_err(err);
>> return -1;
>> + }
>>
>> if (localaddr_str != NULL) {
>> if (inet_aton(localaddr_str, &localaddr) == 0)
>> @@ -644,11 +651,13 @@ static int net_socket_udp_init(NetClientState *peer,
>> struct sockaddr_in laddr, raddr;
>> Error *err = NULL;
>>
>> - if (parse_host_port(&laddr, lhost) < 0) {
>> + if (parse_host_port(&laddr, lhost, &err) < 0) {
>> + error_report_err(err);
>> return -1;
>> }
>>
>> - if (parse_host_port(&raddr, rhost) < 0) {
>> + if (parse_host_port(&raddr, rhost, &err) < 0) {
>> + error_report_err(err);
>> return -1;
>> }
>
> Looks good otherwise.
>
>
>
^ permalink raw reply [flat|nested] 13+ messages in thread* Re: [Qemu-devel] [PATCH v5 3/4] net/net: Convert parse_host_port() to Error
2017-06-28 4:08 ` Mao Zhongyi
@ 2017-06-28 5:51 ` Markus Armbruster
2017-06-28 8:02 ` Paolo Bonzini
0 siblings, 1 reply; 13+ messages in thread
From: Markus Armbruster @ 2017-06-28 5:51 UTC (permalink / raw)
To: Mao Zhongyi; +Cc: pbonzini, jasowang, qemu-devel, kraxel
Mao Zhongyi <maozy.fnst@cn.fujitsu.com> writes:
> Hi, Markus
>
> On 06/27/2017 04:04 PM, Markus Armbruster wrote:
>> Mao Zhongyi <maozy.fnst@cn.fujitsu.com> writes:
>>
>>> Cc: berrange@redhat.com
>>> Cc: kraxel@redhat.com
>>> Cc: pbonzini@redhat.com
>>> Cc: jasowang@redhat.com
>>> Cc: armbru@redhat.com
>>> Signed-off-by: Mao Zhongyi <maozy.fnst@cn.fujitsu.com>
>>> ---
>>> include/qemu/sockets.h | 3 ++-
>>> net/net.c | 22 +++++++++++++++++-----
>>> net/socket.c | 19 ++++++++++++++-----
>>> 3 files changed, 33 insertions(+), 11 deletions(-)
>>>
>>> diff --git a/include/qemu/sockets.h b/include/qemu/sockets.h
>>> index 5c326db..78e2b30 100644
>>> --- a/include/qemu/sockets.h
>>> +++ b/include/qemu/sockets.h
>>> @@ -53,7 +53,8 @@ void socket_listen_cleanup(int fd, Error **errp);
>>> int socket_dgram(SocketAddress *remote, SocketAddress *local, Error **errp);
>>>
>>> /* Old, ipv4 only bits. Don't use for new code. */
>>> -int parse_host_port(struct sockaddr_in *saddr, const char *str);
>>> +int parse_host_port(struct sockaddr_in *saddr, const char *str,
>>> + Error **errp);
>>> int socket_init(void);
>>>
>>> /**
>>> diff --git a/net/net.c b/net/net.c
>>> index 6235aab..884e3ac 100644
>>> --- a/net/net.c
>>> +++ b/net/net.c
>>> @@ -100,7 +100,8 @@ static int get_str_sep(char *buf, int buf_size, const char **pp, int sep)
>>> return 0;
>>> }
>>>
>>> -int parse_host_port(struct sockaddr_in *saddr, const char *str)
>>> +int parse_host_port(struct sockaddr_in *saddr, const char *str,
>>> + Error **errp)
>>> {
>>> char buf[512];
>>> struct hostent *he;
>>> @@ -108,24 +109,35 @@ int parse_host_port(struct sockaddr_in *saddr, const char *str)
>>> int port;
>>>
>>> p = str;
>>> - if (get_str_sep(buf, sizeof(buf), &p, ':') < 0)
>>> + if (get_str_sep(buf, sizeof(buf), &p, ':') < 0) {
>>> + error_setg(errp, "The address should contain ':', for example: "
>>> + "xxxx=230.0.0.1:1234");
>>
>> Suggest "Host address '%s' should ..." like you do in the next error message.
>>
>> The xxxx= is confusing. Do we need an example here? The other error
>> messages in this function apparently don't.
>>
>> What about "host address '%s' doesn't contain ':' separating host from
>> port" or "can't find ':' separating host from port in host address
>> '%s'"?
>>
>
> Yes, my description message is really confusing.
> Both of your prompt message are well understood.
>
> Thank you very much.
>
>>
>>> return -1;
>>> + }
>>> saddr->sin_family = AF_INET;
>>> if (buf[0] == '\0') {
>>> saddr->sin_addr.s_addr = 0;
>>> } else {
>>> if (qemu_isdigit(buf[0])) {
>>> - if (!inet_aton(buf, &saddr->sin_addr))
>>> + if (!inet_aton(buf, &saddr->sin_addr)) {
>>> + error_setg(errp, "Host address '%s' is not a valid "
>>> + "IPv4 address", buf);
>>> return -1;
>>> + }
>>> } else {
>>> - if ((he = gethostbyname(buf)) == NULL)
>>> + he = gethostbyname(buf);
>>> + if (he == NULL) {
>>> + error_setg(errp, "Specified hostname is error.");
>>
>> No. Suggest "can't resolve host address '%s'. This error message still
>> lacks detail, but I'm not sure hstrerror() is sufficiently portable.
>>
>
> The gethostbyname() return a null pointer if an error occurs, and the h_errno
> variable holds an error number. herror() and hstrerror() can prints the error
> message associated with the current value of h_errno, but hstrerror() returns
> the string type is good for passing the error message to Error. So I'd prefer
> the hstrerror.
>
> As for the portability of hstrerror(), sorry, I'm also not sure, but in this
> case I tested, it's OK. so I want to use hstrerror() for a while, if there are
> any problem that can be fixed later. Do you think it can be done?
Standard first portability question: does Windows provide it?
>> Outside this patch's scope: gethostbyname() is obsolete. Applications
>> should use getaddrinfo() instead. Comes with gai_strerror().
>
> Can I try to fix it later?
Sure. By "outside this patch's scope" I mean it's a separate matter
that clearly doesn't have to be addressed to get this patch accepted.
^ permalink raw reply [flat|nested] 13+ messages in thread* Re: [Qemu-devel] [PATCH v5 3/4] net/net: Convert parse_host_port() to Error
2017-06-28 5:51 ` Markus Armbruster
@ 2017-06-28 8:02 ` Paolo Bonzini
2017-06-28 10:56 ` Markus Armbruster
0 siblings, 1 reply; 13+ messages in thread
From: Paolo Bonzini @ 2017-06-28 8:02 UTC (permalink / raw)
To: Markus Armbruster, Mao Zhongyi; +Cc: jasowang, qemu-devel, kraxel
On 28/06/2017 07:51, Markus Armbruster wrote:
>> The gethostbyname() return a null pointer if an error occurs, and the h_errno
>> variable holds an error number. herror() and hstrerror() can prints the error
>> message associated with the current value of h_errno, but hstrerror() returns
>> the string type is good for passing the error message to Error. So I'd prefer
>> the hstrerror.
>>
>> As for the portability of hstrerror(), sorry, I'm also not sure, but in this
>> case I tested, it's OK. so I want to use hstrerror() for a while, if there are
>> any problem that can be fixed later. Do you think it can be done?
>
> Standard first portability question: does Windows provide it?
Nope. But it does have gai_strerror.
Paolo
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [Qemu-devel] [PATCH v5 3/4] net/net: Convert parse_host_port() to Error
2017-06-28 8:02 ` Paolo Bonzini
@ 2017-06-28 10:56 ` Markus Armbruster
2017-06-28 13:01 ` Mao Zhongyi
0 siblings, 1 reply; 13+ messages in thread
From: Markus Armbruster @ 2017-06-28 10:56 UTC (permalink / raw)
To: Paolo Bonzini; +Cc: Mao Zhongyi, jasowang, qemu-devel, kraxel
Paolo Bonzini <pbonzini@redhat.com> writes:
> On 28/06/2017 07:51, Markus Armbruster wrote:
>>> The gethostbyname() return a null pointer if an error occurs, and the h_errno
>>> variable holds an error number. herror() and hstrerror() can prints the error
>>> message associated with the current value of h_errno, but hstrerror() returns
>>> the string type is good for passing the error message to Error. So I'd prefer
>>> the hstrerror.
>>>
>>> As for the portability of hstrerror(), sorry, I'm also not sure, but in this
>>> case I tested, it's OK. so I want to use hstrerror() for a while, if there are
>>> any problem that can be fixed later. Do you think it can be done?
>>
>> Standard first portability question: does Windows provide it?
>
> Nope. But it does have gai_strerror.
Let's go with the generic error message I suggested, and leave adding
detail to the patch that converts to getaddrinfo().
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [Qemu-devel] [PATCH v5 3/4] net/net: Convert parse_host_port() to Error
2017-06-28 10:56 ` Markus Armbruster
@ 2017-06-28 13:01 ` Mao Zhongyi
0 siblings, 0 replies; 13+ messages in thread
From: Mao Zhongyi @ 2017-06-28 13:01 UTC (permalink / raw)
To: Markus Armbruster, Paolo Bonzini; +Cc: jasowang, qemu-devel, kraxel
On 06/28/2017 06:56 PM, Markus Armbruster wrote:
> Paolo Bonzini <pbonzini@redhat.com> writes:
>
>> On 28/06/2017 07:51, Markus Armbruster wrote:
>>>> The gethostbyname() return a null pointer if an error occurs, and the h_errno
>>>> variable holds an error number. herror() and hstrerror() can prints the error
>>>> message associated with the current value of h_errno, but hstrerror() returns
>>>> the string type is good for passing the error message to Error. So I'd prefer
>>>> the hstrerror.
>>>>
>>>> As for the portability of hstrerror(), sorry, I'm also not sure, but in this
>>>> case I tested, it's OK. so I want to use hstrerror() for a while, if there are
>>>> any problem that can be fixed later. Do you think it can be done?
>>>
>>> Standard first portability question: does Windows provide it?
>>
>> Nope. But it does have gai_strerror.
>
> Let's go with the generic error message I suggested, and leave adding
> detail to the patch that converts to getaddrinfo().
OK, I will fix it right away.
Thanks,
Mao
^ permalink raw reply [flat|nested] 13+ messages in thread
* [Qemu-devel] [PATCH v5 4/4] net/socket: Improve -net socket error reporting
2017-06-27 3:24 [Qemu-devel] [PATCH v5 0/4] Improve error reporting Mao Zhongyi
` (2 preceding siblings ...)
2017-06-27 3:24 ` [Qemu-devel] [PATCH v5 3/4] net/net: Convert parse_host_port() " Mao Zhongyi
@ 2017-06-27 3:24 ` Mao Zhongyi
3 siblings, 0 replies; 13+ messages in thread
From: Mao Zhongyi @ 2017-06-27 3:24 UTC (permalink / raw)
To: qemu-devel; +Cc: jasowang, armbru
When -net socket fails, it first reports a specific error, then
a generic one, like this:
$ qemu-system-x86_64 -net socket,
qemu-system-x86_64: -net socket: exactly one of fd=, listen=, connect=, mcast= or udp= is required
qemu-system-x86_64: -net socket: Device 'socket' could not be initialized
Convert net_socket_*_init() to Error to get rid of the superfluous second
error message. After the patch, the effect like this:
$ qemu-system-x86_64 -net socket,
qemu-system-x86_64: -net socket: exactly one of fd=, listen=, connect=, mcast= or udp= is required
At the same time, add many explicit error handling message when it fails.
Cc: jasowang@redhat.com
Cc: armbru@redhat.com
Signed-off-by: Mao Zhongyi <maozy.fnst@cn.fujitsu.com>
---
net/socket.c | 95 ++++++++++++++++++++++++++++++------------------------------
1 file changed, 47 insertions(+), 48 deletions(-)
diff --git a/net/socket.c b/net/socket.c
index a875205..8be37d1 100644
--- a/net/socket.c
+++ b/net/socket.c
@@ -495,22 +495,22 @@ static void net_socket_accept(void *opaque)
static int net_socket_listen_init(NetClientState *peer,
const char *model,
const char *name,
- const char *host_str)
+ const char *host_str,
+ Error **errp)
{
NetClientState *nc;
NetSocketState *s;
struct sockaddr_in saddr;
int fd, ret;
- Error *err = NULL;
- if (parse_host_port(&saddr, host_str, &err) < 0) {
- error_report_err(err);
+ if (parse_host_port(&saddr, host_str, errp) < 0) {
return -1;
}
fd = qemu_socket(PF_INET, SOCK_STREAM, 0);
if (fd < 0) {
- perror("socket");
+ error_setg_errno(errp, errno, "Create socket(PF_INET,"
+ " SOCK_STREAM) failed");
return -1;
}
qemu_set_nonblock(fd);
@@ -519,13 +519,14 @@ static int net_socket_listen_init(NetClientState *peer,
ret = bind(fd, (struct sockaddr *)&saddr, sizeof(saddr));
if (ret < 0) {
- perror("bind");
+ error_setg_errno(errp, errno, "Bind ip=%s to fd=%d failed",
+ inet_ntoa(saddr.sin_addr), fd);
closesocket(fd);
return -1;
}
ret = listen(fd, 0);
if (ret < 0) {
- perror("listen");
+ error_setg_errno(errp, errno, "Listen socket fd=%d failed", fd);
closesocket(fd);
return -1;
}
@@ -544,21 +545,21 @@ static int net_socket_listen_init(NetClientState *peer,
static int net_socket_connect_init(NetClientState *peer,
const char *model,
const char *name,
- const char *host_str)
+ const char *host_str,
+ Error **errp)
{
NetSocketState *s;
int fd, connected, ret;
struct sockaddr_in saddr;
- Error *err = NULL;
- if (parse_host_port(&saddr, host_str, &err) < 0) {
- error_report_err(err);
+ if (parse_host_port(&saddr, host_str, errp) < 0) {
return -1;
}
fd = qemu_socket(PF_INET, SOCK_STREAM, 0);
if (fd < 0) {
- perror("socket");
+ error_setg_errno(errp, errno, "Create socket(PF_INET,"
+ " SOCK_STREAM) failed");
return -1;
}
qemu_set_nonblock(fd);
@@ -574,7 +575,7 @@ static int net_socket_connect_init(NetClientState *peer,
errno == EINVAL) {
break;
} else {
- perror("connect");
+ error_setg_errno(errp, errno, "Connection failed");
closesocket(fd);
return -1;
}
@@ -583,9 +584,8 @@ static int net_socket_connect_init(NetClientState *peer,
break;
}
}
- s = net_socket_fd_init(peer, model, name, fd, connected, &err);
+ s = net_socket_fd_init(peer, model, name, fd, connected, errp);
if (!s) {
- error_report_err(err);
return -1;
}
snprintf(s->nc.info_str, sizeof(s->nc.info_str),
@@ -598,36 +598,36 @@ static int net_socket_mcast_init(NetClientState *peer,
const char *model,
const char *name,
const char *host_str,
- const char *localaddr_str)
+ const char *localaddr_str,
+ Error **errp)
{
NetSocketState *s;
int fd;
struct sockaddr_in saddr;
struct in_addr localaddr, *param_localaddr;
- Error *err = NULL;
- if (parse_host_port(&saddr, host_str, &err) < 0) {
- error_report_err(err);
+ if (parse_host_port(&saddr, host_str, errp) < 0) {
return -1;
}
if (localaddr_str != NULL) {
- if (inet_aton(localaddr_str, &localaddr) == 0)
+ if (inet_aton(localaddr_str, &localaddr) == 0) {
+ error_setg(errp, "localaddr '%s' is not a valid IPv4 address",
+ localaddr_str);
return -1;
+ }
param_localaddr = &localaddr;
} else {
param_localaddr = NULL;
}
- fd = net_socket_mcast_create(&saddr, param_localaddr, &err);
+ fd = net_socket_mcast_create(&saddr, param_localaddr, errp);
if (fd < 0) {
- error_report_err(err);
return -1;
}
- s = net_socket_fd_init(peer, model, name, fd, 0, &err);
+ s = net_socket_fd_init(peer, model, name, fd, 0, errp);
if (!s) {
- error_report_err(err);
return -1;
}
@@ -644,45 +644,46 @@ static int net_socket_udp_init(NetClientState *peer,
const char *model,
const char *name,
const char *rhost,
- const char *lhost)
+ const char *lhost,
+ Error **errp)
{
NetSocketState *s;
int fd, ret;
struct sockaddr_in laddr, raddr;
- Error *err = NULL;
- if (parse_host_port(&laddr, lhost, &err) < 0) {
- error_report_err(err);
+ if (parse_host_port(&laddr, lhost, errp) < 0) {
return -1;
}
- if (parse_host_port(&raddr, rhost, &err) < 0) {
- error_report_err(err);
+ if (parse_host_port(&raddr, rhost, errp) < 0) {
return -1;
}
fd = qemu_socket(PF_INET, SOCK_DGRAM, 0);
if (fd < 0) {
- perror("socket(PF_INET, SOCK_DGRAM)");
+ error_setg_errno(errp, errno, "Create socket(PF_INET,"
+ " SOCK_DGRAM) failed");
return -1;
}
ret = socket_set_fast_reuse(fd);
if (ret < 0) {
+ error_setg_errno(errp, errno, "Set the socket fd=%d attribute"
+ " SO_REUSEADDR failed", fd);
closesocket(fd);
return -1;
}
ret = bind(fd, (struct sockaddr *)&laddr, sizeof(laddr));
if (ret < 0) {
- perror("bind");
+ error_setg_errno(errp, errno, "Bind ip=%s to fd=%d failed",
+ inet_ntoa(laddr.sin_addr), fd);
closesocket(fd);
return -1;
}
qemu_set_nonblock(fd);
- s = net_socket_fd_init(peer, model, name, fd, 0, &err);
+ s = net_socket_fd_init(peer, model, name, fd, 0, errp);
if (!s) {
- error_report_err(err);
return -1;
}
@@ -697,8 +698,6 @@ static int net_socket_udp_init(NetClientState *peer,
int net_init_socket(const Netdev *netdev, const char *name,
NetClientState *peer, Error **errp)
{
- /* FIXME error_setg(errp, ...) on failure */
- Error *err = NULL;
const NetdevSocketOptions *sock;
assert(netdev->type == NET_CLIENT_DRIVER_SOCKET);
@@ -706,22 +705,21 @@ int net_init_socket(const Netdev *netdev, const char *name,
if (sock->has_fd + sock->has_listen + sock->has_connect + sock->has_mcast +
sock->has_udp != 1) {
- error_report("exactly one of fd=, listen=, connect=, mcast= or udp="
+ error_setg(errp, "exactly one of fd=, listen=, connect=, mcast= or udp="
" is required");
return -1;
}
if (sock->has_localaddr && !sock->has_mcast && !sock->has_udp) {
- error_report("localaddr= is only valid with mcast= or udp=");
+ error_setg(errp, "localaddr= is only valid with mcast= or udp=");
return -1;
}
if (sock->has_fd) {
int fd;
- fd = monitor_fd_param(cur_mon, sock->fd, &err);
+ fd = monitor_fd_param(cur_mon, sock->fd, errp);
if (fd == -1) {
- error_report_err(err);
return -1;
}
qemu_set_nonblock(fd);
@@ -732,15 +730,16 @@ int net_init_socket(const Netdev *netdev, const char *name,
}
if (sock->has_listen) {
- if (net_socket_listen_init(peer, "socket", name, sock->listen) == -1) {
+ if (net_socket_listen_init(peer, "socket", name,
+ sock->listen, errp) == -1) {
return -1;
}
return 0;
}
if (sock->has_connect) {
- if (net_socket_connect_init(peer, "socket", name, sock->connect) ==
- -1) {
+ if (net_socket_connect_init(peer, "socket", name,
+ sock->connect, errp) == -1) {
return -1;
}
return 0;
@@ -749,8 +748,8 @@ int net_init_socket(const Netdev *netdev, const char *name,
if (sock->has_mcast) {
/* if sock->localaddr is missing, it has been initialized to "all bits
* zero" */
- if (net_socket_mcast_init(peer, "socket", name, sock->mcast,
- sock->localaddr) == -1) {
+ if (net_socket_mcast_init(peer, "socket", name,
+ sock->mcast, sock->localaddr, errp) == -1) {
return -1;
}
return 0;
@@ -758,11 +757,11 @@ int net_init_socket(const Netdev *netdev, const char *name,
assert(sock->has_udp);
if (!sock->has_localaddr) {
- error_report("localaddr= is mandatory with udp=");
+ error_setg(errp, "localaddr= is mandatory with udp=");
return -1;
}
- if (net_socket_udp_init(peer, "socket", name, sock->udp, sock->localaddr) ==
- -1) {
+ if (net_socket_udp_init(peer, "socket", name,
+ sock->udp, sock->localaddr, errp) == -1) {
return -1;
}
return 0;
--
2.9.4
^ permalink raw reply related [flat|nested] 13+ messages in thread