Hannes Reinecke <h...@suse.de> 于2022年6月27日周一 15:41写道：
>
> 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().
>
Yes, I didn't think it through.
> > + 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.
>
Right! I was thinking about it but got hasty to hardcode everything.
> > + 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.
>
Maybe we can just return a str in get_sysfs_str_val and ignore
returning a zone_model for now? I was using zone_model for testing
purpose. Right now, this function is not used by others in
file-posix.c that causes compilation error in QEMU.
Thanks for reviewing!
Sam
> > 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
> h...@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
