[PATCH] tests: Add test for decoding of PTP_* ioctl commands
Harsha Sharma
harshasharmaiitr at gmail.com
Mon Feb 26 11:16:33 UTC 2018
On Mon, Feb 26, 2018 at 6:59 AM, Dmitry V. Levin <ldv at altlinux.org> wrote:
> 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! :)
OK. I'll fix this.
>> +#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.
Should I add or replace with decoding of non-zero arguments here ?
Other tests check for both.
>
>> + 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);
Using this results, into mismatch of strace decoding and printf output
and test fails.
>> + 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.
I'll do the other changes. Using TAIL_ALLOC_OBJECT_CONST_PTR, causes
mismatch of output.
Let me know what can be done.
Thanks a lot for your review.
Regards,
Harsha Sharma
>
> --
> ldv
>
> --
> Strace-devel mailing list
> Strace-devel at lists.strace.io
> https://lists.strace.io/mailman/listinfo/strace-devel
>
More information about the Strace-devel
mailing list