Message ID | 20170505162305.15763-1-berrange@redhat.com |
---|---|
State | New |
Headers | show |
Hi, This series seems to have some coding style problems. See output below for more information: Type: series Subject: [Qemu-devel] [PATCH] Revert "Change net/socket.c to use socket_*() functions" again Message-id: 20170505162305.15763-1-berrange@redhat.com === TEST SCRIPT BEGIN === #!/bin/bash BASE=base n=1 total=$(git log --oneline $BASE.. | wc -l) failed=0 git config --local diff.renamelimit 0 git config --local diff.renames True commits="$(git log --format=%H --reverse $BASE..)" for c in $commits; do echo "Checking PATCH $n/$total: $(git log -n 1 --format=%s $c)..." if ! git show $c --format=email | ./scripts/checkpatch.pl --mailback -; then failed=1 echo fi n=$((n+1)) done exit $failed === TEST SCRIPT END === Updating 3c8cf5a9c21ff8782164d1def7f44bd888713384 From https://github.com/patchew-project/qemu - [tag update] patchew/20170504173745.27414-1-eblake@redhat.com -> patchew/20170504173745.27414-1-eblake@redhat.com Switched to a new branch 'test' 89f6b9d Revert "Change net/socket.c to use socket_*() functions" again === OUTPUT BEGIN === Checking PATCH 1/1: Revert "Change net/socket.c to use socket_*() functions" again... ERROR: braces {} are necessary for all arms of this statement #56: FILE: net/socket.c:495: + if (parse_host_port(&saddr, host_str) < 0) [...] ERROR: braces {} are necessary for all arms of this statement #159: FILE: net/socket.c:540: + if (parse_host_port(&saddr, host_str) < 0) [...] ERROR: space required before the open parenthesis '(' #172: FILE: net/socket.c:551: + for(;;) { ERROR: braces {} are necessary for all arms of this statement #192: FILE: net/socket.c:571: + if (!s) [...] total: 4 errors, 0 warnings, 162 lines checked Your patch has style problems, please review. If any of these errors are false positives report them to the maintainer, see CHECKPATCH in MAINTAINERS. === OUTPUT END === Test command exited with code: 1 --- Email generated automatically by Patchew [http://patchew.org/]. Please send your feedback to patchew-devel@freelists.org
On Fri, May 05, 2017 at 05:23:05PM +0100, Daniel P. Berrange wrote: > This reverts commit 883e4f7624e10b98d16d9adaffb8b1795664d899. > > This code changed net/socket.c from using socket()+connect(), > to using socket_connect(). In theory this is great, but in > practice this has completely broken the ability to connect > the frontend and backend: > > $ ./x86_64-softmmu/qemu-system-x86_64 \ > -device e1000,id=e0,netdev=hn0,mac=DE:AD:BE:EF:AF:05 \ > -netdev socket,id=hn0,connect=localhost:1234 > qemu-system-x86_64: -device e1000,id=e0,netdev=hn0,mac=DE:AD:BE:EF:AF:05: Property 'e1000.netdev' can't find value 'hn0' > > The old code would call net_socket_fd_init() synchronously, > while letting the connect() complete in the backgorund. The > new code moved net_socket_fd_init() so that it is only called > after connect() completes in the background. > > Thus at the time we initialize the NIC frontend, the backend > does not exist. > > The socket_connect() conversion as done is a bad fit for the > current code, since it did not try to change the way it deals > with async connection completion. Rather than try to fix this, > just revert the socket_connect() conversion entirely. > > The code is about to be converted to use QIOChannel which > will let the problem be solved in a cleaner manner. This > revert is more suitable for stable branches in the meantime. > > Signed-off-by: Daniel P. Berrange <berrange@redhat.com> > --- > net/socket.c | 127 ++++++++++++++++++++++++++--------------------------------- > 1 file changed, 56 insertions(+), 71 deletions(-) Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>
On 2017年05月06日 03:36, no-reply@patchew.org wrote: > Hi, > > This series seems to have some coding style problems. See output below for > more information: > > Type: series > Subject: [Qemu-devel] [PATCH] Revert "Change net/socket.c to use socket_*() functions" again > Message-id: 20170505162305.15763-1-berrange@redhat.com > > === TEST SCRIPT BEGIN === > #!/bin/bash > > BASE=base > n=1 > total=$(git log --oneline $BASE.. | wc -l) > failed=0 > > git config --local diff.renamelimit 0 > git config --local diff.renames True > > commits="$(git log --format=%H --reverse $BASE..)" > for c in $commits; do > echo "Checking PATCH $n/$total: $(git log -n 1 --format=%s $c)..." > if ! git show $c --format=email | ./scripts/checkpatch.pl --mailback -; then > failed=1 > echo > fi > n=$((n+1)) > done > > exit $failed > === TEST SCRIPT END === > > Updating 3c8cf5a9c21ff8782164d1def7f44bd888713384 > From https://github.com/patchew-project/qemu > - [tag update] patchew/20170504173745.27414-1-eblake@redhat.com -> patchew/20170504173745.27414-1-eblake@redhat.com > Switched to a new branch 'test' > 89f6b9d Revert "Change net/socket.c to use socket_*() functions" again > > === OUTPUT BEGIN === > Checking PATCH 1/1: Revert "Change net/socket.c to use socket_*() functions" again... > ERROR: braces {} are necessary for all arms of this statement > #56: FILE: net/socket.c:495: > + if (parse_host_port(&saddr, host_str) < 0) > [...] > > ERROR: braces {} are necessary for all arms of this statement > #159: FILE: net/socket.c:540: > + if (parse_host_port(&saddr, host_str) < 0) > [...] > > ERROR: space required before the open parenthesis '(' > #172: FILE: net/socket.c:551: > + for(;;) { > > ERROR: braces {} are necessary for all arms of this statement > #192: FILE: net/socket.c:571: > + if (!s) > [...] > > total: 4 errors, 0 warnings, 162 lines checked > > Your patch has style problems, please review. If any of these errors > are false positives report them to the maintainer, see > CHECKPATCH in MAINTAINERS. > > === OUTPUT END === > > Test command exited with code: 1 > > > --- > Email generated automatically by Patchew [http://patchew.org/]. > Please send your feedback to patchew-devel@freelists.org Acked-by: Jason Wang <jasowang@redhat.com>
On 2017年05月06日 00:23, Daniel P. Berrange wrote: > This reverts commit 883e4f7624e10b98d16d9adaffb8b1795664d899. > > This code changed net/socket.c from using socket()+connect(), > to using socket_connect(). In theory this is great, but in > practice this has completely broken the ability to connect > the frontend and backend: > > $ ./x86_64-softmmu/qemu-system-x86_64 \ > -device e1000,id=e0,netdev=hn0,mac=DE:AD:BE:EF:AF:05 \ > -netdev socket,id=hn0,connect=localhost:1234 > qemu-system-x86_64: -device e1000,id=e0,netdev=hn0,mac=DE:AD:BE:EF:AF:05: Property 'e1000.netdev' can't find value 'hn0' > > The old code would call net_socket_fd_init() synchronously, > while letting the connect() complete in the backgorund. The > new code moved net_socket_fd_init() so that it is only called > after connect() completes in the background. > > Thus at the time we initialize the NIC frontend, the backend > does not exist. > > The socket_connect() conversion as done is a bad fit for the > current code, since it did not try to change the way it deals > with async connection completion. Rather than try to fix this, > just revert the socket_connect() conversion entirely. > > The code is about to be converted to use QIOChannel which > will let the problem be solved in a cleaner manner. This > revert is more suitable for stable branches in the meantime. > > Signed-off-by: Daniel P. Berrange<berrange@redhat.com> > --- > net/socket.c | 127 ++++++++++++++++++++++++++--------------------------------- > 1 file changed, 56 insertions(+), 71 deletions(-) Applied in -net. Thanks
diff --git a/net/socket.c b/net/socket.c index b8c931e..dcae1ae 100644 --- a/net/socket.c +++ b/net/socket.c @@ -489,106 +489,91 @@ static int net_socket_listen_init(NetClientState *peer, { NetClientState *nc; NetSocketState *s; - SocketAddress *saddr; - int ret; - Error *local_error = NULL; + struct sockaddr_in saddr; + int fd, ret; - saddr = socket_parse(host_str, &local_error); - if (saddr == NULL) { - error_report_err(local_error); + if (parse_host_port(&saddr, host_str) < 0) + return -1; + + fd = qemu_socket(PF_INET, SOCK_STREAM, 0); + if (fd < 0) { + perror("socket"); return -1; } + qemu_set_nonblock(fd); + + socket_set_fast_reuse(fd); - ret = socket_listen(saddr, &local_error); + ret = bind(fd, (struct sockaddr *)&saddr, sizeof(saddr)); if (ret < 0) { - qapi_free_SocketAddress(saddr); - error_report_err(local_error); + perror("bind"); + closesocket(fd); + return -1; + } + ret = listen(fd, 0); + if (ret < 0) { + perror("listen"); + closesocket(fd); return -1; } nc = qemu_new_net_client(&net_socket_info, peer, model, name); s = DO_UPCAST(NetSocketState, nc, nc); s->fd = -1; - s->listen_fd = ret; + s->listen_fd = fd; s->nc.link_down = true; net_socket_rs_init(&s->rs, net_socket_rs_finalize); qemu_set_fd_handler(s->listen_fd, net_socket_accept, NULL, s); - qapi_free_SocketAddress(saddr); return 0; } -typedef struct { - NetClientState *peer; - SocketAddress *saddr; - char *model; - char *name; -} socket_connect_data; - -static void socket_connect_data_free(socket_connect_data *c) -{ - qapi_free_SocketAddress(c->saddr); - g_free(c->model); - g_free(c->name); - g_free(c); -} - -static void net_socket_connected(int fd, Error *err, void *opaque) -{ - socket_connect_data *c = opaque; - NetSocketState *s; - char *addr_str = NULL; - Error *local_error = NULL; - - addr_str = socket_address_to_string(c->saddr, &local_error); - if (addr_str == NULL) { - error_report_err(local_error); - closesocket(fd); - goto end; - } - - s = net_socket_fd_init(c->peer, c->model, c->name, fd, true); - if (!s) { - closesocket(fd); - goto end; - } - - snprintf(s->nc.info_str, sizeof(s->nc.info_str), - "socket: connect to %s", addr_str); - -end: - g_free(addr_str); - socket_connect_data_free(c); -} - static int net_socket_connect_init(NetClientState *peer, const char *model, const char *name, const char *host_str) { - socket_connect_data *c = g_new0(socket_connect_data, 1); - int fd = -1; - Error *local_error = NULL; + NetSocketState *s; + int fd, connected, ret; + struct sockaddr_in saddr; - c->peer = peer; - c->model = g_strdup(model); - c->name = g_strdup(name); - c->saddr = socket_parse(host_str, &local_error); - if (c->saddr == NULL) { - goto err; - } + if (parse_host_port(&saddr, host_str) < 0) + return -1; - fd = socket_connect(c->saddr, net_socket_connected, c, &local_error); + fd = qemu_socket(PF_INET, SOCK_STREAM, 0); if (fd < 0) { - goto err; + perror("socket"); + return -1; } + qemu_set_nonblock(fd); + connected = 0; + for(;;) { + ret = connect(fd, (struct sockaddr *)&saddr, sizeof(saddr)); + if (ret < 0) { + if (errno == EINTR || errno == EWOULDBLOCK) { + /* continue */ + } else if (errno == EINPROGRESS || + errno == EALREADY || + errno == EINVAL) { + break; + } else { + perror("connect"); + closesocket(fd); + return -1; + } + } else { + connected = 1; + break; + } + } + s = net_socket_fd_init(peer, model, name, fd, connected); + if (!s) + 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)); return 0; - -err: - error_report_err(local_error); - socket_connect_data_free(c); - return -1; } static int net_socket_mcast_init(NetClientState *peer,
This reverts commit 883e4f7624e10b98d16d9adaffb8b1795664d899. This code changed net/socket.c from using socket()+connect(), to using socket_connect(). In theory this is great, but in practice this has completely broken the ability to connect the frontend and backend: $ ./x86_64-softmmu/qemu-system-x86_64 \ -device e1000,id=e0,netdev=hn0,mac=DE:AD:BE:EF:AF:05 \ -netdev socket,id=hn0,connect=localhost:1234 qemu-system-x86_64: -device e1000,id=e0,netdev=hn0,mac=DE:AD:BE:EF:AF:05: Property 'e1000.netdev' can't find value 'hn0' The old code would call net_socket_fd_init() synchronously, while letting the connect() complete in the backgorund. The new code moved net_socket_fd_init() so that it is only called after connect() completes in the background. Thus at the time we initialize the NIC frontend, the backend does not exist. The socket_connect() conversion as done is a bad fit for the current code, since it did not try to change the way it deals with async connection completion. Rather than try to fix this, just revert the socket_connect() conversion entirely. The code is about to be converted to use QIOChannel which will let the problem be solved in a cleaner manner. This revert is more suitable for stable branches in the meantime. Signed-off-by: Daniel P. Berrange <berrange@redhat.com> --- net/socket.c | 127 ++++++++++++++++++++++++++--------------------------------- 1 file changed, 56 insertions(+), 71 deletions(-)