mbox

[PULL,0/9] Socket next patches

Message ID 20180312201305.16972-1-berrange@redhat.com
State New
Headers show

Pull-request

https://github.com/berrange/qemu tags/socket-next-pull-request

Message

Daniel P. Berrangé March 12, 2018, 8:12 p.m. UTC 
The following changes since commit 819fd4699c7b36d574292bcbd8bc25e9d716c84b:

  Merge remote-tracking branch 'remotes/dgilbert/tags/pull-migration-20180309a' into staging (2018-03-12 13:21:53 +0000)

are available in the Git repository at:

  https://github.com/berrange/qemu tags/socket-next-pull-request

for you to fetch changes up to f4d2a296de2ccf5ff80ddd343c09f0075d10583a:

  char: allow passing pre-opened socket file descriptor at startup (2018-03-12 17:50:52 +0000)

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

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

Daniel P. Berrangé (9):
  char: don't silently skip tn3270 protocol init when TLS is enabled
  cutils: add qemu_strtoi & qemu_strtoui parsers for int/unsigned int
    types
  sockets: pull code for testing IP availability out of specific test
  sockets: strengthen test suite IP protocol availability checks
  sockets: move fd_is_socket() into common sockets code
  sockets: check that the named file descriptor is a socket
  sockets: allow SocketAddress 'fd' to reference numeric file
    descriptors
  char: refactor parsing of socket address information
  char: allow passing pre-opened socket file descriptor at startup

 chardev/char-socket.c          |  34 ++-
 chardev/char.c                 |   3 +
 include/qemu/cutils.h          |   4 +
 include/qemu/sockets.h         |   1 +
 io/channel-util.c              |  13 -
 qapi/sockets.json              |   7 +
 tests/.gitignore               |   1 +
 tests/Makefile.include         |   5 +-
 tests/socket-helpers.c         | 149 ++++++++++
 tests/socket-helpers.h         |  42 +++
 tests/test-char.c              |  47 ++-
 tests/test-cutils.c            | 657 +++++++++++++++++++++++++++++++++++++++++
 tests/test-io-channel-socket.c |  72 +----
 tests/test-util-sockets.c      | 266 +++++++++++++++++
 util/cutils.c                  | 108 +++++++
 util/qemu-sockets.c            |  36 ++-
 16 files changed, 1347 insertions(+), 98 deletions(-)
 create mode 100644 tests/socket-helpers.c
 create mode 100644 tests/socket-helpers.h
 create mode 100644 tests/test-util-sockets.c

Comments

no-reply@patchew.org March 12, 2018, 8:27 p.m. UTC | #1 
Hi,

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

Type: series
Message-id: 20180312201305.16972-1-berrange@redhat.com
Subject: [Qemu-devel] [PULL 0/9] Socket next patches

=== 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
git config --local diff.algorithm histogram

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
 * [new tag]               patchew/20180312201305.16972-1-berrange@redhat.com -> patchew/20180312201305.16972-1-berrange@redhat.com
Switched to a new branch 'test'
b48aef2db5 char: allow passing pre-opened socket file descriptor at startup
7acac469a5 char: refactor parsing of socket address information
52a971acae sockets: allow SocketAddress 'fd' to reference numeric file descriptors
3c42c553df sockets: check that the named file descriptor is a socket
fd55c60083 sockets: move fd_is_socket() into common sockets code
3c1dd42246 sockets: strengthen test suite IP protocol availability checks
56581333a1 sockets: pull code for testing IP availability out of specific test
1487a31959 cutils: add qemu_strtoi & qemu_strtoui parsers for int/unsigned int types
3623d8518e char: don't silently skip tn3270 protocol init when TLS is enabled

=== OUTPUT BEGIN ===
Checking PATCH 1/9: char: don't silently skip tn3270 protocol init when TLS is enabled...
Checking PATCH 2/9: cutils: add qemu_strtoi & qemu_strtoui parsers for int/unsigned int types...
ERROR: consider using qemu_strtol in preference to strtol
#753: FILE: util/cutils.c:338:
+    lresult = strtol(nptr, &ep, base);

ERROR: consider using qemu_strtoul in preference to strtoul
#805: FILE: util/cutils.c:390:
+    lresult = strtoul(nptr, &ep, base);

total: 2 errors, 0 warnings, 793 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.

Checking PATCH 3/9: sockets: pull code for testing IP availability out of specific test...
Checking PATCH 4/9: sockets: strengthen test suite IP protocol availability checks...
Checking PATCH 5/9: sockets: move fd_is_socket() into common sockets code...
Checking PATCH 6/9: sockets: check that the named file descriptor is a socket...
Checking PATCH 7/9: sockets: allow SocketAddress 'fd' to reference numeric file descriptors...
Checking PATCH 8/9: char: refactor parsing of socket address information...
Checking PATCH 9/9: char: allow passing pre-opened socket file descriptor at startup...
=== 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
Peter Maydell March 13, 2018, 4:20 p.m. UTC | #2 
On 12 March 2018 at 20:12, Daniel P. Berrangé <berrange@redhat.com> wrote:
> The following changes since commit 819fd4699c7b36d574292bcbd8bc25e9d716c84b:
>
>   Merge remote-tracking branch 'remotes/dgilbert/tags/pull-migration-20180309a' into staging (2018-03-12 13:21:53 +0000)
>
> are available in the Git repository at:
>
>   https://github.com/berrange/qemu tags/socket-next-pull-request
>
> for you to fetch changes up to f4d2a296de2ccf5ff80ddd343c09f0075d10583a:
>
>   char: allow passing pre-opened socket file descriptor at startup (2018-03-12 17:50:52 +0000)
>
> ----------------------------------------------------------------
>
> ----------------------------------------------------------------
>
> Daniel P. Berrangé (9):
>   char: don't silently skip tn3270 protocol init when TLS is enabled
>   cutils: add qemu_strtoi & qemu_strtoui parsers for int/unsigned int
>     types
>   sockets: pull code for testing IP availability out of specific test
>   sockets: strengthen test suite IP protocol availability checks
>   sockets: move fd_is_socket() into common sockets code
>   sockets: check that the named file descriptor is a socket
>   sockets: allow SocketAddress 'fd' to reference numeric file
>     descriptors
>   char: refactor parsing of socket address information
>   char: allow passing pre-opened socket file descriptor at startup
>

Test failure, 32-bit arm:

MALLOC_PERTURB_=${MALLOC_PERTURB_:-$(( ${RANDOM:-0} % 255 + 1))}
gtester -k --verbose -m=quick tests/test-c
utils
TEST: tests/test-cutils... (pid=21029)
  /cutils/parse_uint/null:                                             OK
[etc]
  /cutils/parse_uint_full/trailing:                                    **
ERROR:/home/peter.maydell/qemu/tests/test-cutils.c:715:test_qemu_strtoui_underflow:
assertion failed (err =
= -ERANGE): (0 == -34)
OK
  /cutils/parse_uint_full/correct:                                     OK

Test failure, x86-64 Linux:

TEST: tests/boot-serial-test... (pid=11205)
  /sparc64/boot-serial/sun4u:                                          **
ERROR:/home/petmay01/linaro/qemu-for-merges/tests/boot-serial-test.c:137:check_guest_output:
assertion failed: (output_ok)
FAIL
GTester: last random seed: R02S99166ae475faeb3280608eeed6d61c4f
(pid=15216)

Test failure, 64-bit arm:

ERROR:/home/pm215/qemu/tests/test-aio-multithread.c:365:test_multi_fair_mutex:
assertion failed (counter ==
 atomic_counter): (59827 == 59828)

thanks
-- PMM
Daniel P. Berrangé March 13, 2018, 4:27 p.m. UTC | #3 
On Tue, Mar 13, 2018 at 04:20:19PM +0000, Peter Maydell wrote:
> On 12 March 2018 at 20:12, Daniel P. Berrangé <berrange@redhat.com> wrote:
> > The following changes since commit 819fd4699c7b36d574292bcbd8bc25e9d716c84b:
> >
> >   Merge remote-tracking branch 'remotes/dgilbert/tags/pull-migration-20180309a' into staging (2018-03-12 13:21:53 +0000)
> >
> > are available in the Git repository at:
> >
> >   https://github.com/berrange/qemu tags/socket-next-pull-request
> >
> > for you to fetch changes up to f4d2a296de2ccf5ff80ddd343c09f0075d10583a:
> >
> >   char: allow passing pre-opened socket file descriptor at startup (2018-03-12 17:50:52 +0000)
> >
> > ----------------------------------------------------------------
> >
> > ----------------------------------------------------------------
> >
> > Daniel P. Berrangé (9):
> >   char: don't silently skip tn3270 protocol init when TLS is enabled
> >   cutils: add qemu_strtoi & qemu_strtoui parsers for int/unsigned int
> >     types
> >   sockets: pull code for testing IP availability out of specific test
> >   sockets: strengthen test suite IP protocol availability checks
> >   sockets: move fd_is_socket() into common sockets code
> >   sockets: check that the named file descriptor is a socket
> >   sockets: allow SocketAddress 'fd' to reference numeric file
> >     descriptors
> >   char: refactor parsing of socket address information
> >   char: allow passing pre-opened socket file descriptor at startup
> >
> 
> Test failure, 32-bit arm:
> 
> MALLOC_PERTURB_=${MALLOC_PERTURB_:-$(( ${RANDOM:-0} % 255 + 1))}
> gtester -k --verbose -m=quick tests/test-c
> utils
> TEST: tests/test-cutils... (pid=21029)
>   /cutils/parse_uint/null:                                             OK
> [etc]
>   /cutils/parse_uint_full/trailing:                                    **
> ERROR:/home/peter.maydell/qemu/tests/test-cutils.c:715:test_qemu_strtoui_underflow:
> assertion failed (err =
> = -ERANGE): (0 == -34)

This will be a genuine bug. Not sure if its in the test case or code
yet....

> OK
>   /cutils/parse_uint_full/correct:                                     OK
> 
> Test failure, x86-64 Linux:
> 
> TEST: tests/boot-serial-test... (pid=11205)
>   /sparc64/boot-serial/sun4u:                                          **
> ERROR:/home/petmay01/linaro/qemu-for-merges/tests/boot-serial-test.c:137:check_guest_output:
> assertion failed: (output_ok)
> FAIL
> GTester: last random seed: R02S99166ae475faeb3280608eeed6d61c4f
> (pid=15216)

This feels unrelated to the series, so possibly a non-deterministic
failure

> 
> Test failure, 64-bit arm:
> 
> ERROR:/home/pm215/qemu/tests/test-aio-multithread.c:365:test_multi_fair_mutex:
> assertion failed (counter ==
>  atomic_counter): (59827 == 59828)

I'm sure I've seen this non-deterministic failure before, unrelated to
this series.

Regards,
Daniel
Daniel P. Berrangé March 13, 2018, 6:11 p.m. UTC | #4 
On Mon, Mar 12, 2018 at 08:12:58PM +0000, Daniel P. Berrangé wrote:
> From: "Daniel P. Berrange" <berrange@redhat.com>
> 
> There are qemu_strtoNN functions for various sized integers. This adds two
> more for plain int & unsigned int types, with suitable range checking.
> 
> Reviewed-by: Eric Blake <eblake@redhat.com>
> Reviewed-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> Signed-off-by: Daniel P. Berrange <berrange@redhat.com>
> diff --git a/util/cutils.c b/util/cutils.c
> index b33ede83d1..774d5f7362 100644
> --- a/util/cutils.c
> +++ b/util/cutils.c
> +int qemu_strtoui(const char *nptr, const char **endptr, int base,
> +                 unsigned int *result)
> +{
> +    char *ep;
> +    long lresult;
> +
> +    if (!nptr) {
> +        if (endptr) {
> +            *endptr = nptr;
> +        }
> +        return -EINVAL;
> +    }
> +
> +    errno = 0;
> +    lresult = strtoul(nptr, &ep, base);

The test failure on 32-bit is caused by use of strtoul instead
of strtoull here, so I'll just switch them, so the logic that
follows below actually works on 32-bit.

> +    /* Windows returns 1 for negative out-of-range values.  */
> +    if (errno == ERANGE) {
> +        *result = -1;
> +    } else {
> +        if (lresult > UINT_MAX) {
> +            *result = UINT_MAX;
> +            errno = ERANGE;
> +        } else if (lresult < INT_MIN) {
> +            *result = UINT_MAX;
> +            errno = ERANGE;
> +        } else {
> +            *result = lresult;
> +        }
> +    }
> +    return check_strtox_error(nptr, ep, endptr, errno);
> +}
> +
>  /**
>   * Convert string @nptr to a long integer, and store it in @result.
>   *
> -- 
> 2.14.3
> 

Regards,
Daniel
Daniel P. Berrangé March 13, 2018, 6:23 p.m. UTC | #5 
On Tue, Mar 13, 2018 at 04:27:08PM +0000, Daniel P. Berrangé wrote:
> On Tue, Mar 13, 2018 at 04:20:19PM +0000, Peter Maydell wrote:
> > On 12 March 2018 at 20:12, Daniel P. Berrangé <berrange@redhat.com> wrote:
> > > The following changes since commit 819fd4699c7b36d574292bcbd8bc25e9d716c84b:
> > >
> > >   Merge remote-tracking branch 'remotes/dgilbert/tags/pull-migration-20180309a' into staging (2018-03-12 13:21:53 +0000)
> > >
> > > are available in the Git repository at:
> > >
> > >   https://github.com/berrange/qemu tags/socket-next-pull-request
> > >
> > > for you to fetch changes up to f4d2a296de2ccf5ff80ddd343c09f0075d10583a:
> > >
> > >   char: allow passing pre-opened socket file descriptor at startup (2018-03-12 17:50:52 +0000)
> > >
> > > ----------------------------------------------------------------
> > >
> > > ----------------------------------------------------------------
> > >
> > > Daniel P. Berrangé (9):
> > >   char: don't silently skip tn3270 protocol init when TLS is enabled
> > >   cutils: add qemu_strtoi & qemu_strtoui parsers for int/unsigned int
> > >     types
> > >   sockets: pull code for testing IP availability out of specific test
> > >   sockets: strengthen test suite IP protocol availability checks
> > >   sockets: move fd_is_socket() into common sockets code
> > >   sockets: check that the named file descriptor is a socket
> > >   sockets: allow SocketAddress 'fd' to reference numeric file
> > >     descriptors
> > >   char: refactor parsing of socket address information
> > >   char: allow passing pre-opened socket file descriptor at startup
> > >
> > 
> > Test failure, 32-bit arm:
> > 
> > MALLOC_PERTURB_=${MALLOC_PERTURB_:-$(( ${RANDOM:-0} % 255 + 1))}
> > gtester -k --verbose -m=quick tests/test-c
> > utils
> > TEST: tests/test-cutils... (pid=21029)
> >   /cutils/parse_uint/null:                                             OK
> > [etc]
> >   /cutils/parse_uint_full/trailing:                                    **
> > ERROR:/home/peter.maydell/qemu/tests/test-cutils.c:715:test_qemu_strtoui_underflow:
> > assertion failed (err =
> > = -ERANGE): (0 == -34)
> 
> This will be a genuine bug. Not sure if its in the test case or code
> yet....
> 
> > OK
> >   /cutils/parse_uint_full/correct:                                     OK
> > 
> > Test failure, x86-64 Linux:
> > 
> > TEST: tests/boot-serial-test... (pid=11205)
> >   /sparc64/boot-serial/sun4u:                                          **
> > ERROR:/home/petmay01/linaro/qemu-for-merges/tests/boot-serial-test.c:137:check_guest_output:
> > assertion failed: (output_ok)
> > FAIL
> > GTester: last random seed: R02S99166ae475faeb3280608eeed6d61c4f
> > (pid=15216)
> 
> This feels unrelated to the series, so possibly a non-deterministic
> failure

Looking at the test source code, it has a 60 second wait for the
emulator to print expected data on the serial port. My guess
would be that when running with a highly parallel make check
we're sometimes exceeding that 60 seconds. I don't see a
reason why my series would have affected this in a way that
only failed with sparc64 qtest, and none others.

> 
> > 
> > Test failure, 64-bit arm:
> > 
> > ERROR:/home/pm215/qemu/tests/test-aio-multithread.c:365:test_multi_fair_mutex:
> > assertion failed (counter ==
> >  atomic_counter): (59827 == 59828)
> 
> I'm sure I've seen this non-deterministic failure before, unrelated to
> this series.

Regards,
Daniel
Peter Maydell March 13, 2018, 6:26 p.m. UTC | #6 
On 13 March 2018 at 18:23, Daniel P. Berrangé <berrange@redhat.com> wrote:
> On Tue, Mar 13, 2018 at 04:27:08PM +0000, Daniel P. Berrangé wrote:
>> This feels unrelated to the series, so possibly a non-deterministic
>> failure
>
> Looking at the test source code, it has a 60 second wait for the
> emulator to print expected data on the serial port. My guess
> would be that when running with a highly parallel make check
> we're sometimes exceeding that 60 seconds. I don't see a
> reason why my series would have affected this in a way that
> only failed with sparc64 qtest, and none others.

Sounds plausible. Feel free to resubmit the pullreq with the
actual bug fixed, for another spin of the intermittent-failure
roulette wheel :-)

-- PMM
Kamil Rytarowski March 13, 2018, 7:10 p.m. UTC | #7 
On 12.03.2018 21:12, Daniel P. Berrangé wrote:
> From: "Daniel P. Berrange" <berrange@redhat.com>
> 
> There are qemu_strtoNN functions for various sized integers. This adds two
> more for plain int & unsigned int types, with suitable range checking.
> 

There is a prior art in NetBSD with strtoi(3) and strtou(3).

http://netbsd.gw.com/cgi-bin/man-cgi?strtoi++NetBSD-current
http://netbsd.gw.com/cgi-bin/man-cgi?strtou++NetBSD-current

This is a clean room alternative for strtonum(3) with compat with some
other OSes.

The original code is in:

https://github.com/NetBSD/src/blob/trunk/common/lib/libc/stdlib/

It uses a wrapper for regular strtol(3)-like functions and strtol_l(3).

A simpler strtoi(3) wrapper is used in dhcpcd:

https://github.com/NetBSD/src/blob/trunk/external/bsd/dhcpcd/dist/compat/

It has one difference compared to new POSIX (release 2016 or so) - no
conversion at all is ECANCELED, not EINVAL. I think ECANCELED makes more
sense and is distinguishable with input base error, and should be preserved.