From: Petr Vorel <pvorel@suse.cz>
To: ltp@lists.linux.it
Subject: [LTP] [PATCH 1/2] syscalls/sendfile: Convert sendfile08 to the new API
Date: Thu, 20 May 2021 23:28:10 +0200 [thread overview]
Message-ID: <YKbUah2GaIHb6f19@pevik> (raw)
In-Reply-To: <20210519084655.52780-2-xieziyao@huawei.com>
Hi Xio,
> +++ b/testcases/kernel/syscalls/sendfile/sendfile08.c
I'd put your or LTP copyright (as your wish) because test was significantly
changed. (We had some copyright issues in the past thus it's better to state it.)
...
> +/*\
> + * [Description]
> + *
> * Bug in the splice code has caused the file position on the write side
> * of the sendfile system call to be incorrectly set to the read side file
> * position. This can result in the data being written to an incorrect offset.
> *
> - * This is a regression test for kernel commit
> - * 2cb4b05e7647891b46b91c07c9a60304803d1688
> + * This is a regression test for kernel commit 2cb4b05e7647.
nit: I wonder if we want to repeat what we already declare in .min_kver.
This is not specific to this patch, we keep doing it, but IMHO necessary
and we should stop that.
> */
> -#include <sys/sendfile.h>
> -#include <sys/stat.h>
> -#include <sys/types.h>
> -#include <errno.h>
> -#include <fcntl.h>
> #include <stdio.h>
> +#include <fcntl.h>
> #include <string.h>
> #include <unistd.h>
> -#include "test.h"
> -#include "safe_macros.h"
> +#include <sys/stat.h>
> +#include <sys/types.h>
> +#include <sys/sendfile.h>
nit: it looks to me that only <stdio.h> <string.h> <sys/sendfile.h> are needed.
But maybe others are needed and included in other headers.
Also only these were needed in legacy API:
#include <sys/sendfile.h>
#include <errno.h>
#include "test.h"
#include "safe_macros.h"
But <errno.h> is needed only for legacy API => use just these 3 mentioned above.
...
> + char buf[BUFSIZ];
> + SAFE_LSEEK(out_fd, 0, SEEK_SET);
nit: sendfile08.c:43: WARNING: Missing a blank line after declarations
It's actually more readable to have blank line after char buf[BUFSIZ];
> + SAFE_READ(0, out_fd, buf, BUFSIZ);
> +
> + if (!strncmp(buf, TEST_MSG_ALL, strlen(TEST_MSG_ALL)))
> + tst_res(TPASS, "sendfile(2) copies data correctly");
> + else
> + tst_res(TFAIL, "sendfile(2) copies data incorrectly. "
> + "Expect \"%s%s\", got \"%s\"",
> + TEST_MSG_OUT, TEST_MSG_IN, buf);
sendfile08.c:50: WARNING: quoted string split across lines
if (!strncmp(buf, TEST_MSG_ALL, strlen(TEST_MSG_ALL))) {
tst_res(TPASS, "sendfile() copied data correctly");
return;
}
tst_res(TFAIL, "sendfile() copied data incorrectly: '%s', expected '%s%s'",
buf, TEST_MSG_OUT, TEST_MSG_IN);
i.e. not splitting string, get some space by return instead else,
we don't mind using single quote (code is more readable),
removing also 2 in sendfile(2) (2 is man section, but that's just confusing).
Changes are minor, if we agre on that it can be done during merge.
Reviewed-by: Petr Vorel <pvorel@suse.cz>
Kind regards,
Petr
next prev parent reply other threads:[~2021-05-20 21:28 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 [this message]
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
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=YKbUah2GaIHb6f19@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 a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox