Re: [PATCH 2/4] oslib/linux-blkzoned: move sysfs reading into its own function

[Damien Le Moal <Damien.LeMoal@wdc.com>]

On 2021/05/13 7:36, Niklas Cassel wrote:
> From: Niklas Cassel <niklas.cassel@wdc.com>
> 
> Move the sysfs reading into its own function so that it can be reused.
> This new function will be reused in an succeeding patch.

s/succeeding/following (or subsequent)

> 
> No functional change intended.
> 
> Signed-off-by: Niklas Cassel <niklas.cassel@wdc.com>
> ---
>  oslib/linux-blkzoned.c | 60 ++++++++++++++++++++++++++----------------
>  1 file changed, 38 insertions(+), 22 deletions(-)
> 
> diff --git a/oslib/linux-blkzoned.c b/oslib/linux-blkzoned.c
> index 81e4e7f0..127069ca 100644
> --- a/oslib/linux-blkzoned.c
> +++ b/oslib/linux-blkzoned.c
> @@ -74,12 +74,14 @@ static char *read_file(const char *path)
>  	return strdup(line);
>  }
>  
> -int blkzoned_get_zoned_model(struct thread_data *td, struct fio_file *f,
> -			     enum zbd_zoned_model *model)
> +/*

Please add a simple description of what the function does (ok, it is clear from
the function name, but that comes after the comment so it becomes weird to read).

> + * Returns NULL on failure.
> + * Returns a pointer to a string on success.
> + * The caller is responsible for freeing the memory.
> + */
> +static char *blkzoned_get_sysfs_attr(const char *file_name, const char *attr)
>  {
> -	const char *file_name = f->file_name;
> -	char *zoned_attr_path = NULL;
> -	char *model_str = NULL;
> +	char *attr_path = NULL;
>  	struct stat statbuf;
>  	char *sys_devno_path = NULL;
>  	char *part_attr_path = NULL;
> @@ -87,13 +89,7 @@ int blkzoned_get_zoned_model(struct thread_data *td, struct fio_file *f,
>  	char sys_path[PATH_MAX];
>  	ssize_t sz;
>  	char *delim = NULL;
> -
> -	if (f->filetype != FIO_TYPE_BLOCK) {
> -		*model = ZBD_IGNORE;
> -		return 0;
> -	}
> -
> -	*model = ZBD_NONE;
> +	char *ret = NULL;

calling this ret is unusual. Generally, an int is assumed. Can you change that
to something like attr_str or val_str ?

>  
>  	if (stat(file_name, &statbuf) < 0)
>  		goto out;
> @@ -123,24 +119,44 @@ int blkzoned_get_zoned_model(struct thread_data *td, struct fio_file *f,
>  		*delim = '\0';
>  	}
>  
> -	if (asprintf(&zoned_attr_path,
> -		     "/sys/dev/block/%s/queue/zoned", sys_path) < 0)
> +	if (asprintf(&attr_path,
> +		     "/sys/dev/block/%s/%s", sys_path, attr) < 0)
>  		goto out;
>  
> -	model_str = read_file(zoned_attr_path);
> +	ret = read_file(attr_path);
> +out:
> +	free(attr_path);
> +	free(part_str);
> +	free(part_attr_path);
> +	free(sys_devno_path);
> +
> +	return ret;
> +}
> +
> +int blkzoned_get_zoned_model(struct thread_data *td, struct fio_file *f,
> +			     enum zbd_zoned_model *model)
> +{
> +	char *model_str = NULL;
> +
> +	if (f->filetype != FIO_TYPE_BLOCK) {
> +		*model = ZBD_IGNORE;
> +		return 0;
> +	}
> +
> +	*model = ZBD_NONE;
> +
> +	model_str = blkzoned_get_sysfs_attr(f->file_name, "queue/zoned");
>  	if (!model_str)
> -		goto out;
> -	dprint(FD_ZBD, "%s: zbd model string: %s\n", file_name, model_str);
> +		return 0;
> +
> +	dprint(FD_ZBD, "%s: zbd model string: %s\n", f->file_name, model_str);
>  	if (strcmp(model_str, "host-aware") == 0)
>  		*model = ZBD_HOST_AWARE;
>  	else if (strcmp(model_str, "host-managed") == 0)
>  		*model = ZBD_HOST_MANAGED;
> -out:
> +
>  	free(model_str);
> -	free(zoned_attr_path);
> -	free(part_str);
> -	free(part_attr_path);
> -	free(sys_devno_path);
> +
>  	return 0;
>  }
>  
> 


-- 
Damien Le Moal
Western Digital Research