Re: [PATCH blktests] create a test for direct io offsets

[Bart Van Assche <bvanassche@acm.org>]

On 10/14/25 1:54 PM, Keith Busch wrote:
> +static void find_mount_point(const char *filepath, char *mount_point,
> +			     size_t mp_len)
> +{
> +	char path[PATH_MAX - 1], abs_path[PATH_MAX], *pos;
> +	struct stat file_stat, mp_stat, parent_stat;

Why C instead of C++? Any code that manipulates strings is usually
significantly easier to write in C++ rather than C.

> +	strncpy(mount_point, path, mp_len - 1);
> +	mount_point[mp_len - 1] = '\0';

Why strncpy() instead of strdup()?

> +	len = strlen(base);
> +	p = base + len - 1;
> +	while (p > base && *p >= '0' && *p <= '9')
> +		p--;
> +
> +	if (p > base && *p == 'p' && *(p + 1) != '\0')
> +		*p = '\0';
> +	else if (len >= 4 && p > base && p < base + len - 1) {
> +		if ((strncmp(base, "sd", 2) == 0 ||
> +		     strncmp(base, "hd", 2) == 0 ||
> +		     strncmp(base, "vd", 2) == 0) &&
> +		     (*p >= 'a' && *p <= 'z'))
> +			*(p + 1) = '\0';
> +	}

Deriving the disk name from a partition name by stripping a suffix is 
fragile. A better way is to iterate over /sys/class/block/*/*. See also
the PartitionParent() function in 
https://cs.android.com/android/platform/superproject/main/+/main:system/core/fs_mgr/blockdev.cpp.

> +static void read_sysfs_attr(char *path, unsigned long *value)
> +{
> +	FILE *f;
> +	int ret;
> +
> +	f = fopen(path, "r");
> +	if (!f)
> +		err(ENOENT, "%s", path);
> +
> +	ret = fscanf(f, "%lu", value);
> +	fclose(f);
> +	if (ret != 1)
> +		err(ENOENT, "%s", basename(path));
> +}

Why is the result stored in a pointer argument instead of returning it
as return value?

> +static void read_queue_attrs(const char *path)
> +{
> +	char attr[PATH_MAX];
> +
> +	if (snprintf(attr, sizeof(attr), "%s/max_segments", path) < 0)
> +		err(errno, "max_segments");
> +	read_sysfs_attr(attr, &max_segments);

Has it been considered to make read_sysfs_attr() accept a format string
+ arguments? I think that would make the code in this function more
compact.

Thanks,

Bart.