[PATCH] tests: Add test for decoding of PTP_* ioctl commands
Dmitry V. Levin
ldv at altlinux.org
Mon Feb 26 01:29:37 UTC 2018
On Sun, Feb 25, 2018 at 04:01:14PM +0530, Harsha Sharma wrote:
> * tests/ioctl_ptp.c: New file.
> * tests/.gitignore: Add ioctl_ptp.
> * tests/Makefile.am (check_PROGRAMS): Likewise.
> * tests/gen_tests.in (ioctl_ptp): New entry.
> * tests/pure_executables.list: Likewise.
Thanks, see my comments below.
> ---
> tests/.gitignore | 1 +
> tests/Makefile.am | 1 +
> tests/gen_tests.in | 1 +
> tests/ioctl_ptp.c | 104 ++++++++++++++++++++++++++++++++++++++++++++
> tests/pure_executables.list | 2 +
> 5 files changed, 109 insertions(+)
> create mode 100644 tests/ioctl_ptp.c
>
> diff --git a/tests/.gitignore b/tests/.gitignore
> index 623b98f8..ad0ee1c2 100644
> --- a/tests/.gitignore
> +++ b/tests/.gitignore
> @@ -135,6 +135,7 @@ ioctl_loop-nv
> ioctl_loop-v
> ioctl_mtd
> ioctl_nsfs
> +ioctl_ptp
> ioctl_rtc
> ioctl_rtc-v
> ioctl_scsi
> diff --git a/tests/Makefile.am b/tests/Makefile.am
> index 493a7f97..ced96816 100644
> --- a/tests/Makefile.am
> +++ b/tests/Makefile.am
> @@ -110,6 +110,7 @@ check_PROGRAMS = $(PURE_EXECUTABLES) \
> ioctl_loop-nv \
> ioctl_loop-v \
> ioctl_nsfs \
> + ioctl_ptp \
If you add ioctl_ptp to the list of pure executables, it will be
automatically added to PURE_EXECUTABLES, making this hunk unneeded.
> ioctl_rtc-v \
> is_linux_mips_n64 \
> ksysent \
> diff --git a/tests/gen_tests.in b/tests/gen_tests.in
> index d41bd670..ea611a8f 100644
> --- a/tests/gen_tests.in
> +++ b/tests/gen_tests.in
> @@ -134,6 +134,7 @@ ioctl_loop-nv +ioctl.test -a22 -e verbose=none
> ioctl_loop-v +ioctl.test -v
> ioctl_mtd +ioctl.test
> ioctl_nsfs +ioctl.test -esignal=none
> +ioctl_ptp +ioctl.test
> ioctl_rtc +ioctl.test
> ioctl_rtc-v +ioctl.test -v
> ioctl_scsi +ioctl.test
> diff --git a/tests/ioctl_ptp.c b/tests/ioctl_ptp.c
> new file mode 100644
> index 00000000..009c12cd
> --- /dev/null
> +++ b/tests/ioctl_ptp.c
> @@ -0,0 +1,104 @@
> +/*
> + * Check decoding of PTP_* commands of ioctl syscall.
> + *
> + * Copyright (c) 2017 Harsha Sharma <harshasharmaiitr at gmail.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 "xlat.h"
> +#include <fcntl.h>
> +#include <stdio.h>
> +#include <stdlib.h>
> +#include <sys/ioctl.h>
> +#include <inttypes.h>
> +#include <unistd.h>
> +#include <linux/ptp_clock.h>
Please note that <linux/ptp_clock.h> was introduced by linux kernel commit
v3.0-rc1~164^2~4, PTP_SYS_OFFSET was added even later (by commit
v3.8-rc1~139^2~514), and the test suite as well as strace itself should
not fail to compile with kernel headers older than v3.8. In ptp.c we
handle this by checking for HAVE_STRUCT_PTP_SYS_OFFSET, you can probably
do the same here.
Welcome to the world of portability issues! :)
> +#include "xlat/ptp_flags_options.h"
> +
> +static void
> +test_no_device(void)
> +{
> + ioctl(-1, PTP_CLOCK_GETCAPS, NULL);
> + printf("ioctl(-1, PTP_CLOCK_GETCAPS, NULL) = -1 EBADF (%m)\n");
You can also check decoding of non-zero arguments here.
> + ioctl(-1, PTP_SYS_OFFSET, NULL);
> + printf("ioctl(-1, PTP_SYS_OFFSET, NULL) = -1 EBADF (%m)\n");
Error path decoding of struct ptp_sys_offset can be tested without any
device.
> + ioctl(-1, PTP_ENABLE_PPS, 0);
> + printf("ioctl(-1, PTP_ENABLE_PPS, 0) = -1 EBADF (%m)\n");
You can also check decoding of non-zero arguments here.
> + ioctl(-1, PTP_EXTTS_REQUEST, NULL);
> + printf("ioctl(-1, PTP_EXTTS_REQUEST, NULL) = -1 EBADF (%m)\n");
As PTP_EXTTS_REQUEST is fully decoded on entering syscall,
decoding of struct ptp_extts_request can be fully tested without any device.
> + ioctl(-1, PTP_PEROUT_REQUEST, NULL);
> + printf("ioctl(-1, PTP_PEROUT_REQUEST, NULL) = -1 EBADF (%m)\n");
Likewise, with PTP_PEROUT_REQUEST and struct ptp_perout_request.
> +}
> +
> +static void
> +test_ptp0_device(void)
> +{
> + struct ptp_clock_caps *caps;
> + struct ptp_extts_request extts;
> + struct ptp_perout_request perout;
> + struct ptp_sys_offset sysoff;
> +
> + int fd = open("/dev/ptp0", O_RDWR);
> +
> + if (fd < 0)
> + perror_msg_and_skip("clock invalid");
I'd rather print the device name, e.g.
static const char device[] = "/dev/ptp0";
int fd = open(device, O_RDWR);
if (fd < 0)
perror_msg_and_skip(device);
Besides that, I think test_no_device() does quite a lot of testing so
skipping the whole test when /dev/ptp0 is not available is not the right
thing to do. Let's just skip test_ptp0_device() in this case.
This way we'll get at least partial coverage in those environment where
/dev/ptp0 is not available, e.g. in travis.
> + int getcaps_fd = ioctl(fd, PTP_CLOCK_GETCAPS, &caps);
> + printf("ioctl(%d, PTP_CLOCK_GETCAPS, %p) = %s\n",
> + fd, &caps, sprintrc(getcaps_fd));
I don't think PTP_CLOCK_GETCAPS returns a file descriptor, so the name
getcaps_fd is confusing.
It's definitely not a good idea to specify "struct ptp_clock_caps **"
argument to PTP_CLOCK_GETCAPS.
When testing decoders of structures, we often place the structure
at the end of mapped memory region to test that strace does not attempt
to fetch memory beyond the end of structure, e.g.
TAIL_ALLOC_OBJECT_CONST_PTR(struct ptp_clock_caps, caps);
> + int sysoff_fd = ioctl(fd, PTP_SYS_OFFSET, &sysoff);
> + printf("ioctl(%d, PTP_SYS_OFFSET, {n_samples=%u}) = %s\n",
> + fd, sysoff.n_samples, sprintrc(sysoff_fd));
Likewise, I'm sure PTP_SYS_OFFSET doesn't return any file descriptors.
Likewise, TAIL_ALLOC_OBJECT_CONST_PTR(struct ptp_sys_offset, sysoff);
It's not a good idea to pass garbage in sysoff->n_samples as the kernel
checks it against PTP_MAX_SAMPLES, see linux/drivers/ptp/ptp_chardev.c.
> + int pps_fd = ioctl(fd, PTP_ENABLE_PPS, 1);
> + printf("ioctl(%d, PTP_ENABLE_PPS, 1) = %s\n", fd, sprintrc(pps_fd));
Likewise, PTP_ENABLE_PPS definitely doesn't return any file descriptors.
This test should go to test_no_device(), I'd also check decoding
of a negative argument.
> + int extts_fd = ioctl(fd, PTP_EXTTS_REQUEST, &extts);
> + printf("ioctl(%d, PTP_EXTTS_REQUEST, {index=%d, flags=",
> + fd, extts.index);
> + printflags(ptp_flags_options, extts.flags, "PTP_???");
> + printf("}) = %s\n", sprintrc(extts_fd));
Likewise, PTP_EXTTS_REQUEST doesn't return any file descriptors.
Likewise, this test should go to test_no_device().
> + int perout_fd = ioctl(fd, PTP_PEROUT_REQUEST, &perout);
> + printf("ioctl(%d, PTP_PEROUT_REQUEST, {start={%" PRId64 ""
> + ", %" PRIu32 "}, period={%" PRId64 ", %" PRIu32 "}"
> + ", index=%d, flags=", fd, (int64_t)perout.start.sec,
> + perout.start.nsec, (int64_t)perout.period.sec,
> + perout.period.nsec, perout.index);
> + printflags(ptp_flags_options, perout.flags, "PTP_???");
> + printf("}) = %s\n", sprintrc(perout_fd));
Likewise.
> +}
> +
> +int
> +main(void)
> +{
> + test_no_device();
> + test_ptp0_device();
> + puts("+++ exited with 0 +++");
> + return 0;
> +}
> diff --git a/tests/pure_executables.list b/tests/pure_executables.list
> index 1ed40d1f..3e346619 100755
> --- a/tests/pure_executables.list
> +++ b/tests/pure_executables.list
> @@ -107,6 +107,8 @@ ioctl_kvm_run
> ioctl_loop
> ioctl_mtd
> ioctl_rtc
> +ioctl_nsfs
> +ioctl_ptp
> ioctl_scsi
> ioctl_sg_io_v3
> ioctl_sg_io_v4
Why do you sneak this in? If you think ioctl_nsfs was omitted here by
mistake, it should be a separate commit that moves ioctl_nsfs from
check_PROGRAMS to this list.
--
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/20180226/58531813/attachment.bin>
More information about the Strace-devel
mailing list