From mboxrd@z Thu Jan  1 00:00:00 1970
From: Petr Vorel <pvorel@suse.cz>
Date: Thu, 20 May 2021 23:28:10 +0200
Subject: [LTP] [PATCH 1/2] syscalls/sendfile: Convert sendfile08 to the
 new API
In-Reply-To: <20210519084655.52780-2-xieziyao@huawei.com>
References: <20210519084655.52780-1-xieziyao@huawei.com>
 <20210519084655.52780-2-xieziyao@huawei.com>
Message-ID: <YKbUah2GaIHb6f19@pevik>
List-Id: <ltp.lists.linux.it>
MIME-Version: 1.0
Content-Type: text/plain; charset="us-ascii"
Content-Transfer-Encoding: 7bit
To: ltp@lists.linux.it

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