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

[Keith Busch <kbusch@kernel.org>]

On Tue, Oct 14, 2025 at 02:21:20PM -0700, Bart Van Assche wrote:
> 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.

The only reason I have is that I'm not a very good C++ coder.
 
> > +	strncpy(mount_point, path, mp_len - 1);
> > +	mount_point[mp_len - 1] = '\0';
> 
> Why strncpy() instead of strdup()?

I'm just using stack strings here, avoiding any heap allocations.

> > +	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.

Completely agree.

> 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.

Thanks for the pointer.

> > +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?

No particular reason.
 
> > +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.

That sounds good to me.

Though all the comments are about the boiler plate parameter extraction.
That was supposed to be the least interesting part about this patch :)