[PATCH v2] net: enhance decoding of getsockopt(SO_ERROR)
Dmitry V. Levin
ldv at altlinux.org
Mon Dec 17 16:48:52 UTC 2018
On Sun, Dec 16, 2018 at 06:40:56PM +0900, Masatake YAMATO wrote:
> * net.c (print_get_error): New function decoding error
> number returned as option value for SO_ERROR option.
> (print_getsockopt) <case SO_ERROR>: Call print_get_error.
>
> * tests/so_error.c: New test.
> * tests/gen_tests.in (so_error): Likewise.
> * tests/pure_executables.list (so_error): Likewise.
Strictly speaking, pure_executables.list lists executables, so please
change to
* tests/pure_executables.list: Add so_error.
Also please add so_error to tests/.gitignore file.
>
> Change in v2:
> * Use print_xlat_ex to print errno.
> Suggested by ldv.
>
> Signed-off-by: Masatake YAMATO <yamato at redhat.com>
> ---
> net.c | 21 +++++++
> tests/gen_tests.in | 1 +
> tests/pure_executables.list | 1 +
> tests/so_error.c | 121 ++++++++++++++++++++++++++++++++++++
> 4 files changed, 144 insertions(+)
> create mode 100644 tests/so_error.c
>
> diff --git a/net.c b/net.c
> index f65b6619..209275af 100644
> --- a/net.c
> +++ b/net.c
> @@ -671,6 +671,24 @@ print_get_ucred(struct tcb *const tcp, const kernel_ulong_t addr,
> tprints("}");
> }
>
> +static void
> +print_get_error(struct tcb *const tcp, const kernel_ulong_t addr,
> + unsigned int len)
> +{
> + unsigned int err;
> + const char *ename;
net.c: In function 'print_get_error':
net.c:659:14: error: unused variable 'ename' [-Werror=unused-variable]
> +
> + if (len > sizeof(err))
> + err = sizeof(err);
> +
> + if (umoven_or_printaddr(tcp, addr, len, &err))
> + return;
> +
> + tprints("[");
> + print_xlat_ex(err, err_name(err), XLAT_STYLE_FMT_U);
> + tprints("]");
> +}
> +
> #ifdef PACKET_STATISTICS
> static void
> print_tpacket_stats(struct tcb *const tcp, const kernel_ulong_t addr,
> @@ -786,6 +804,9 @@ print_getsockopt(struct tcb *const tcp, const unsigned int level,
> else
> printaddr(addr);
> return;
> + case SO_ERROR:
> + print_get_error(tcp, addr, rlen);
> + return;
> }
> break;
>
> diff --git a/tests/gen_tests.in b/tests/gen_tests.in
> index 7374add8..d6b4a068 100644
> --- a/tests/gen_tests.in
> +++ b/tests/gen_tests.in
> @@ -431,6 +431,7 @@ sigpending -a15
> sigprocmask -a34
> sigreturn -esignal='!USR1'
> sigsuspend -a19 -esignal=none
> +so_error -e trace=getsockopt
> so_linger -e trace=getsockopt,setsockopt
> so_peercred -e trace=getsockopt
> so_peercred-Xabbrev -e trace=getsockopt -Xabbrev
> diff --git a/tests/pure_executables.list b/tests/pure_executables.list
> index a4e9020a..53adb99f 100755
> --- a/tests/pure_executables.list
> +++ b/tests/pure_executables.list
> @@ -410,6 +410,7 @@ sigpending
> sigprocmask
> sigreturn
> sigsuspend
> +so_error
> so_linger
> so_peercred
> so_peercred-Xabbrev
> diff --git a/tests/so_error.c b/tests/so_error.c
> new file mode 100644
> index 00000000..85d2a889
> --- /dev/null
> +++ b/tests/so_error.c
> @@ -0,0 +1,121 @@
> +/*
> + * Check decoding of SO_ERROR socket option.
> + *
> + * Copyright (c) 2018 Masatake YAMATO <yamato at redhat.com>
> + * All rights reserved.
> + *
> + * Redistribution and use in source and binary forms, with or without
> + * modification, are permitted provided that the following conditions
> + * are met:
> + * 1. Redistributions of source code must retain the above copyright
> + * notice, this list of conditions and the following disclaimer.
> + * 2. Redistributions in binary form must reproduce the above copyright
> + * notice, this list of conditions and the following disclaimer in the
> + * documentation and/or other materials provided with the distribution.
> + * 3. The name of the author may not be used to endorse or promote products
> + * derived from this software without specific prior written permission.
> + *
> + * THIS SOFTWARE IS PROVIDED BY THE AUTHOR ``AS IS'' AND ANY EXPRESS OR
> + * IMPLIED WARRANTIES, INCLUDING, BUT NOT LIMITED TO, THE IMPLIED WARRANTIES
> + * OF MERCHANTABILITY AND FITNESS FOR A PARTICULAR PURPOSE ARE DISCLAIMED.
> + * IN NO EVENT SHALL THE AUTHOR BE LIABLE FOR ANY DIRECT, INDIRECT,
> + * INCIDENTAL, SPECIAL, EXEMPLARY, OR CONSEQUENTIAL DAMAGES (INCLUDING, BUT
> + * NOT LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES; LOSS OF USE,
> + * DATA, OR PROFITS; OR BUSINESS INTERRUPTION) HOWEVER CAUSED AND ON ANY
> + * THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY, OR TORT
> + * (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE OF
> + * THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
> + */
> +
> +#include "tests.h"
> +
> +#include <errno.h>
> +#include <fcntl.h>
> +#include <netinet/in.h>
> +#include <stdio.h>
> +#include <sys/select.h>
> +#include <sys/socket.h>
> +#include <sys/types.h>
> +#include <unistd.h>
> +
> +static in_port_t
> +reserve_ephemeral_port(void)
> +{
> + int sd = socket(AF_INET, SOCK_STREAM, IPPROTO_TCP);
> + if (sd < 0)
> + perror_msg_and_skip("(server)socket AF_UNIX SOCK_STREAM");
This message doesn't look nice,
maybe just "server socket AF_UNIX SOCK_STREAM" would do?
> +
> + struct sockaddr_in addr = {
> + .sin_family = AF_INET,
> + .sin_addr.s_addr = htonl(INADDR_LOOPBACK),
> + };
> +
> + /* The range is defined in /proc/sys/net/ipv4/ip_local_port_range.
> + We use default range here. */
This is not the style of comments we use in strace nowadays,
please change to
/*
* The range is defined in /proc/sys/net/ipv4/ip_local_port_range.
* We use the default range here.
*/
> + in_port_t port;
> + for (port = 49152; port < 61000; port++)
> + {
Please use "for" loop initial declarations, e.g.
for (in_port_t port = 49152; port < 61000; port++) {
> + /* Just bind here. No listen. */
> + addr.sin_port = htons(port);
> + if (bind(sd, &addr, sizeof(addr)) == 0)
> + return port;
> + }
> + error_msg_and_skip("no ephemeral port available for testing purpose");
You probably meant to say "for test purposes".
> +}
> +
> +int
> +main(void)
> +{
> + in_port_t port = reserve_ephemeral_port ();
> +
> + /* Connect to the reserved port in NONBLOCK mode.
> + The port is reserved but not listend. So
not listened
> + the client doing "connect" gets error asynchronously. */
Please adjust the comment style.
> + int fd = socket(AF_INET, SOCK_STREAM, IPPROTO_TCP);
> + if (fd < 0)
> + perror_msg_and_skip("socket AF_UNIX SOCK_STREAM");
> +
> + int flag = fcntl(fd, F_GETFL);
> + if (flag < 0)
> + perror_msg_and_skip("fcntl F_GETFL");
> + flag |= O_NONBLOCK;
> + if (fcntl(fd, F_SETFL, flag) < 0)
> + perror_msg_and_skip("fcntl F_SETFL");
> +
> + struct sockaddr_in addr = {
> + .sin_family = AF_INET,
> + .sin_addr.s_addr = htonl(INADDR_LOOPBACK),
> + .sin_port = htons(port),
> + };
> + if (connect(fd, &addr, sizeof(addr)) == 0)
> + error_msg_and_skip("successfully connected unexpectedly");
"connect unexpectedly succeeded"
> + else if (errno != EINPROGRESS)
> + perror_msg_and_skip("failed in connect unexpectedly reason");
"connect failed for unexpected reason"
As error_msg_and_skip is noreturn, please omit "else".
> +
> + struct timeval to = {
> + .tv_sec = 1,
> + .tv_usec = 0,
> + };
> + fd_set wfds;
> + FD_ZERO(&wfds);
> + FD_SET(fd, &wfds);
> + if (select(fd + 1, NULL, &wfds, NULL, &to) < 0)
> + perror_msg_and_skip("select");
> +
> + int sock_errno;
> + socklen_t optlen = sizeof(int);
sizeof(sock_errno)?
> + if (getsockopt(fd, SOL_SOCKET, SO_ERROR, &sock_errno, &optlen) < 0)
> + perror_msg_and_skip("getsockopt");
> + else if (sock_errno != ECONNREFUSED)
As perror_msg_and_skip is noreturn, please omit "else".
> + {
style: this brace should be on the same line with its "if" statement.
> + errno = sock_errno;
> + perror_msg_and_skip("unexpected socket error");
> + }
> + else if (optlen != sizeof(int))
Likewise, sizeof(sock_errno) would look better, and "else" can be safely
omitted.
> + error_msg_and_skip("unexpected data size for error option");
You might want to include the odd data size returned by the kernel into
the diagnostic message of this "cannot happen" event.
> +
> + printf("getsockopt(%d, SOL_SOCKET, SO_ERROR, [ECONNREFUSED], [%u]) = 0\n",
> + fd, optlen);
> + puts("+++ exited with 0 +++");
> + return 0;
> +}
Please correct these minor issues, and I'll give it a try.
--
ldv
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 801 bytes
Desc: not available
URL: <http://lists.strace.io/pipermail/strace-devel/attachments/20181217/267cbff5/attachment.bin>
More information about the Strace-devel
mailing list