[LTP] [PATCH v5 1/7] API: Add safe openat, printfat, readat and unlinkat
Cyril Hrubis
chrubis@suse.cz
Fri Apr 30 16:22:53 CEST 2021
Hi!
> Add 'at' variants for a number of system calls and LTP SAFE API
> functions. This avoids using sprintf everywhere to build paths.
>
> Also adds tst_decode_fd which allows us to retrieve the path for an FD
> for debugging purposes without having to store it ourselves. However
> the proc symlink may not be available on some systems.
>
> Signed-off-by: Richard Palethorpe <rpalethorpe@suse.com>
> Reviewed-by: Petr Vorel <pvorel@suse.cz>
> ---
> include/tst_safe_file_at.h | 51 +++++++++++
> lib/tst_safe_file_at.c | 176 +++++++++++++++++++++++++++++++++++++
> 2 files changed, 227 insertions(+)
> create mode 100644 include/tst_safe_file_at.h
> create mode 100644 lib/tst_safe_file_at.c
>
> diff --git a/include/tst_safe_file_at.h b/include/tst_safe_file_at.h
> new file mode 100644
> index 000000000..fbb63b4a8
> --- /dev/null
> +++ b/include/tst_safe_file_at.h
> @@ -0,0 +1,51 @@
> +// SPDX-License-Identifier: GPL-2.0-or-later
> +/*
> + * Copyright (c) 2021 SUSE LLC <rpalethorpe@suse.com>
> + */
> +
> +#ifndef TST_SAFE_FILE_AT_H
> +#define TST_SAFE_FILE_AT_H
> +
> +#include <sys/types.h>
> +#include <stdarg.h>
> +
> +#define SAFE_OPENAT(dirfd, path, oflags, ...) \
> + safe_openat(__FILE__, __LINE__, \
> + (dirfd), (path), (oflags), ## __VA_ARGS__)
> +
> +#define SAFE_FILE_READAT(dirfd, path, buf, nbyte) \
> + safe_file_readat(__FILE__, __LINE__, \
> + (dirfd), (path), (buf), (nbyte))
> +
> +
> +#define SAFE_FILE_PRINTFAT(dirfd, path, fmt, ...) \
> + safe_file_printfat(__FILE__, __LINE__, \
> + (dirfd), (path), (fmt), __VA_ARGS__)
> +
> +#define SAFE_UNLINKAT(dirfd, path, flags) \
> + safe_unlinkat(__FILE__, __LINE__, (dirfd), (path), (flags))
> +
> +char *tst_decode_fd(int fd);
> +
> +int safe_openat(const char *file, const int lineno,
> + int dirfd, const char *path, int oflags, ...);
> +
> +ssize_t safe_file_readat(const char *file, const int lineno,
> + int dirfd, const char *path, char *buf, size_t nbyte);
> +
> +int tst_file_vprintfat(int dirfd, const char *path, const char *fmt, va_list va);
> +int tst_file_printfat(int dirfd, const char *path, const char *fmt, ...)
> + __attribute__ ((format (printf, 3, 4)));
> +
> +int safe_file_vprintfat(const char *file, const int lineno,
> + int dirfd, const char *path,
> + const char *fmt, va_list va);
> +
> +int safe_file_printfat(const char *file, const int lineno,
> + int dirfd, const char *path, const char *fmt, ...)
> + __attribute__ ((format (printf, 5, 6)));
> +
> +int safe_unlinkat(const char *file, const int lineno,
> + int dirfd, const char *path, int flags);
> +
> +#endif
> diff --git a/lib/tst_safe_file_at.c b/lib/tst_safe_file_at.c
> new file mode 100644
> index 000000000..43372998e
> --- /dev/null
> +++ b/lib/tst_safe_file_at.c
> @@ -0,0 +1,176 @@
> +// SPDX-License-Identifier: GPL-2.0-or-later
> +/*
> + * Copyright (c) 2021 SUSE LLC <rpalethorpe@suse.com>
> + */
> +
> +#define _GNU_SOURCE
> +#include <stdio.h>
> +#include "lapi/fcntl.h"
> +#include "tst_safe_file_at.h"
> +
> +#define TST_NO_DEFAULT_MAIN
> +#include "tst_test.h"
> +
> +char fd_path[PATH_MAX];
^
static otherwise the symbol will end up exported in the object file
> +char *tst_decode_fd(int fd)
> +{
> + ssize_t ret;
> + char proc_path[32];
> +
> + if (fd < 0)
> + return "!";
> +
> + sprintf(proc_path, "/proc/self/fd/%d", fd);
> + ret = readlink(proc_path, fd_path, sizeof(fd_path));
> +
> + if (ret < 0)
> + return "?";
> +
> + fd_path[ret] = '\0';
> +
> + return fd_path;
> +}
> +
> +int safe_openat(const char *file, const int lineno,
> + int dirfd, const char *path, int oflags, ...)
> +{
> + va_list ap;
> + int fd;
> + mode_t mode;
> +
> + va_start(ap, oflags);
> + mode = va_arg(ap, int);
> + va_end(ap);
> +
> + fd = openat(dirfd, path, oflags, mode);
> + if (fd > -1)
> + return fd;
> +
> + tst_brk_(file, lineno, TBROK | TERRNO,
> + "openat(%d<%s>, '%s', %o, %o)",
> + dirfd, tst_decode_fd(dirfd), path, oflags, mode);
> +
> + return fd;
> +}
> +
> +ssize_t safe_file_readat(const char *file, const int lineno,
> + int dirfd, const char *path, char *buf, size_t nbyte)
> +{
> + int fd = safe_openat(file, lineno, dirfd, path, O_RDONLY);
> + ssize_t rval;
> +
> + if (fd < 0)
> + return -1;
> +
> + rval = safe_read(file, lineno, NULL, 0, fd, buf, nbyte - 1);
> + if (rval < 0)
> + return -1;
> +
> + close(fd);
> + buf[rval] = '\0';
> +
> + if (rval >= (ssize_t)nbyte - 1) {
> + tst_brk_(file, lineno, TBROK,
> + "Buffer length %zu too small to read %d<%s>/%s",
> + nbyte, dirfd, tst_decode_fd(dirfd), path);
> + }
> +
> + return rval;
> +}
> +
> +int tst_file_vprintfat(int dirfd, const char *path, const char *fmt, va_list va)
> +{
> + int fd = openat(dirfd, path, O_WRONLY);
> +
> + if (fd < 0)
> + return -1;
> +
> + TEST(vdprintf(fd, fmt, va));
Can we please avoid using the TEST() macro in the test library?
The value of TST_RET is supposed not to change unless actuall test code
does that.
> + close(fd);
> +
> + if (TST_RET < 0) {
> + errno = TST_ERR;
> + return -2;
> + }
> +
> + return TST_RET;
> +}
> +
> +int tst_file_printfat(int dirfd, const char *path, const char *fmt, ...)
> +{
> + va_list va;
> + int rval;
> +
> + va_start(va, fmt);
> + rval = tst_file_vprintfat(dirfd, path, fmt, va);
> + va_end(va);
> +
> + return rval;
> +}
> +
> +int safe_file_vprintfat(const char *file, const int lineno,
> + int dirfd, const char *path,
> + const char *fmt, va_list va)
> +{
> + char buf[16];
> + va_list vac;
> + int rval;
> +
> + va_copy(vac, va);
> +
> + TEST(tst_file_vprintfat(dirfd, path, fmt, va));
Here as well.
> + if (TST_RET == -2) {
> + rval = vsnprintf(buf, sizeof(buf), fmt, vac);
> + va_end(vac);
> +
> + if (rval >= (ssize_t)sizeof(buf))
> + strcpy(buf + sizeof(buf) - 5, "...");
> +
> + tst_brk_(file, lineno, TBROK | TTERRNO,
> + "vdprintf(%d<%s>, '%s', '%s'<%s>)",
> + dirfd, tst_decode_fd(dirfd), path, fmt,
> + rval > 0 ? buf : "???");
I do not think that vsnprintf() can even return < 0 and the only way how
to make it return 0 is possibly with an empty fmt string. So I would
just skip this check.
> + return -1;
> + }
> +
> + va_end(vac);
> +
> + if (TST_RET == -1) {
> + tst_brk_(file, lineno, TBROK | TTERRNO,
> + "openat(%d<%s>, '%s', O_WRONLY)",
> + dirfd, tst_decode_fd(dirfd), path);
> + }
> +
> + return TST_RET;
> +}
> +
> +int safe_file_printfat(const char *file, const int lineno,
> + int dirfd, const char *path,
> + const char *fmt, ...)
> +{
> + va_list va;
> + int rval;
> +
> + va_start(va, fmt);
> + rval = safe_file_vprintfat(file, lineno, dirfd, path, fmt, va);
> + va_end(va);
> +
> + return rval;
> +}
> +
> +int safe_unlinkat(const char *file, const int lineno,
> + int dirfd, const char *path, int flags)
> +{
> + int rval = unlinkat(dirfd, path, flags);
> +
> + if (rval > -1)
> + return rval;
if (!rval)
return 0;
Anything else than 0 is either error or invalid.
> + tst_brk_(file, lineno, TBROK | TERRNO,
> + "unlinkat(%d<%s>, '%s', %s)", dirfd, tst_decode_fd(dirfd), path,
> + flags == AT_REMOVEDIR ? "AT_REMOVEDIR" : (flags ? "?" : "0"));
> +
> + return rval;
> +}
The rest looks good.
With issues I pointed you can add:
Reviewed-by: Cyril Hrubis <chrubis@suse.cz>
--
Cyril Hrubis
chrubis@suse.cz
More information about the ltp
mailing list