On 05/03/2017 04:37 PM, Daniel P. Berrange wrote:
On Wed, May 03, 2017 at 03:09:57PM +0800, Mao Zhongyi wrote:Hi, Markus,Daniel On 04/28/2017 04:02 PM, Markus Armbruster wrote:"Daniel P. Berrange" <berra...@redhat.com> writes:On Thu, Apr 27, 2017 at 06:24:17PM +0200, Markus Armbruster wrote:No review, just an observation. Mao Zhongyi <maozy.f...@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: jasow...@redhat.com, arm...@redhat.com Signed-off-by: Mao Zhongyi <maozy.f...@cn.fujitsu.com> --- 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?Yes, please drop that 'default' case since it is broken already. BTW, drop the default case in a separate patch at the start of your series, before changing the error code, so the functional change is clear in git history. Regards, Daniel
OK，I see. Thank you very much. Mao
