[Qemu-commits] [qemu/qemu] 9ce22d: test-util-sockets: Plug file descriptor leak

[Peter Maydell]

  Branch: refs/heads/master
  Home:   https://github.com/qemu/qemu
  Commit: 9ce22da0d834f0c9f57bd36f5d0d10e5e2f4992c
      
https://github.com/qemu/qemu/commit/9ce22da0d834f0c9f57bd36f5d0d10e5e2f4992c
  Author: Markus Armbruster <armbru@redhat.com>
  Date:   2020-11-03 (Tue, 03 Nov 2020)

  Changed paths:
    M tests/test-util-sockets.c

  Log Message:
  -----------
  test-util-sockets: Plug file descriptor leak

Fixes: 4d3a329af59ef8acd076f99f05e82531d8129b34
Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>
Reviewed-by: Paolo Bonzini <pbonzini@redhat.com>
Reviewed-by: Eric Blake <eblake@redhat.com>
Signed-off-by: Markus Armbruster <armbru@redhat.com>
Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>


  Commit: d1a393211b5333f9374b439394424f594f69d282
      
https://github.com/qemu/qemu/commit/d1a393211b5333f9374b439394424f594f69d282
  Author: Markus Armbruster <armbru@redhat.com>
  Date:   2020-11-03 (Tue, 03 Nov 2020)

  Changed paths:
    M tests/test-util-sockets.c

  Log Message:
  -----------
  test-util-sockets: Correct to set has_abstract, has_tight

The code tested doesn't care, which is a bug I will fix shortly.

Reviewed-by: Paolo Bonzini <pbonzini@redhat.com>
Reviewed-by: Eric Blake <eblake@redhat.com>
Signed-off-by: Markus Armbruster <armbru@redhat.com>
Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>


  Commit: 718a9be02df880ca4b4e34ce253daf2bfc5d059c
      
https://github.com/qemu/qemu/commit/718a9be02df880ca4b4e34ce253daf2bfc5d059c
  Author: Markus Armbruster <armbru@redhat.com>
  Date:   2020-11-03 (Tue, 03 Nov 2020)

  Changed paths:
    M tests/test-util-sockets.c

  Log Message:
  -----------
  test-util-sockets: Clean up SocketAddress construction

The thread functions build the SocketAddress from global variable
@abstract_sock_name and the tight flag passed as pointer
argument (either NULL or (gpointer)1).  There is no need for such
hackery; simply pass the SocketAddress instead.

While there, dumb down g_rand_int_range() to g_random_int().

Reviewed-by: Paolo Bonzini <pbonzini@redhat.com>
Reviewed-by: Eric Blake <eblake@redhat.com>
Signed-off-by: Markus Armbruster <armbru@redhat.com>
Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>


  Commit: 89cb0bb554ee2365d948d3f593ea04f03d5bc4f8
      
https://github.com/qemu/qemu/commit/89cb0bb554ee2365d948d3f593ea04f03d5bc4f8
  Author: Markus Armbruster <armbru@redhat.com>
  Date:   2020-11-03 (Tue, 03 Nov 2020)

  Changed paths:
    M tests/test-util-sockets.c

  Log Message:
  -----------
  test-util-sockets: Factor out test_socket_unix_abstract_one()

Reviewed-by: Paolo Bonzini <pbonzini@redhat.com>
Reviewed-by: Eric Blake <eblake@redhat.com>
Signed-off-by: Markus Armbruster <armbru@redhat.com>
Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>


  Commit: 39458d4e3059d37e3331258a50fd77f8cf5b365e
      
https://github.com/qemu/qemu/commit/39458d4e3059d37e3331258a50fd77f8cf5b365e
  Author: Markus Armbruster <armbru@redhat.com>
  Date:   2020-11-03 (Tue, 03 Nov 2020)

  Changed paths:
    M tests/test-util-sockets.c

  Log Message:
  -----------
  test-util-sockets: Synchronize properly, don't sleep(1)

The abstract sockets test spawns a thread to listen and accept, and a
second one to connect, with a sleep(1) in between to "ensure" the
former is listening when the latter tries to connect.  Review fail.
Risks spurious test failure, say when a heavily loaded machine doesn't
schedule the first thread quickly enough.  It's also slow.

Listen and accept in the main thread, and start the connect thread in
between.  Look ma, no sleep!  Run time drops from 2s wall clock to a
few milliseconds.

Reviewed-by: Paolo Bonzini <pbonzini@redhat.com>
Reviewed-by: Eric Blake <eblake@redhat.com>
Signed-off-by: Markus Armbruster <armbru@redhat.com>
Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>


  Commit: a72f6754a10ea4f4bf76e83ecaa7f82931991c24
      
https://github.com/qemu/qemu/commit/a72f6754a10ea4f4bf76e83ecaa7f82931991c24
  Author: Markus Armbruster <armbru@redhat.com>
  Date:   2020-11-03 (Tue, 03 Nov 2020)

  Changed paths:
    M tests/test-util-sockets.c

  Log Message:
  -----------
  test-util-sockets: Test the complete abstract socket matrix

The test covers only two out of nine combinations.  Test all nine.
Four turn out to be broken.  Marked /* BUG */.

Reviewed-by: Paolo Bonzini <pbonzini@redhat.com>
Reviewed-by: Eric Blake <eblake@redhat.com>
Signed-off-by: Markus Armbruster <armbru@redhat.com>
Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>


  Commit: b08cc97d6ba4250439829a8a1d476064a1cb54da
      
https://github.com/qemu/qemu/commit/b08cc97d6ba4250439829a8a1d476064a1cb54da
  Author: Markus Armbruster <armbru@redhat.com>
  Date:   2020-11-03 (Tue, 03 Nov 2020)

  Changed paths:
    M chardev/char-socket.c
    M tests/test-util-sockets.c
    M util/qemu-sockets.c

  Log Message:
  -----------
  sockets: Fix default of UnixSocketAddress member @tight

An optional bool member of a QAPI struct can be false, true, or absent.
The previous commit demonstrated that socket_listen() and
socket_connect() are broken for absent @tight, and indeed QMP chardev-
add also defaults absent member @tight to false instead of true.

In C, QAPI members are represented by two fields, has_MEMBER and MEMBER.
We have:

            has_MEMBER    MEMBER
    false         true     false
    true          true      true
    absent       false  false/ignore

When has_MEMBER is false, MEMBER should be set to false on write, and
ignored on read.

For QMP, the QAPI visitors handle absent @tight by setting both
@has_tight and @tight to false.  unix_listen_saddr() and
unix_connect_saddr() however use @tight only, disregarding @has_tight.
This is wrong and means that absent @tight defaults to false whereas it
should default to true.

The same is true for @has_abstract, though @abstract defaults to
false and therefore has the same behavior for all of QMP, HMP and CLI.
Fix unix_listen_saddr() and unix_connect_saddr() to check
@has_abstract/@has_tight, and to default absent @tight to true.

However, this is only half of the story.  HMP chardev-add and CLI
-chardev so far correctly defaulted @tight to true, but defaults to
false again with the above fix for HMP and CLI.  In fact, the "tight"
and "abstract" options now break completely.

Digging deeper, we find that qemu_chr_parse_socket() also ignores
@has_tight, leaving it false when it sets @tight.  That is also wrong,
but the two wrongs cancelled out.  Fix qemu_chr_parse_socket() to set
@has_tight and @has_abstract; writing testcases for HMP and CLI is left
for another day.

Fixes: 776b97d3605ed0fc94443048fdf988c7725e38a9
Reported-by: Kevin Wolf <kwolf@redhat.com>
Reviewed-by: Paolo Bonzini <pbonzini@redhat.com>
Reviewed-by: Eric Blake <eblake@redhat.com>
Signed-off-by: Markus Armbruster <armbru@redhat.com>
Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>


  Commit: 3b14b4ec49a801067da19d6b8469eb1c1911c020
      
https://github.com/qemu/qemu/commit/3b14b4ec49a801067da19d6b8469eb1c1911c020
  Author: Markus Armbruster <armbru@redhat.com>
  Date:   2020-11-03 (Tue, 03 Nov 2020)

  Changed paths:
    M util/qemu-sockets.c

  Log Message:
  -----------
  sockets: Fix socket_sockaddr_to_address_unix() for abstract sockets

Commit 776b97d360 "qemu-sockets: add abstract UNIX domain socket
support" neglected to update socket_sockaddr_to_address_unix().  The
function returns a non-abstract socket address for abstract
sockets (wrong) with a null @path (also wrong; a non-optional QAPI str
member must never be null).

The null @path is due to confused code going back all the way to
commit 17c55decec "sockets: add helpers for creating SocketAddress
from a socket".

Add the required special case, and simplify the confused code.

Fixes: 776b97d3605ed0fc94443048fdf988c7725e38a9
Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>
Reviewed-by: Paolo Bonzini <pbonzini@redhat.com>
Signed-off-by: Markus Armbruster <armbru@redhat.com>
Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>


  Commit: dea7cd1794f33c52e4b59fe085daffb318a4bb07
      
https://github.com/qemu/qemu/commit/dea7cd1794f33c52e4b59fe085daffb318a4bb07
  Author: Markus Armbruster <armbru@redhat.com>
  Date:   2020-11-03 (Tue, 03 Nov 2020)

  Changed paths:
    M chardev/char-socket.c

  Log Message:
  -----------
  char-socket: Fix qemu_chr_socket_address() for abstract sockets

Commit 776b97d360 "qemu-sockets: add abstract UNIX domain socket
support" neglected to update qemu_chr_socket_address().  It shows
shows neither @abstract nor @tight.  Fix that.

Reviewed-by: Paolo Bonzini <pbonzini@redhat.com>
Signed-off-by: Markus Armbruster <armbru@redhat.com>
Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>


  Commit: ef298e3826e574c712d10e38a5f2a3629d6f5e01
      
https://github.com/qemu/qemu/commit/ef298e3826e574c712d10e38a5f2a3629d6f5e01
  Author: Markus Armbruster <armbru@redhat.com>
  Date:   2020-11-03 (Tue, 03 Nov 2020)

  Changed paths:
    M util/qemu-sockets.c

  Log Message:
  -----------
  sockets: Bypass "replace empty @path" for abstract unix sockets

unix_listen_saddr() replaces empty @path by unique value.  It obtains
the value by creating and deleting a unique temporary file with
mkstemp().  This is racy, as the comment explains.  It's also entirely
undocumented as far as I can tell.  Goes back to commit d247d25f18
"sockets: helper functions for qemu (Gerd Hoffman)", v0.10.0.

Since abstract socket addresses have no connection with filesystem
pathnames, making them up with mkstemp() seems inappropriate.  Bypass
the replacement of empty @path.

Reviewed-by: Eric Blake <eblake@redhat.com>
Reviewed-by: Paolo Bonzini <pbonzini@redhat.com>
Signed-off-by: Markus Armbruster <armbru@redhat.com>
Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>


  Commit: 8acefc79deaab1c7ee2ab07b540b0e3edf0f9f47
      
https://github.com/qemu/qemu/commit/8acefc79deaab1c7ee2ab07b540b0e3edf0f9f47
  Author: Markus Armbruster <armbru@redhat.com>
  Date:   2020-11-03 (Tue, 03 Nov 2020)

  Changed paths:
    M chardev/char-socket.c
    M chardev/char.c
    M qapi/sockets.json
    M tests/test-util-sockets.c
    M util/qemu-sockets.c

  Log Message:
  -----------
  sockets: Make abstract UnixSocketAddress depend on CONFIG_LINUX

The abstract socket namespace is a non-portable Linux extension.  An
attempt to use it elsewhere should fail with ENOENT (the abstract
address looks like a "" pathname, which does not resolve).  We report
this failure like

    Failed to connect socket abc: No such file or directory

Tolerable, although ENOTSUP would be better.

However, introspection lies: it has @abstract regardless of host
support.  Easy enough to fix: since Linux provides them since 2.2,
'if': 'defined(CONFIG_LINUX)' should do.

The above failure becomes

    Parameter 'backend.data.addr.data.abstract' is unexpected

I consider this an improvement.

Reviewed-by: Paolo Bonzini <pbonzini@redhat.com>
Signed-off-by: Markus Armbruster <armbru@redhat.com>
Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>


  Commit: ad262888993f795db68fd7c2bdfa72f467fe0096
      
https://github.com/qemu/qemu/commit/ad262888993f795db68fd7c2bdfa72f467fe0096
  Author: Peter Maydell <peter.maydell@linaro.org>
  Date:   2020-11-03 (Tue, 03 Nov 2020)

  Changed paths:
    M chardev/char-socket.c
    M chardev/char.c
    M qapi/sockets.json
    M tests/test-util-sockets.c
    M util/qemu-sockets.c

  Log Message:
  -----------
  Merge remote-tracking branch 
'remotes/berrange-gitlab/tags/sock-next-pull-request' into staging

 - Fix inverted logic in abstract socket QAPI support
 - Only report abstract socket support in QAPI on Linux hosts
 - Expand test coverage
 - Misc other code cleanups

# gpg: Signature made Tue 03 Nov 2020 14:00:53 GMT
# gpg:                using RSA key DAF3A6FDB26B62912D0E8E3FBE86EBB415104FDF
# gpg: Good signature from "Daniel P. Berrange <dan@berrange.com>" [full]
# gpg:                 aka "Daniel P. Berrange <berrange@redhat.com>" [full]
# Primary key fingerprint: DAF3 A6FD B26B 6291 2D0E  8E3F BE86 EBB4 1510 4FDF

* remotes/berrange-gitlab/tags/sock-next-pull-request:
  sockets: Make abstract UnixSocketAddress depend on CONFIG_LINUX
  sockets: Bypass "replace empty @path" for abstract unix sockets
  char-socket: Fix qemu_chr_socket_address() for abstract sockets
  sockets: Fix socket_sockaddr_to_address_unix() for abstract sockets
  sockets: Fix default of UnixSocketAddress member @tight
  test-util-sockets: Test the complete abstract socket matrix
  test-util-sockets: Synchronize properly, don't sleep(1)
  test-util-sockets: Factor out test_socket_unix_abstract_one()
  test-util-sockets: Clean up SocketAddress construction
  test-util-sockets: Correct to set has_abstract, has_tight
  test-util-sockets: Plug file descriptor leak

Signed-off-by: Peter Maydell <peter.maydell@linaro.org>


Compare: https://github.com/qemu/qemu/compare/83851c7c60c9...ad262888993f