[Qemu-devel] [PULL 0/2] Net patches

[Qemu-devel] [PULL 0/2] Net patches

[Jason Wang]

The following changes since commit 199e19ee538eb61fd08b1c1ee5aa838ebdcc968e:

  Merge remote-tracking branch 'remotes/mjt/tags/trivial-patches-fetch' into staging (2017-06-05 15:28:12 +0100)

are available in the git repository at:

  https://github.com/jasowang/qemu.git tags/net-pull-request

for you to fetch changes up to 6701e5514beab7b781a10424a94e9850c707287c:

  Revert "Change net/socket.c to use socket_*() functions" again (2017-06-07 10:58:31 +0800)

----------------------------------------------------------------

----------------------------------------------------------------
Daniel P. Berrange (1):
      Revert "Change net/socket.c to use socket_*() functions" again

Mao Zhongyi (1):
      net/rocker: Cleanup the useless return value check

 hw/net/rocker/rocker.c      |   9 ++--
 hw/net/rocker/rocker_desc.c |   4 +-
 net/socket.c                | 127 +++++++++++++++++++-------------------------
 3 files changed, 60 insertions(+), 80 deletions(-)

[Qemu-devel] [PULL 1/2] net/rocker: Cleanup the useless return value check

[Jason Wang]

From: Mao Zhongyi <maozy.fnst@cn.fujitsu.com>

None of pci_dma_read()'s callers check the return value except
rocker. There is no need to check it because it always return
0. So the check work is useless. Remove it entirely.

Suggested-by: Markus Armbruster <armbru@redhat.com>
Signed-off-by: Mao Zhongyi <maozy.fnst@cn.fujitsu.com>
Signed-off-by: Jason Wang <jasowang@redhat.com>
---
 hw/net/rocker/rocker.c      | 9 +++------
 hw/net/rocker/rocker_desc.c | 4 +---
 2 files changed, 4 insertions(+), 9 deletions(-)

diff --git a/hw/net/rocker/rocker.c b/hw/net/rocker/rocker.c
index 6e70fdd..4f0f6d7 100644
--- a/hw/net/rocker/rocker.c
+++ b/hw/net/rocker/rocker.c
@@ -244,11 +244,9 @@ static int tx_consume(Rocker *r, DescInfo *info)
             goto err_no_mem;
         }
 
-        if (pci_dma_read(dev, frag_addr, iov[iovcnt].iov_base,
-                     iov[iovcnt].iov_len)) {
-            err = -ROCKER_ENXIO;
-            goto err_bad_io;
-        }
+        pci_dma_read(dev, frag_addr, iov[iovcnt].iov_base,
+                     iov[iovcnt].iov_len);
+
         iovcnt++;
     }
 
@@ -261,7 +259,6 @@ static int tx_consume(Rocker *r, DescInfo *info)
     err = fp_port_eg(r->fp_port[port], iov, iovcnt);
 
 err_too_many_frags:
-err_bad_io:
 err_no_mem:
 err_bad_attr:
     for (i = 0; i < ROCKER_TX_FRAGS_MAX; i++) {
diff --git a/hw/net/rocker/rocker_desc.c b/hw/net/rocker/rocker_desc.c
index ac02797..6184c40 100644
--- a/hw/net/rocker/rocker_desc.c
+++ b/hw/net/rocker/rocker_desc.c
@@ -69,9 +69,7 @@ char *desc_get_buf(DescInfo *info, bool read_only)
         return NULL;
     }
 
-    if (pci_dma_read(dev, le64_to_cpu(info->desc.buf_addr), info->buf, size)) {
-        return NULL;
-    }
+    pci_dma_read(dev, le64_to_cpu(info->desc.buf_addr), info->buf, size);
 
     return info->buf;
 }
-- 
2.7.4

[Qemu-devel] [PULL 2/2] Revert "Change net/socket.c to use socket_*() functions" again

[Jason Wang]

From: "Daniel P. Berrange" <berrange@redhat.com>

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>
Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>
Signed-off-by: Jason Wang <jasowang@redhat.com>
---
 net/socket.c | 127 ++++++++++++++++++++++++++---------------------------------
 1 file changed, 56 insertions(+), 71 deletions(-)

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,
-- 
2.7.4

Re: [Qemu-devel] [PULL 0/2] Net patches

[no-reply]

Hi,

This series seems to have some coding style problems. See output below for
more information:

Type: series
Subject: [Qemu-devel] [PULL 0/2] Net patches
Message-id: 1496806444-27910-1-git-send-email-jasowang@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
Switched to a new branch 'test'
a0be159 Revert "Change net/socket.c to use socket_*() functions" again
0e3dcc6 net/rocker: Cleanup the useless return value check

=== OUTPUT BEGIN ===
Checking PATCH 1/2: net/rocker: Cleanup the useless return value check...
Checking PATCH 2/2: Revert "Change net/socket.c to use socket_*() functions" again...
ERROR: braces {} are necessary for all arms of this statement
#58: FILE: net/socket.c:495:
+    if (parse_host_port(&saddr, host_str) < 0)
[...]

ERROR: braces {} are necessary for all arms of this statement
#161: FILE: net/socket.c:540:
+    if (parse_host_port(&saddr, host_str) < 0)
[...]

ERROR: space required before the open parenthesis '('
#174: FILE: net/socket.c:551:
+    for(;;) {

ERROR: braces {} are necessary for all arms of this statement
#194: 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

Re: [Qemu-devel] [PULL 0/2] Net patches

[Jason Wang]

On 2017年06月07日 12:31, 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] [PULL 0/2] Net patches
> Message-id: 1496806444-27910-1-git-send-email-jasowang@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
> Switched to a new branch 'test'
> a0be159 Revert "Change net/socket.c to use socket_*() functions" again
> 0e3dcc6 net/rocker: Cleanup the useless return value check
>
> === OUTPUT BEGIN ===
> Checking PATCH 1/2: net/rocker: Cleanup the useless return value check...
> Checking PATCH 2/2: Revert "Change net/socket.c to use socket_*() functions" again...
> ERROR: braces {} are necessary for all arms of this statement
> #58: FILE: net/socket.c:495:
> +    if (parse_host_port(&saddr, host_str) < 0)
> [...]
>
> ERROR: braces {} are necessary for all arms of this statement
> #161: FILE: net/socket.c:540:
> +    if (parse_host_port(&saddr, host_str) < 0)
> [...]
>
> ERROR: space required before the open parenthesis '('
> #174: FILE: net/socket.c:551:
> +    for(;;) {
>
> ERROR: braces {} are necessary for all arms of this statement
> #194: 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

This patch is a revert, so looks like it was a false positive.

Thanks

Re: [Qemu-devel] [PULL 0/2] Net patches

[Peter Maydell]

On 7 June 2017 at 04:34, Jason Wang <jasowang@redhat.com> wrote:
> The following changes since commit 199e19ee538eb61fd08b1c1ee5aa838ebdcc968e:
>
>   Merge remote-tracking branch 'remotes/mjt/tags/trivial-patches-fetch' into staging (2017-06-05 15:28:12 +0100)
>
> are available in the git repository at:
>
>   https://github.com/jasowang/qemu.git tags/net-pull-request
>
> for you to fetch changes up to 6701e5514beab7b781a10424a94e9850c707287c:
>
>   Revert "Change net/socket.c to use socket_*() functions" again (2017-06-07 10:58:31 +0800)
>
> ----------------------------------------------------------------
>
> ----------------------------------------------------------------
> Daniel P. Berrange (1):
>       Revert "Change net/socket.c to use socket_*() functions" again
>
> Mao Zhongyi (1):
>       net/rocker: Cleanup the useless return value check
>
>  hw/net/rocker/rocker.c      |   9 ++--
>  hw/net/rocker/rocker_desc.c |   4 +-
>  net/socket.c                | 127 +++++++++++++++++++-------------------------
>  3 files changed, 60 insertions(+), 80 deletions(-)
>
Applied, thanks.

-- PMM