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

[Hannes Reinecke <hare@suse.de>]

On 6/27/22 02:19, Sam Li wrote:
> ---
>   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 73c2cdfbca..74c0245e0f 100644
> --- a/block/file-posix.c
> +++ b/block/file-posix.c
> @@ -1277,6 +1277,66 @@ out:
>   #endif
>   }
>   
> +/*
> + * Convert the zoned attribute file in sysfs to internal value.
> + */
> +static zone_model get_sysfs_str_val(int fd, struct stat *st) {
> +#ifdef CONFIG_LINUX
> +    char buf[32];
> +    char *sysfspath = NULL;
> +    int ret;
> +    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;
> +    }
> +    do {
> +        ret = read(sysfd, buf, sizeof(buf) - 1);
> +    } while (ret == -1 && errno == EINTR);

This is wrong.
read() might return a value smaller than the 'len' argument (sizeof(buf) 
-1 in your case). But in that case it's a short read, and one need to 
call 'read()' again to fetch the remaining bytes.

So the correct code would be something like:

offset = 0;
do {
     ret = read(sysfd, buf + offset, sizeof(buf) - 1 + offset);
     if (ret > 0)
         offset += ret;
} while (ret > 0);

Not that you'd actually need it; reads from sysfs are basically never 
interrupted, so you should be able to read from an attribute in one go.
IE alternatively you can drop the 'while' loop and just call read().

> +    if (ret < 0) {
> +        ret = -errno;
> +        goto out;
> +    } else if (ret == 0) {
> +        ret = -EIO;
> +        goto out;
> +    }
> +    buf[ret] = 0;
> +
> +    /* The file is ended with '\n' */

I'd rather check if the string ends with an '\n', and overwrite
it with a '\0'. That way you'd be insulated against any changes
to sysfs.

> +    if (strcmp(buf, "host-managed\n") == 0) {
> +        return BLK_Z_HM;
> +    } else if (strcmp(buf, "host-aware\n") == 0) {
> +        return BLK_Z_HA;
> +    } else {
> +        return -ENOTSUP;
> +    }
> +
> +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) {
>       int ret;
>       ret = get_sysfs_long_val(fd, st, "max_segments");

And as you already set a precedent in your previous patch, I'd recommend 
split this in two patches, one introducing a generic function 
'get_sysfs_str_val()' which returns a string and another function
(eg hdev_get_zone_model()) which calls this function to fetch the device 
zoned model.

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

Cheers,

Hannes
-- 
Dr. Hannes Reinecke		           Kernel Storage Architect
hare@suse.de			                  +49 911 74053 688
SUSE Software Solutions Germany GmbH, Maxfeldstr. 5, 90409 Nürnberg
HRB 36809 (AG Nürnberg), GF: Felix Imendörffer