Re: [Devel] [PATCH VZ10 2/3] selftests/ve: Add more helpers

[Vladimir Riabchun <vladimir.riabchun@virtuozzo.com>]

Sashiko review + replies.

On 9/7/26 21:14, Vladimir Riabchun wrote:
> Some more read/write helpers may be useful.
> 
> Also, add a helper to execute functions in child process with
> switched namespaces and cgroup.
> 
> https://virtuozzo.atlassian.net/browse/VSTOR-144504
> 
> Feature: ve selftests
> Signed-off-by: Vladimir Riabchun <vladimir.riabchun@virtuozzo.com>
> ---
>   tools/testing/selftests/ve/ve_selftest.h | 78 +++++++++++++++++++++++++++++---
>   1 file changed, 72 insertions(+), 6 deletions(-)
> 
> diff --git a/tools/testing/selftests/ve/ve_selftest.h b/tools/testing/selftests/ve/ve_selftest.h
> index b3c5bf63f34d..d35fec30c7b4 100644
> --- a/tools/testing/selftests/ve/ve_selftest.h
> +++ b/tools/testing/selftests/ve/ve_selftest.h
> @@ -163,6 +163,14 @@ static inline int write_file_at(int dirfd, const char *path, const char *val)
>   	return (ret == (int)len) ? 0 : -1;
>   }
>   
> +static inline int write_u64_at(int dirfd, const char *path, unsigned long long val)
> +{
> +	char s[20];
> +
> +	snprintf(s, sizeof(s), "%llu", val);
> +	return write_file_at(dirfd, path, s);
> +}

Sashiko says:
  Could this buffer be too small? A 64-bit unsigned integer can have up to
  20 digits. With the null terminator, the buffer s[] might need 21 bytes
  to avoid truncation.

Fair. Will be fixed in v2.

> +
>   static inline int read_file_at(int dirfd, const char *path, char *buf,
>   			       size_t buflen)
>   {
> @@ -193,19 +201,31 @@ static inline int read_u64_at(int dirfd, const char *path,
>   			      unsigned long long *out)
>   {
>   	char buf[32] = {0}, *end;
> -	int fd, ret;
> +	int ret;
>   
> -	fd = openat(dirfd, path, O_RDONLY);
> -	if (fd < 0)
> +	ret = read_file_at(dirfd, path, buf, sizeof(buf));
> +	if (ret <= 0)
>   		return -1;
>   
> -	ret = read(fd, buf, sizeof(buf) - 1);
> -	close(fd);
> +	errno = 0;
> +	*out = strtoull(buf, &end, 10);

Sashiko says:
  If read_file_at() fills the 32-byte buffer completely, does that remove
  the null terminator? If so, could strtoull() read out of bounds?

No, read_file_at is guaranteed to put \0 at the end of buffer.

> +	if (errno || end == buf)
> +		return -1;
> +	return 0;
> +}
> +
> +static inline int read_s32_at(int dirfd, const char *path,
> +			      int *out)
> +{
> +	char buf[32] = {0}, *end;
> +	int ret;
> +
> +	ret = read_file_at(dirfd, path, buf, sizeof(buf));
>   	if (ret <= 0)
>   		return -1;
>   
>   	errno = 0;
> -	*out = strtoull(buf, &end, 10);
> +	*out = strtol(buf, &end, 10);

Sashiko says:
  Does reading 32 bytes into a 32-byte buffer cause a similar out-of-bounds
  read issue here in read_s32_at() via strtol()?
  On 64-bit systems, strtol() returns a long. Can this lead to integer
  truncation when the result is stored in the 32-bit int pointer out?

1. No, same reason as above.
2. Maybe. It's worth adding range sanity check. Will do in v2.

>   	if (errno || end == buf)
>   		return -1;
>   	return 0;
> @@ -254,6 +274,52 @@ static inline int enter_cgroup(int cgv2_fd, int ctid)
>   	return ret;
>   }
>   
> +/*
> + * Run function in VE cgroup and new namespaces.
> + *
> + * Namespaces are provided via unshare_flags.
> + * CLONE_NEWVE flag is set by this function.
> + * Return values:
> + *  -  0 if function returns zero
> + *  - -1 if function returns negative value
> + *  -  1 if setup fails or function returns positive value
> + */
> +static inline int run_in_ve(int cgv2_fd, int ctid, struct namespace_ctx *ctx,
> +		int flags, int (*fn)(void *), void *arg)
> +{
> +	int status;
> +	pid_t pid;
> +
> +	pid = fork();
> +	if (pid < 0) {
> +		fprintf(stderr, "%s: fork failed\n", __func__);
> +		return 1;
> +	}
> +	if (pid == 0) {
> +		int ret;
> +
> +		if (enter_cgroup(cgv2_fd, ctid) < 0) {
> +			fprintf(stderr, "%s: enter_cgroup failed\n", __func__);
> +			_exit(255);
> +		}
> +		if (enter_namespaces(ctx, flags) < 0)
> +			_exit(255);
> +
> +		ret = fn(arg);
> +		if (ret < 0)
> +			ret = 1;
> +		else if (ret > 0)
> +			ret = 255;
> +		_exit(ret);
> +	}
> +	if (waitpid(pid, &status, 0) < 0 || !WIFEXITED(status) || WEXITSTATUS(status) == 255)
> +		return 1;
> +	if (WEXITSTATUS(status))
> +		return -1;
> +	return 0;
> +
> +}
> +

Sashiko says:
  Does the return logic in run_in_ve() mask the root cause of failures?
  By returning 1 for waitpid failures, signal terminations, and
  WEXITSTATUS 255, it seems setup failures and process crashes are
  collapsed into a single value.

True. While values still will collapse due to return value truncation,
it is worth separating fork/waitpid errors and fn setup errors.
Will do this in v2.

>   /*
>    * Create a fresh VE cgroup at the first free id at or after @from and unhide
>    * its ve.* control files. Return the new id, or -1.
> 

-- 
Best regards, Riabchun Vladimir
Linux Kernel Developer, Virtuozzo

_______________________________________________
Devel mailing list
Devel@openvz.org
https://lists.openvz.org/mailman/listinfo/devel