Re: [RFC v4 4/9] file-posix: introduce get_sysfs_str_val for device zoned model.

[Damien Le Moal]

On 7/12/22 11:13, Sam Li wrote:
> Signed-off-by: Sam Li <faithilikerun@gmail.com>
> ---
>  block/file-posix.c           | 60 ++++++++++++++++++++++++++++++++++++
>  include/block/block-common.h |  4 +--
>  2 files changed, 62 insertions(+), 2 deletions(-)
> 
> diff --git a/block/file-posix.c b/block/file-posix.c
> index 3161d39ea4..42708012ff 100644
> --- a/block/file-posix.c
> +++ b/block/file-posix.c
> @@ -1279,6 +1279,65 @@ out:
>  #endif
>  }
>  
> +/*
> + * Convert the zoned attribute file in sysfs to internal value.
> + */
> +static zone_model get_sysfs_str_val(int fd, struct stat *st) {

This is not a generic definition of a helper function: as-is, this
function can only get the zoned attribute string. It would be better to do
the same as what you did for get_sysfs_long_val() and pass the attribute
name you want to look at and add another argument to return the string, e.g.:

static void get_sysfs_str_val(int fd, struct stat *st,
                              const char *attribute, char **val)

And if the attribute does not exist, or this is not linux, always return
"none" in str.

> +#ifdef CONFIG_LINUX
> +    char buf[32];
> +    char *sysfspath = NULL;
> +    int ret, offset;
> +    int sysfd = -1;
> +
> +    if (S_ISCHR(st->st_mode)) {
> +        if (ioctl(fd, SG_GET_SG_TABLESIZE, &ret) == 0) {
> +            return ret;
> +        }
> +        return -ENOTSUP;
> +    }
> +
> +    if (!S_ISBLK(st->st_mode)) {
> +        return -ENOTSUP;
> +    }
> +
> +    sysfspath = g_strdup_printf("/sys/dev/block/%u:%u/queue/zoned",
> +                                major(st->st_rdev), minor(st->st_rdev));
> +    sysfd = open(sysfspath, O_RDONLY);
> +    if (sysfd == -1) {
> +        ret = -errno;
> +        goto out;
> +    }
> +    offset = 0;
> +    do {
> +        ret = read(sysfd, buf + offset, sizeof(buf) - 1 + offset);
> +        if (ret > 0) {
> +            offset += ret;
> +        }
> +    } while (ret == -1);
> +    /* The file is ended with '\n' */
> +    if (buf[ret - 1] == '\n') {
> +        buf[ret - 1] = '\0';
> +    }
> +
> +    if (strcmp(buf, "host-managed") == 0) {
> +        return BLK_Z_HM;
> +    } else if (strcmp(buf, "host-aware") == 0) {
> +        return BLK_Z_HA;
> +    } else {
> +        return -ENOTSUP;
> +    }

Then all this code actually looking at the string value can go into a
different patch, or in patch 1 (same comment as for patch 3).

> +
> +out:
> +    if (sysfd != -1) {
> +        close(sysfd);
> +    }
> +    g_free(sysfspath);
> +    return ret;
> +#else
> +    return -ENOTSUP;
> +#endif
> +}
> +
>  static int hdev_get_max_segments(int fd, struct stat *st) {
>      return get_sysfs_long_val(fd, st, "max_segments");
>  }
> @@ -1885,6 +1944,7 @@ static int handle_aiocb_zone_mgmt(void *opaque) {
>      int64_t len = aiocb->aio_nbytes;
>      zone_op op = aiocb->zone_mgmt.op;
>  
> +    zone_model mod;
>      struct blk_zone_range range;
>      const char *ioctl_name;
>      unsigned long ioctl_op;
> diff --git a/include/block/block-common.h b/include/block/block-common.h
> index 78cddeeda5..35e00afe8e 100644
> --- a/include/block/block-common.h
> +++ b/include/block/block-common.h
> @@ -56,8 +56,8 @@ typedef enum zone_op {
>  } zone_op;
>  
>  typedef enum zone_model {
> -    BLK_Z_HM,
> -    BLK_Z_HA,
> +    BLK_Z_HM = 0x1,
> +    BLK_Z_HA = 0x2,
>  } zone_model;
>  
>  typedef enum BlkZoneCondition {


-- 
Damien Le Moal
Western Digital Research