Wrap file metadata calls #31
@@ -207,6 +207,7 @@ add_library(akstdlib SHARED
|
||||
src/stdlib.c
|
||||
src/string.c
|
||||
src/stream.c
|
||||
src/stat.c
|
||||
src/collections.c
|
||||
)
|
||||
|
||||
@@ -317,6 +318,7 @@ set(AKSL_TESTS
|
||||
strbuf
|
||||
stream
|
||||
streamio
|
||||
stat
|
||||
strhash
|
||||
string
|
||||
strto
|
||||
|
||||
@@ -76,6 +76,9 @@
|
||||
#include <stddef.h>
|
||||
#include <stdint.h>
|
||||
#include <stdio.h>
|
||||
#include <fcntl.h>
|
||||
#include <sys/stat.h>
|
||||
|
logikoma marked this conversation as resolved
|
||||
#include <sys/statvfs.h>
|
||||
/* off_t, for the aksl_fseeko/aksl_ftello pair. POSIX, like aksl_realpath. */
|
||||
#include <sys/types.h>
|
||||
|
||||
@@ -808,6 +811,88 @@ akerr_ErrorContext AKERR_NOIGNORE *aksl_strhash_djb2_str(const char *str, uint32
|
||||
|
||||
/** @} */
|
||||
|
||||
/* ====================================================================== */
|
||||
/** @name File and filesystem metadata
|
||||
*
|
||||
* The stat family reports into caller-owned POSIX structs rather than a smaller
|
||||
* library-defined copy: the platform owns the fields, and a wrapper should not
|
||||
* discard a field merely because this library does not currently use it. libc
|
||||
* failures retain their errno value as the status, so callers can distinguish
|
||||
* absent paths from inaccessible ones without parsing an error message.
|
||||
* @{
|
||||
*/
|
||||
/* ====================================================================== */
|
||||
|
||||
/**
|
||||
* @brief stat(2).
|
||||
* @param[in] pathname Path to inspect. Required.
|
||||
* @param[out] dest File metadata. Required.
|
||||
* @throws AKERR_NULLPOINTER If pathname or dest is NULL.
|
||||
* @throws AKERR_IO If stat(2) failed and left errno at 0.
|
||||
* @throws (errno) The errno stat(2) set, reported directly as the status.
|
||||
* @return NULL on success, an error context otherwise.
|
||||
*/
|
||||
akerr_ErrorContext AKERR_NOIGNORE *aksl_stat(const char *pathname, struct stat *dest);
|
||||
|
||||
/**
|
||||
* @brief lstat(2), inspecting a symbolic link itself.
|
||||
* @param[in] pathname Path to inspect. Required.
|
||||
* @param[out] dest File metadata. Required.
|
||||
* @throws AKERR_NULLPOINTER If pathname or dest is NULL.
|
||||
* @throws AKERR_IO If lstat(2) failed and left errno at 0.
|
||||
* @throws (errno) The errno lstat(2) set, reported directly as the status.
|
||||
* @return NULL on success, an error context otherwise.
|
||||
*/
|
||||
akerr_ErrorContext AKERR_NOIGNORE *aksl_lstat(const char *pathname, struct stat *dest);
|
||||
|
||||
/**
|
||||
* @brief fstat(2).
|
||||
* @param[in] fd Open file descriptor.
|
||||
* @param[out] dest File metadata. Required.
|
||||
* @throws AKERR_NULLPOINTER If dest is NULL.
|
||||
* @throws AKERR_IO If fstat(2) failed and left errno at 0.
|
||||
* @throws (errno) The errno fstat(2) set, reported directly as the status.
|
||||
* @return NULL on success, an error context otherwise.
|
||||
*/
|
||||
akerr_ErrorContext AKERR_NOIGNORE *aksl_fstat(int fd, struct stat *dest);
|
||||
|
||||
/**
|
||||
* @brief fstatat(2).
|
||||
* @param[in] dirfd Directory descriptor, or AT_FDCWD.
|
||||
|
logikoma marked this conversation as resolved
tachikoma
commented
DEFECT (medium) — these six declarations are outside every Doxygen group. They sit between the Fix: wrap them the way every neighbouring block is wrapped: ...declarations... Also, line 862-863 is a double blank line before the Streams banner; every other group boundary in the file uses one. **DEFECT (medium) — these six declarations are outside every Doxygen group.**
They sit between the `/** @} */` on line 811 that closes *Paths and hashing* and the `/* ==== */ /** @name Streams ... @{` banner on line 864-872. Every other one of the 146 `aksl_` declarations in this header is inside a `@name ... @{ ... @}` group with a separator banner — there are 15 such groups. These six are the only ungrouped declarations in 2300 lines, so in the generated documentation they fall outside the module structure the rest of the API is organised by.
**Fix:** wrap them the way every neighbouring block is wrapped:
```c
/* ====================================================================== */
/** @name File and filesystem metadata
*
* ...rationale...
* @{
*/
/* ====================================================================== */
```
...declarations...
```c
/** @} */
```
Also, line 862-863 is a double blank line before the Streams banner; every other group boundary in the file uses one.
|
||||
* @param[in] pathname Path to inspect. Required.
|
||||
* @param[out] dest File metadata. Required.
|
||||
* @param[in] flags libc fstatat flags, passed unchanged.
|
||||
* @throws AKERR_NULLPOINTER If pathname or dest is NULL.
|
||||
* @throws AKERR_IO If fstatat(2) failed and left errno at 0.
|
||||
* @throws (errno) The errno fstatat(2) set, reported directly as the status.
|
||||
* @return NULL on success, an error context otherwise.
|
||||
*/
|
||||
akerr_ErrorContext AKERR_NOIGNORE *aksl_fstatat(int dirfd, const char *pathname, struct stat *dest, int flags);
|
||||
|
||||
/**
|
||||
* @brief statvfs(3).
|
||||
* @param[in] path Path on the filesystem. Required.
|
||||
* @param[out] dest Filesystem metadata. Required.
|
||||
* @throws AKERR_NULLPOINTER If path or dest is NULL.
|
||||
* @throws AKERR_IO If statvfs(3) failed and left errno at 0.
|
||||
* @throws (errno) The errno statvfs(3) set, reported directly as the status.
|
||||
* @return NULL on success, an error context otherwise.
|
||||
*/
|
||||
akerr_ErrorContext AKERR_NOIGNORE *aksl_statvfs(const char *path, struct statvfs *dest);
|
||||
|
||||
/**
|
||||
* @brief fstatvfs(3).
|
||||
* @param[in] fd Open file descriptor.
|
||||
* @param[out] dest Filesystem metadata. Required.
|
||||
* @throws AKERR_NULLPOINTER If dest is NULL.
|
||||
* @throws AKERR_IO If fstatvfs(3) failed and left errno at 0.
|
||||
* @throws (errno) The errno fstatvfs(3) set, reported directly as the status.
|
||||
* @return NULL on success, an error context otherwise.
|
||||
*/
|
||||
akerr_ErrorContext AKERR_NOIGNORE *aksl_fstatvfs(int fd, struct statvfs *dest);
|
||||
|
||||
|
||||
/** @} */
|
||||
/* ====================================================================== */
|
||||
/** @name Streams: open, read, write, close
|
||||
*
|
||||
|
||||
108
src/stat.c
Normal file
@@ -0,0 +1,108 @@
|
|||||||||||||||||||||||||
/*
|
|||||||||||||||||||||||||
* sys/stat.h and sys/statvfs.h metadata wrappers.
|
|||||||||||||||||||||||||
*
|
|||||||||||||||||||||||||
* These calls make information about a file or filesystem available only by a
|
|||||||||||||||||||||||||
* return value that must be checked alongside a caller-owned POSIX struct. The
|
|||||||||||||||||||||||||
* wrappers put failure in the return value, where AKERR_NOIGNORE prevents it
|
|||||||||||||||||||||||||
* from being silently dropped, while preserving the errno that distinguishes
|
|||||||||||||||||||||||||
* missing paths, inaccessible paths, and invalid descriptors. errno is cleared
|
|||||||||||||||||||||||||
* immediately before each libc call, so a broken libc that reports failure
|
|||||||||||||||||||||||||
* without setting errno is still an AKERR_IO failure rather than status 0.
|
|||||||||||||||||||||||||
*/
|
|||||||||||||||||||||||||
#include <akstdlib.h>
|
|||||||||||||||||||||||||
|
|||||||||||||||||||||||||
#include <errno.h>
|
|||||||||||||||||||||||||
#include <sys/stat.h>
|
|||||||||||||||||||||||||
#include <sys/statvfs.h>
|
|||||||||||||||||||||||||
|
|||||||||||||||||||||||||
#include "aksl_internal.h"
|
|||||||||||||||||||||||||
|
|||||||||||||||||||||||||
/*
|
|||||||||||||||||||||||||
* stat(2) follows pathname through any symbolic links and writes the target's
|
|||||||||||||||||||||||||
* metadata into the caller-owned struct stat. A file that is gone, cannot be
|
|||||||||||||||||||||||||
* searched, or lives below a non-directory component is reported as the errno
|
|||||||||||||||||||||||||
* from libc rather than as a library-specific status.
|
|||||||||||||||||||||||||
*/
|
|||||||||||||||||||||||||
akerr_ErrorContext AKERR_NOIGNORE *aksl_stat(const char *pathname, struct stat *dest)
|
|||||||||||||||||||||||||
{
|
|||||||||||||||||||||||||
|
andrew marked this conversation as resolved
andrew
commented
Missing newline Missing newline
|
|||||||||||||||||||||||||
PREPARE_ERROR(e);
|
|||||||||||||||||||||||||
FAIL_ZERO_RETURN(e, pathname, AKERR_NULLPOINTER, "pathname=%p, dest=%p", (void *)pathname, (void *)dest);
|
|||||||||||||||||||||||||
FAIL_ZERO_RETURN(e, dest, AKERR_NULLPOINTER, "pathname=%p, dest=%p", (void *)pathname, (void *)dest);
|
|||||||||||||||||||||||||
errno = 0;
|
|||||||||||||||||||||||||
FAIL_NONZERO_RETURN(e, stat(pathname, dest), AKSL_ERRNO_OR(AKERR_IO), "pathname=%s", pathname);
|
|||||||||||||||||||||||||
SUCCEED_RETURN(e);
|
|||||||||||||||||||||||||
|
logikoma marked this conversation as resolved
tachikoma
commented
DEFECT (high) — this failure branch is never executed by any test in the suite. Measured, not guessed.
Every branch inside the Line coverage hides this completely — it reads The functional consequence: Fix (one line, next to the existing **DEFECT (high) — this failure branch is never executed by any test in the suite.**
Measured, not guessed. `-DAKSL_COVERAGE=ON`, full 20-test `ctest` run, `gcov -b` on `src/stat.c`:
| line | function | libc-failure branch |
|---|---|---|
| 18 | `aksl_stat` | `taken 50%` |
| **33** | **`aksl_lstat`** | **`taken 0%`** |
| 45 | `aksl_fstat` | `taken 50%` |
| 60 | `aksl_fstatat` | `taken 50%` |
| **75** | **`aksl_statvfs`** | **`taken 0%`** |
| **87** | **`aksl_fstatvfs`** | **`taken 0%`** |
Every branch inside the `FAIL` body on this line reports `never executed`.
Line coverage hides this completely — it reads `Lines executed:100.00% of 34`, because the *line* is reached by the success path. Branch coverage is `Taken at least once: 46.77% of 248`.
The functional consequence: `AKSL_ERRNO_OR(AKERR_IO)` on this line is dead code as far as the suite is concerned. That macro is the one `src/aksl_internal.h` documents as the fix for a FAIL "whose status is 0, which every downstream DETECT and CATCH reads as success while the context still holds a pool slot: an error that is invisible and leaks at the same time." Half the wrappers in this PR ship that mechanism with it never having run once. Drop the `errno = 0` on line 32, or write `AKERR_IO` bare instead of the macro, and the suite stays green.
**Fix (one line, next to the existing `aksl_stat` ENOENT check at `tests/test_stat.c:46`):**
```c
AKSL_CHECK_STATUS(aksl_lstat("/nonexistent/aksl/stat", &ldest), ENOENT);
```
|
|||||||||||||||||||||||||
}
|
|||||||||||||||||||||||||
|
|||||||||||||||||||||||||
/*
|
|||||||||||||||||||||||||
* lstat(2) is stat(2) without the final symbolic-link traversal. That is the
|
|||||||||||||||||||||||||
* difference a caller needs when it is deciding whether a path is a link or
|
|||||||||||||||||||||||||
* when the link's ownership and mode are the metadata of interest.
|
|||||||||||||||||||||||||
*/
|
|||||||||||||||||||||||||
akerr_ErrorContext AKERR_NOIGNORE *aksl_lstat(const char *pathname, struct stat *dest)
|
|||||||||||||||||||||||||
{
|
|||||||||||||||||||||||||
PREPARE_ERROR(e);
|
|||||||||||||||||||||||||
FAIL_ZERO_RETURN(e, pathname, AKERR_NULLPOINTER, "pathname=%p, dest=%p", (void *)pathname, (void *)dest);
|
|||||||||||||||||||||||||
FAIL_ZERO_RETURN(e, dest, AKERR_NULLPOINTER, "pathname=%p, dest=%p", (void *)pathname, (void *)dest);
|
|||||||||||||||||||||||||
|
logikoma marked this conversation as resolved
tachikoma
commented
Style — multiple statements per line, and it is not only a style question here. Lines 44-45 put five statements on two lines. Worth separating the cosmetic part from the real part. The code as written is correct — I checked the expansion. But collapsing unbraced control-flow macros onto a shared line is the exact shape that makes the next edit here go wrong silently, and the one-statement-per-line rule exists to stop it. This is also the only place in the file where the visual structure does not match the other five wrappers, which is what makes a reviewer stop. Fix: **Style — multiple statements per line, and it is not only a style question here.**
Lines 44-45 put five statements on two lines. `src/stat.c:86-87` in `aksl_fstatvfs` does the same. The other four wrappers in this file, and every wrapper in `src/stream.c`, put one statement per line.
Worth separating the cosmetic part from the real part. `PREPARE_ERROR`, `FAIL_ZERO_RETURN`, `FAIL_NONZERO_RETURN` and `SUCCEED_RETURN` are all **multi-statement macros with no wrapping braces** — `FAIL_ZERO_RETURN` expands to a bare `if ( __x == 0 ) { FAIL(...); return __err_context; }` and `SUCCEED_RETURN` expands to `RELEASE_ERROR(e); return NULL;`, which is itself two statements including an `if`. `PREPARE_ERROR` expands to a function call *plus a declaration*.
The code as written is correct — I checked the expansion. But collapsing unbraced control-flow macros onto a shared line is the exact shape that makes the next edit here go wrong silently, and the one-statement-per-line rule exists to stop it. This is also the only place in the file where the visual structure does not match the other five wrappers, which is what makes a reviewer stop.
**Fix:**
```c
PREPARE_ERROR(e);
FAIL_ZERO_RETURN(e, dest, AKERR_NULLPOINTER, "fd=%d, dest=%p", fd, (void *)dest);
errno = 0;
FAIL_NONZERO_RETURN(e, fstat(fd, dest), AKSL_ERRNO_OR(AKERR_IO), "fd=%d", fd);
SUCCEED_RETURN(e);
```
|
|||||||||||||||||||||||||
errno = 0;
|
|||||||||||||||||||||||||
FAIL_NONZERO_RETURN(e, lstat(pathname, dest), AKSL_ERRNO_OR(AKERR_IO), "pathname=%s", pathname);
|
|||||||||||||||||||||||||
SUCCEED_RETURN(e);
|
|||||||||||||||||||||||||
}
|
|||||||||||||||||||||||||
|
|||||||||||||||||||||||||
/*
|
|||||||||||||||||||||||||
* fstat(2) gets metadata from an already-open descriptor, so it has no path
|
|||||||||||||||||||||||||
* lookup race and remains useful after the file has been renamed or unlinked.
|
|||||||||||||||||||||||||
* A closed or otherwise invalid descriptor reports EBADF from libc.
|
|||||||||||||||||||||||||
*/
|
|||||||||||||||||||||||||
akerr_ErrorContext AKERR_NOIGNORE *aksl_fstat(int fd, struct stat *dest)
|
|||||||||||||||||||||||||
{
|
|||||||||||||||||||||||||
PREPARE_ERROR(e);
|
|||||||||||||||||||||||||
FAIL_ZERO_RETURN(e, dest, AKERR_NULLPOINTER, "fd=%d, dest=%p", fd, (void *)dest);
|
|||||||||||||||||||||||||
errno = 0;
|
|||||||||||||||||||||||||
FAIL_NONZERO_RETURN(e, fstat(fd, dest), AKSL_ERRNO_OR(AKERR_IO), "fd=%d", fd);
|
|||||||||||||||||||||||||
SUCCEED_RETURN(e);
|
|||||||||||||||||||||||||
}
|
|||||||||||||||||||||||||
|
|||||||||||||||||||||||||
/*
|
|||||||||||||||||||||||||
* fstatat(2) is the directory-descriptor form of stat(2). pathname is resolved
|
|||||||||||||||||||||||||
* relative to dirfd unless it is absolute, and flags retain the libc choices
|
|||||||||||||||||||||||||
* such as inspecting a link itself. Keeping those flags unchanged prevents this
|
|||||||||||||||||||||||||
* wrapper from inventing a smaller policy than the POSIX call already exposes.
|
|||||||||||||||||||||||||
*/
|
|||||||||||||||||||||||||
akerr_ErrorContext AKERR_NOIGNORE *aksl_fstatat(int dirfd, const char *pathname, struct stat *dest, int flags)
|
|||||||||||||||||||||||||
{
|
|||||||||||||||||||||||||
PREPARE_ERROR(e);
|
|||||||||||||||||||||||||
FAIL_ZERO_RETURN(e, pathname, AKERR_NULLPOINTER, "dirfd=%d, pathname=%p, dest=%p", dirfd, (void *)pathname, (void *)dest);
|
|||||||||||||||||||||||||
FAIL_ZERO_RETURN(e, dest, AKERR_NULLPOINTER, "dirfd=%d, pathname=%p, dest=%p", dirfd, (void *)pathname, (void *)dest);
|
|||||||||||||||||||||||||
|
logikoma marked this conversation as resolved
tachikoma
commented
DEFECT (high) — Third of the three. gcov, full suite: Fix, at **DEFECT (high) — `aksl_statvfs`'s failure branch is never executed.**
Third of the three. gcov, full suite: `taken 0%`, `FAIL` body `never executed`. `tests/test_stat.c:69` is the only non-NULL call and it is the success path.
**Fix, at `tests/test_stat.c:70`:**
```c
AKSL_CHECK_STATUS(aksl_statvfs("/nonexistent/aksl/stat", &vdest), ENOENT);
```
|
|||||||||||||||||||||||||
errno = 0;
|
|||||||||||||||||||||||||
FAIL_NONZERO_RETURN(e, fstatat(dirfd, pathname, dest, flags), AKSL_ERRNO_OR(AKERR_IO), "dirfd=%d, pathname=%s, flags=0x%x", dirfd, pathname, flags);
|
|||||||||||||||||||||||||
SUCCEED_RETURN(e);
|
|||||||||||||||||||||||||
}
|
|||||||||||||||||||||||||
|
|||||||||||||||||||||||||
/*
|
|||||||||||||||||||||||||
* statvfs(3) writes information about the mounted filesystem containing path,
|
|||||||||||||||||||||||||
* not merely the named file. The result includes the filesystem block sizes and
|
|||||||||||||||||||||||||
* available space the caller needs before it decides whether an operation fits.
|
|||||||||||||||||||||||||
*/
|
|||||||||||||||||||||||||
akerr_ErrorContext AKERR_NOIGNORE *aksl_statvfs(const char *path, struct statvfs *dest)
|
|||||||||||||||||||||||||
{
|
|||||||||||||||||||||||||
|
logikoma marked this conversation as resolved
tachikoma
commented
DEFECT (high) — Same gcov measurement as This one is the most conspicuous of the three, because its sibling already has the test. Fix, inserted at (Separately, lines 86-87 cram five statements onto two lines — see the note on line 45.) **DEFECT (high) — `aksl_fstatvfs`'s failure branch is never executed, and this one is a two-line fix.**
Same gcov measurement as `src/stat.c:33`: the libc-failure branch here is `taken 0%` across the whole 20-test suite, and every branch in the `FAIL` body reports `never executed`.
This one is the most conspicuous of the three, because its sibling already has the test. `tests/test_stat.c:29-32` closes the descriptor and then asserts `aksl_fstat` reports `EBADF` on it — but the `aksl_fstatvfs` call three lines earlier (line 27) is only exercised on the success path. The test already has a closed fd sitting right there.
**Fix, inserted at `tests/test_stat.c:32` beside the existing check:**
```c
/* A descriptor that was just closed must surface libc's EBADF. */
AKSL_CHECK_STATUS(aksl_fstat(fd, &fd_dest), EBADF);
AKSL_CHECK_STATUS(aksl_fstatvfs(fd, &vfs_dest), EBADF);
```
(Separately, lines 86-87 cram five statements onto two lines — see the note on line 45.)
|
|||||||||||||||||||||||||
PREPARE_ERROR(e);
|
|||||||||||||||||||||||||
FAIL_ZERO_RETURN(e, path, AKERR_NULLPOINTER, "path=%p, dest=%p", (void *)path, (void *)dest);
|
|||||||||||||||||||||||||
FAIL_ZERO_RETURN(e, dest, AKERR_NULLPOINTER, "path=%p, dest=%p", (void *)path, (void *)dest);
|
|||||||||||||||||||||||||
errno = 0;
|
|||||||||||||||||||||||||
FAIL_NONZERO_RETURN(e, statvfs(path, dest), AKSL_ERRNO_OR(AKERR_IO), "path=%s", path);
|
|||||||||||||||||||||||||
SUCCEED_RETURN(e);
|
|||||||||||||||||||||||||
}
|
|||||||||||||||||||||||||
|
|||||||||||||||||||||||||
/*
|
|||||||||||||||||||||||||
* fstatvfs(3) is the descriptor form of statvfs(3). It asks the filesystem that
|
|||||||||||||||||||||||||
* owns fd for the same capacity and flag information without resolving a path
|
|||||||||||||||||||||||||
* again, and reports a bad descriptor through the errno libc supplies.
|
|||||||||||||||||||||||||
*/
|
|||||||||||||||||||||||||
akerr_ErrorContext AKERR_NOIGNORE *aksl_fstatvfs(int fd, struct statvfs *dest)
|
|||||||||||||||||||||||||
{
|
|||||||||||||||||||||||||
PREPARE_ERROR(e);
|
|||||||||||||||||||||||||
FAIL_ZERO_RETURN(e, dest, AKERR_NULLPOINTER, "fd=%d, dest=%p", fd, (void *)dest);
|
|||||||||||||||||||||||||
errno = 0;
|
|||||||||||||||||||||||||
FAIL_NONZERO_RETURN(e, fstatvfs(fd, dest), AKSL_ERRNO_OR(AKERR_IO), "fd=%d", fd);
|
|||||||||||||||||||||||||
SUCCEED_RETURN(e);
|
|||||||||||||||||||||||||
}
|
|||||||||||||||||||||||||
134
tests/test_stat.c
Normal file
@@ -0,0 +1,134 @@
|
||||
/* File metadata wrapper tests. */
|
||||
#include "aksl_capture.h"
|
||||
#include <errno.h>
|
||||
#include <fcntl.h>
|
||||
#include <string.h>
|
||||
|
||||
/*
|
||||
* A bit libc has never assigned in the AT_ space. fstatat(2) must reject it
|
||||
* with EINVAL rather than ignoring it, which is what proves flags reach libc
|
||||
* unmasked. Re-check this constant if AT_ ever grows into 0x40000000.
|
||||
*/
|
||||
#define AKSL_TEST_AT_INVALID 0x40000000
|
||||
|
||||
static int test_stat_success_and_fstat(void)
|
||||
{
|
||||
char path[AKSL_TMP_MAX];
|
||||
int fd = -1;
|
||||
struct stat path_dest;
|
||||
struct stat fd_dest;
|
||||
struct statvfs vfs_dest;
|
||||
/* A real file makes the mode and four-byte size observable to stat(2). */
|
||||
AKSL_CHECK(aksl_temp_file(path, sizeof(path)) == 0);
|
||||
fd = open(path, O_RDWR);
|
||||
AKSL_CHECK(fd >= 0);
|
||||
AKSL_CHECK(write(fd, "stat", 4) == 4);
|
||||
AKSL_CHECK_OK(aksl_stat(path, &path_dest));
|
||||
AKSL_CHECK(S_ISREG(path_dest.st_mode));
|
||||
AKSL_CHECK(path_dest.st_size == 4);
|
||||
|
||||
/* fstat(2) addresses the same opened inode, not a second pathname lookup. */
|
||||
AKSL_CHECK_OK(aksl_fstat(fd, &fd_dest));
|
||||
AKSL_CHECK(fd_dest.st_ino == path_dest.st_ino);
|
||||
|
||||
/* fstatvfs(3) describes the backing filesystem and has a usable block size. */
|
||||
AKSL_CHECK_OK(aksl_fstatvfs(fd, &vfs_dest));
|
||||
AKSL_CHECK(vfs_dest.f_frsize != 0);
|
||||
AKSL_CHECK(close(fd) == 0);
|
||||
|
||||
/* A descriptor that was just closed must surface libc's EBADF. */
|
||||
AKSL_CHECK_STATUS(aksl_fstat(fd, &fd_dest), EBADF);
|
||||
AKSL_CHECK_STATUS(aksl_fstatvfs(fd, &vfs_dest), EBADF);
|
||||
AKSL_CHECK(unlink(path) == 0);
|
||||
return 0;
|
||||
}
|
||||
|
||||
static int test_stat_paths_and_fstatat(void)
|
||||
{
|
||||
char path[AKSL_TMP_MAX];
|
||||
char child[AKSL_TMP_MAX * 2];
|
||||
char directory[AKSL_TMP_MAX];
|
||||
char linkpath[AKSL_TMP_MAX * 2];
|
||||
char *basename = NULL;
|
||||
int dirfd = -1;
|
||||
struct stat dest;
|
||||
struct stat ldest;
|
||||
struct stat path_dest;
|
||||
struct statvfs vdest;
|
||||
/* Missing paths and a regular file used as a directory preserve errno. */
|
||||
AKSL_CHECK_STATUS(aksl_stat("/nonexistent/aksl/stat", &dest), ENOENT);
|
||||
AKSL_CHECK_STATUS(aksl_lstat("/nonexistent/aksl/stat", &ldest), ENOENT);
|
||||
/* The temporary path is a regular file, so adding a child tests ENOTDIR. */
|
||||
AKSL_CHECK(aksl_temp_file(path, sizeof(path)) == 0);
|
||||
AKSL_CHECK(snprintf(child, sizeof(child), "%s/child", path) < (int)sizeof(child));
|
||||
AKSL_CHECK_OK(aksl_stat(path, &path_dest));
|
||||
AKSL_CHECK_STATUS(aksl_stat(child, &dest), ENOTDIR);
|
||||
|
logikoma marked this conversation as resolved
tachikoma
commented
Nit — magic number.
Fix: name it and say why. **Nit — magic number.**
`0x40000000` is "a flag bit libc will reject", and nothing in the file says so. It is currently unallocated on Linux, but the `AT_` space is still being handed out — `AT_RECURSIVE` (0x8000), the `AT_STATX_*` bits and `AT_HANDLE_FID` were all added over time. When 0x40000000 is eventually assigned, this test either starts failing for a reason nobody will connect to this line, or worse, keeps passing for a different reason.
**Fix:** name it and say why.
```c
/*
* A bit libc has never assigned in the AT_ space. fstatat(2) must reject it
* with EINVAL rather than ignoring it, which is what proves flags reach libc
* unmasked. Re-check this constant if AT_ ever grows into 0x40000000.
*/
#define AKSL_TEST_AT_INVALID 0x40000000
```
|
||||
AKSL_CHECK(snprintf(linkpath, sizeof(linkpath), "%s.link", path) < (int)sizeof(linkpath));
|
||||
|
logikoma marked this conversation as resolved
tachikoma
commented
DEFECT (medium) — this asserts on
C permits a standard library function to set Two further problems with it:
Fix: delete line 66. The Worth stating the other half plainly, since it is the thing this test was reaching for: the errno handling in the library itself is correct. I traced it. **DEFECT (medium) — this asserts on `errno` after intervening library calls that are allowed to change it.**
`AKSL_CHECK_STATUS` on line 65 does not just evaluate the expression. It calls `aksl_take()` (`tests/aksl_capture.h`), which runs **four `snprintf` calls and `akerr_release_error()`** before returning. Only then does line 66 read `errno`.
C permits a standard library function to set `errno` to a non-zero value even on a successful call. So this assertion is relying on glibc's `snprintf` happening not to touch `errno` for these particular inputs. It passes today. It is not guaranteed by anything, and it will fail on a libc where that is not true, with a failure message that points at `fstatat` rather than at `snprintf`.
Two further problems with it:
1. **It is redundant.** Line 65 already proved the wrapper reported `EINVAL`. Line 66 adds no coverage of the wrapper.
2. **It asserts a contract the library does not offer.** Nothing in `include/akstdlib.h` promises anything about `errno` after a wrapper returns — and it could not, because the NULL-argument paths (`src/stat.c:15-16`, `30-31`, `44`, `57-58`, `72-73`, `86`) return *before* `errno = 0` is ever reached, so on those paths `errno` is whatever stale value the caller already had. Codifying a post-call `errno` reading for one path out of two is how an accidental contract gets born.
**Fix:** delete line 66. The `errno = E2BIG` on line 64 is the valuable part — it proves the wrapper does not report a stale errno — and line 65 already checks it.
Worth stating the other half plainly, since it is the thing this test was reaching for: **the errno handling in the library itself is correct.** I traced it. `FAIL` expands to `ENSURE_ERROR_READY(e); e->status = __err;`, so `AKSL_ERRNO_OR` is evaluated *after* `akerr_next_error()` runs — and `akerr_next_error()` (`deps/libakerror/src/error.c:240-248`) is a bare loop over `AKERR_ARRAY_ERROR` with no library calls in it, so it cannot clobber `errno` in between. All six wrappers reset `errno` immediately before their libc call and read it before anything else can run. No defect there.
|
||||
AKSL_CHECK(symlink(path, linkpath) == 0);
|
||||
|
||||
/* stat follows the link while lstat reports the link object itself. */
|
||||
AKSL_CHECK_OK(aksl_stat(linkpath, &dest));
|
||||
AKSL_CHECK_OK(aksl_lstat(linkpath, &ldest));
|
||||
AKSL_CHECK(S_ISREG(dest.st_mode));
|
||||
AKSL_CHECK(S_ISLNK(ldest.st_mode));
|
||||
|
||||
/* A real dirfd must resolve a relative name against that directory. */
|
||||
AKSL_CHECK(snprintf(directory, sizeof(directory), "%s", path) < (int)sizeof(directory));
|
||||
basename = strrchr(directory, '/');
|
||||
AKSL_CHECK(basename != NULL);
|
||||
*basename = '\0';
|
||||
dirfd = open(directory, O_RDONLY | O_DIRECTORY);
|
||||
AKSL_CHECK(dirfd >= 0);
|
||||
AKSL_CHECK_OK(aksl_fstatat(dirfd, strrchr(path, '/') + 1, &dest, 0));
|
||||
AKSL_CHECK(dest.st_ino == path_dest.st_ino);
|
||||
AKSL_CHECK(close(dirfd) == 0);
|
||||
|
||||
/* A valid non-zero flag must reach libc, making this call an lstat. */
|
||||
AKSL_CHECK_OK(aksl_fstatat(AT_FDCWD, linkpath, &dest, AT_SYMLINK_NOFOLLOW));
|
||||
AKSL_CHECK(S_ISLNK(dest.st_mode));
|
||||
|
||||
/* Invalid flags must replace stale errno with the EINVAL libc reports. */
|
||||
errno = E2BIG;
|
||||
AKSL_CHECK_STATUS(aksl_fstatat(AT_FDCWD, path, &dest, AKSL_TEST_AT_INVALID), EINVAL);
|
||||
|
||||
/* statvfs reports ENOENT before it can describe a missing filesystem path. */
|
||||
AKSL_CHECK_STATUS(aksl_statvfs("/nonexistent/aksl/stat", &vdest), ENOENT);
|
||||
|
||||
/* statvfs reports the filesystem containing the current directory. */
|
||||
AKSL_CHECK_OK(aksl_statvfs(".", &vdest));
|
||||
AKSL_CHECK(vdest.f_frsize != 0);
|
||||
AKSL_CHECK(unlink(linkpath) == 0);
|
||||
AKSL_CHECK(unlink(path) == 0);
|
||||
return 0;
|
||||
}
|
||||
|
||||
static int test_stat_null_arguments(void)
|
||||
{
|
||||
char path[AKSL_TMP_MAX];
|
||||
struct stat dest;
|
||||
struct statvfs vdest;
|
||||
/* Create a valid path so each failure below isolates a NULL argument. */
|
||||
AKSL_CHECK(aksl_temp_file(path, sizeof(path)) == 0);
|
||||
/* Every wrapper refuses either missing caller-owned input or output storage. */
|
||||
AKSL_CHECK_STATUS(aksl_stat(NULL, &dest), AKERR_NULLPOINTER);
|
||||
AKSL_CHECK_STATUS(aksl_stat(path, NULL), AKERR_NULLPOINTER);
|
||||
AKSL_CHECK_STATUS(aksl_lstat(NULL, &dest), AKERR_NULLPOINTER);
|
||||
AKSL_CHECK_STATUS(aksl_lstat(path, NULL), AKERR_NULLPOINTER);
|
||||
AKSL_CHECK_STATUS(aksl_fstat(0, NULL), AKERR_NULLPOINTER);
|
||||
AKSL_CHECK_STATUS(aksl_fstatat(AT_FDCWD, NULL, &dest, 0), AKERR_NULLPOINTER);
|
||||
AKSL_CHECK_STATUS(aksl_fstatat(AT_FDCWD, path, NULL, 0), AKERR_NULLPOINTER);
|
||||
AKSL_CHECK_STATUS(aksl_statvfs(NULL, &vdest), AKERR_NULLPOINTER);
|
||||
AKSL_CHECK_STATUS(aksl_statvfs(path, NULL), AKERR_NULLPOINTER);
|
||||
AKSL_CHECK_STATUS(aksl_fstatvfs(0, NULL), AKERR_NULLPOINTER);
|
||||
AKSL_CHECK(unlink(path) == 0);
|
||||
return 0;
|
||||
}
|
||||
|
||||
int main(void)
|
||||
{
|
||||
int failures = 0;
|
||||
AKSL_RUN(failures, test_stat_success_and_fstat);
|
||||
AKSL_RUN(failures, test_stat_paths_and_fstatat);
|
||||
AKSL_RUN(failures, test_stat_null_arguments);
|
||||
AKSL_REPORT(failures);
|
||||
}
|
||||
DEFECT (high) — the header is not self-contained for
aksl_fstatat.Line 79-80 add
<sys/stat.h>and<sys/statvfs.h>, but not<fcntl.h>. POSIX declaresAT_FDCWD,AT_SYMLINK_NOFOLLOW,AT_EMPTY_PATHandAT_NO_AUTOMOUNTin<fcntl.h>, not in<sys/stat.h>. Theaksl_fstatatdoc block on line 838-844 tells the caller to passAT_FDCWDand "libc fstatat flags", and then does not give them a way to spell either one.Verified. A translation unit whose only include is
<akstdlib.h>:Adding
#include <fcntl.h>to the test file compiles it clean. The suite never caught this becausetests/test_stat.c:4includes<fcntl.h>itself, so the test is not testing the header the way a consumer sees it.Fix: add
#include <fcntl.h>here, beside the other two.