From: Petr Vorel <pvorel@suse.cz>
To: ltp@lists.linux.it
Subject: [LTP] [PATCH 2/2] syscalls/sendfile: Convert sendfile09 to the new API
Date: Thu, 20 May 2021 23:56:24 +0200 [thread overview]
Message-ID: <YKbbCLUlaICqSIK5@pevik> (raw)
In-Reply-To: <20210519084655.52780-3-xieziyao@huawei.com>
Hi Xie,
> +++ b/testcases/kernel/syscalls/sendfile/sendfile09.c
...
> + * Copyright (c) International Business Machines Corp., 2014
Again, missing copyright.
> +/*\
> + * [Description]
> *
> - * DESCRIPTION
> - * Testcase copied from sendfile02.c to test the basic functionality of
> - * the sendfile(2) system call on large file. There is a kernel bug which
> - * introduced by commit 8f9c0119d7ba and fixed by commit 5d73320a96fcc.
> + * Testcase copied from sendfile02.c to test the basic functionality of
> + * the sendfile(2) system call on large file. There is a kernel bug which
> + * introduced by commit 8f9c0119d7ba and fixed by commit 5d73320a96fcc.
If 8f9c0119d7ba caused a regression it shouldn't be in .tags (it contains
commits which are fixes and should be backported). Also it's a question if it's
useful info, because this commit is mentioned in 5d73320a96fcc (fixing commit).
> *
> - * ALGORITHM
> - * 1. call sendfile(2) with offset at 0
> - * 2. call sendfile(2) with offset at 3GB
> + * [Algorithm]
> *
> - * USAGE: <for command-line>
> - * sendfile09 [-c n] [-i n] [-I x] [-P x] [-t]
> - * where,
> - * -i n : Execute test n times.
> - * -I x : Execute test for x seconds.
> - * -P x : Pause for x seconds between iterations.
> - * -t : Turn on syscall timing.
> + * 1. Call sendfile(2) with offset at 0;
> + * 2. Call sendfile(2) with offset at 3GB.
> *
> + * [Restrictions]
> *
> - * RESTRICTIONS
> - * Only supports 64bit systems and kernel 2.6.33 or above
> + * Only supports 64bit systems and kernel 2.6.33 or above.
nit: Maybe: Only 64bit systems are supported.
I'm not a native speaker, but "Only supports" sounds wrong to me.
Also although I'd keep .min_kver, mentioning it is IMHO necessary -
why to repeat info we have in .tags? We still do that, but IMHO we should
stop doing it. And this ancient version is certainly not worth duplicity
(latest tested kernel is 3.10 [1]).
sendfile09.c:58: WARNING: Missing a blank line after declarations
sendfile09.c:75: WARNING: Missing a blank line after declarations
sendfile09.c:80: WARNING: Comparisons should place the constant on the right side of the test
sendfile09.c:82: WARNING: quoted string split across lines
sendfile09.c:86: WARNING: quoted string split across lines
sendfile09.c:90: WARNING: quoted string split across lines
> */
> -#include <stdio.h>
> -#include <errno.h>
> +
> #include <fcntl.h>
> #include <sys/stat.h>
> #include <sys/sendfile.h>
> #include <sys/types.h>
> #include <unistd.h>
> #include <inttypes.h>
Again, only these are needed:
#include <inttypes.h>
#include <sys/sendfile.h>
#include "tst_test.h"
#include "lapi/abisize.h"
> -static void cleanup(void);
> -static void setup(void);
> +#ifndef OFF_T
> +#define OFF_T off_t
> +#endif
I wonder where OFF_T comes from and if we can just simply use off_t.
> -#define ONE_GB (INT64_C(1) << 30)
> +#define ONE_GB (INT64_C(1) << 30)
> +#define IN_FILE "in_file"
> +#define OUT_FILE "out_file"
:...
> +static void setup(void)
> {
> - int i;
> -
> - tst_sig(FORK, DEF_HANDLER, cleanup);
> - TEST_PAUSE;
> -
> - /* make a temporary directory and cd to it */
> - tst_tmpdir();
> -
> - if (!tst_fs_has_free(NULL, ".", 5, TST_GB))
> - tst_brkm(TCONF, cleanup, "sendfile(2) on large file"
> - " needs 5G free space.");
> + if (!tst_fs_has_free(".", 5, TST_GB))
> + tst_brk(TCONF, "Test on large file needs 5G free space");
> - /* create a 4G file */
> - fd = SAFE_CREAT(cleanup, in_file, 00700);
> - for (i = 1; i <= (4 * 1024); i++) {
> - SAFE_LSEEK(cleanup, fd, 1024 * 1024 - 1, SEEK_CUR);
> - SAFE_WRITE(cleanup, 1, fd, "C", 1);
> + int fd = SAFE_CREAT(IN_FILE, 00700);
> + for (int i = 1; i <= (4 * 1024); ++i) {
This will lead to error in old compilers:
error: 'for' loop initial declarations are only allowed in C99 mode
https://travis-ci.org/github/pevik/ltp/jobs/771837120
https://travis-ci.org/github/pevik/ltp/jobs/771837126
It must be:
int i;
...
for (i = 1; i <= (4 * 1024); ++i) {
Thus probably better to declare fd before as well.
int i, fd;
...
> +static void run(unsigned int i)
> {
...
> + off_t before_pos, after_pos;
> + before_pos = SAFE_LSEEK(in_fd, 0, SEEK_CUR);
> +
> + TEST(sendfile(out_fd, in_fd, &offset, tc[i].count));
> + after_pos = SAFE_LSEEK(in_fd, 0, SEEK_CUR);
> +
> + if (TST_RET != tc[i].exp_retval)
> + tst_res(TFAIL, "sendfile(2) failed to return expected value, "
> + "expected: %" PRId64 ", got: %ld",
> + tc[i].exp_retval, TST_RET);
> + else if (offset != tc[i].exp_updated_offset)
> + tst_res(TFAIL, "sendfile(2) failed to update OFFSET parameter to "
> + "expected value, expected: %" PRId64 ", got: %" PRId64,
> + tc[i].exp_updated_offset, (int64_t)(offset));
> + else if (before_pos != after_pos)
> + tst_res(TFAIL, "sendfile(2) updated the file position of in_fd "
> + "unexpectedly, expected file position: %" PRId64
> + ", actual file position %" PRId64,
> + (int64_t)(before_pos), (int64_t)(after_pos));
Yes, we probably cannot avoid splitting string here, unless TST_EXP_FAIL() can
be used here.
I'd avoid 2 in "sendfile(2).
> + else
> + tst_res(TPASS, "sendfile(2) with %s", tc[i].desc);
Again, minor things, I can fix them before merge.
Kind regards,
Petr
next prev parent reply other threads:[~2021-05-20 21:56 UTC|newest]
Thread overview: 16+ messages / expand[flat|nested] mbox.gz Atom feed top
2021-05-19 8:46 [LTP] [PATCH 0/2] syscalls/sendfile: Convert sendfile{08, 09} to the new API Xie Ziyao
2021-05-19 8:46 ` [LTP] [PATCH 1/2] syscalls/sendfile: Convert sendfile08 " Xie Ziyao
2021-05-19 9:18 ` Cyril Hrubis
2021-05-20 21:28 ` Petr Vorel
2021-05-21 3:29 ` Xie Ziyao
2021-05-21 6:48 ` Petr Vorel
2021-05-19 8:46 ` [LTP] [PATCH 2/2] syscalls/sendfile: Convert sendfile09 " Xie Ziyao
2021-05-19 9:37 ` Cyril Hrubis
2021-05-20 11:10 ` Xie Ziyao
2021-05-20 10:49 ` Cyril Hrubis
2021-05-25 15:17 ` Petr Vorel
2021-05-20 21:56 ` Petr Vorel [this message]
2021-05-21 9:20 ` Xie Ziyao
2021-05-21 11:18 ` Petr Vorel
2021-05-25 15:13 ` Petr Vorel
2021-05-26 3:23 ` Xie Ziyao
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=YKbbCLUlaICqSIK5@pevik \
--to=pvorel@suse.cz \
--cc=ltp@lists.linux.it \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.