|
From: | Mao Zhongyi |
Subject: | Re: [Qemu-devel] [PATCH v2 3/4] net/socket: Convert error report message to Error |
Date: | Wed, 3 May 2017 16:59:11 +0800 |
User-agent: | Mozilla/5.0 (X11; Linux x86_64; rv:45.0) Gecko/20100101 Thunderbird/45.8.0 |
On 05/03/2017 04:54 PM, Markus Armbruster wrote:
Mao Zhongyi <address@hidden> writes:Hi, Markus,Daniel On 04/28/2017 04:02 PM, Markus Armbruster wrote:"Daniel P. Berrange" <address@hidden> writes:On Thu, Apr 27, 2017 at 06:24:17PM +0200, Markus Armbruster wrote:No review, just an observation. Mao Zhongyi <address@hidden> 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: address@hidden, address@hidden Signed-off-by: Mao Zhongyi <address@hidden> --- net/socket.c | 66 +++++++++++++++++++++++++++++++++++++----------------------- 1 file changed, 41 insertions(+), 25 deletions(-) diff --git a/net/socket.c b/net/socket.c index b0decbe..559e09a 100644 --- a/net/socket.c +++ b/net/socket.c[...]@@ -433,25 +437,27 @@ 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: /* 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); + error_setg(errp, "qemu: warning: socket type=%d for fd=%d is not SOCK_DGRAM" + " or SOCK_STREAM", so_type, fd);Not this patches problem: this case is odd, and the comment is bogus. If @fd really was a pty, getsockopt() would fail with ENOTSOCK, wouldn't it? If @fd is a socket, but neither SOCK_DGRAM nor SOCK_STREAM (say SOCK_RAW), why is it safe to continue as if it was SOCK_STREAM? Jason?IMHO it is a problem with this patch. Previously we merely printed a warning & carried on, which is conceptually ok in general, though dubious here for the reason you say. Now we are filling an Error **errp object, and carrying on - this is conceptually broken anywhere. If an Error ** is filled, we must return. If we want to carry on, we shouldn't fill Error **.You're right.return net_socket_fd_init_stream(peer, model, name, fd, is_connected);IMHO, we just kill this and put return NULL here. If there is a genuine reason to support something like SOCK_RAW, it should be explicitly handled, because this default: case will certainly break SOCK_SEQPACKET and SOCK_RDM which can't be treated as streams.It's either magic or misguided defensive programming. Probably the latter, but I'd like to hear Jason's opinion. If it's *necessary* magic, we can't use error_setg(). Else, we should drop the default, and insert a closesocket(fd) before the final return NULL.As Daniel said, although the previous printed warning message is dubious, but it is conceptually ok in general. Simply filling it to Error ** is problematic. Of course, as you said drop the default case, there will be no problem. But really to do that?Since Jason hasn't spoken up, I suggest you follow Dan's recommendation and insert a patch to drop the default case before your error rework.
I see, will drop the 'default' case in a separated patch. Thanks. Mao
[Prev in Thread] | Current Thread | [Next in Thread] |