diff mbox series

[v7,3/8] file-posix: introduce get_sysfs_long_val for the long sysfs attribute

Message ID 20220816062522.85714-4-faithilikerun@gmail.com (mailing list archive)
State New, archived
Headers show
Series Add support for zoned device | expand

Commit Message

Sam Li Aug. 16, 2022, 6:25 a.m. UTC 
Use sysfs attribute files to get the long value of zoned device
information.

Signed-off-by: Sam Li <faithilikerun@gmail.com>
Reviewed-by: Hannes Reinecke <hare@suse.de>
Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>
---
 block/file-posix.c | 27 +++++++++++++++++++++++++++
 1 file changed, 27 insertions(+)

Comments

Sam Li Aug. 16, 2022, 4:13 p.m. UTC | #1 
Sam Li <faithilikerun@gmail.com> 于2022年8月16日周二 14:25写道：
>
> Use sysfs attribute files to get the long value of zoned device
> information.
>
> Signed-off-by: Sam Li <faithilikerun@gmail.com>
> Reviewed-by: Hannes Reinecke <hare@suse.de>
> Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>
> ---
>  block/file-posix.c | 27 +++++++++++++++++++++++++++
>  1 file changed, 27 insertions(+)
>
> diff --git a/block/file-posix.c b/block/file-posix.c
> index c07ac4c697..727389488c 100644
> --- a/block/file-posix.c
> +++ b/block/file-posix.c
> @@ -1258,6 +1258,33 @@ static int get_sysfs_zoned_model(struct stat *st, BlockZoneModel *zoned) {
>      return 0;
>  }
>
> +/*
> + * Get zoned device information (chunk_sectors, zoned_append_max_bytes,
> + * max_open_zones, max_active_zones) through sysfs attribute files.
> + */
> +static long get_sysfs_long_val(struct stat *st, const char *attribute) {
> +#ifdef CONFIG_LINUX
> +    g_autofree char *str = NULL;
> +    const char *end;
> +    long val;
> +    int ret;
> +
> +    ret = get_sysfs_str_val(st, attribute, &str);
> +    if (ret < 0) {
> +        return ret;
> +    }
> +
> +    /* The file is ended with '\n', pass 'end' to accept that. */
> +    ret = qemu_strtol(str, &end, 10, &val);
> +    if (ret == 0 && end && *end == '\n') {

Should be  "if (ret == 0 && end && *end == '\0') {". Changes accordingly.

> +        ret = val;
> +    }
> +    return ret;
> +#else
> +    return -ENOTSUP;
> +#endif
> +}
> +
>  static int hdev_get_max_segments(int fd, struct stat *st) {
>      int ret;
>      if (S_ISCHR(st->st_mode)) {
> --
> 2.37.1
>
Damien Le Moal Aug. 16, 2022, 5:35 p.m. UTC | #2 
On 2022/08/15 23:25, Sam Li wrote:
> Use sysfs attribute files to get the long value of zoned device
> information.
> 
> Signed-off-by: Sam Li <faithilikerun@gmail.com>
> Reviewed-by: Hannes Reinecke <hare@suse.de>
> Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>
> ---
>  block/file-posix.c | 27 +++++++++++++++++++++++++++
>  1 file changed, 27 insertions(+)
> 
> diff --git a/block/file-posix.c b/block/file-posix.c
> index c07ac4c697..727389488c 100644
> --- a/block/file-posix.c
> +++ b/block/file-posix.c
> @@ -1258,6 +1258,33 @@ static int get_sysfs_zoned_model(struct stat *st, BlockZoneModel *zoned) {
>      return 0;
>  }
>  
> +/*
> + * Get zoned device information (chunk_sectors, zoned_append_max_bytes,
> + * max_open_zones, max_active_zones) through sysfs attribute files.
> + */

The comment here needs to be more generic since this helper is used in patch 2
in hdev_get_max_segments(). So simply something like:

/*
 * Get a sysfs attribute value as a long integer.
 */

And since this helper is used in patch 2, this patch needs to go before patch 2
(reverse patch 2 and 3 order).

> +static long get_sysfs_long_val(struct stat *st, const char *attribute) {
> +#ifdef CONFIG_LINUX
> +    g_autofree char *str = NULL;
> +    const char *end;
> +    long val;
> +    int ret;
> +
> +    ret = get_sysfs_str_val(st, attribute, &str);
> +    if (ret < 0) {
> +        return ret;
> +    }
> +
> +    /* The file is ended with '\n', pass 'end' to accept that. */
> +    ret = qemu_strtol(str, &end, 10, &val);
> +    if (ret == 0 && end && *end == '\n') {
> +        ret = val;
> +    }
> +    return ret;
> +#else
> +    return -ENOTSUP;
> +#endif
> +}
> +
>  static int hdev_get_max_segments(int fd, struct stat *st) {
>      int ret;
>      if (S_ISCHR(st->st_mode)) {
Sam Li Aug. 16, 2022, 5:53 p.m. UTC | #3 
Damien Le Moal <damien.lemoal@opensource.wdc.com> 于2022年8月17日周三 01:35写道：
>
> On 2022/08/15 23:25, Sam Li wrote:
> > Use sysfs attribute files to get the long value of zoned device
> > information.
> >
> > Signed-off-by: Sam Li <faithilikerun@gmail.com>
> > Reviewed-by: Hannes Reinecke <hare@suse.de>
> > Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>
> > ---
> >  block/file-posix.c | 27 +++++++++++++++++++++++++++
> >  1 file changed, 27 insertions(+)
> >
> > diff --git a/block/file-posix.c b/block/file-posix.c
> > index c07ac4c697..727389488c 100644
> > --- a/block/file-posix.c
> > +++ b/block/file-posix.c
> > @@ -1258,6 +1258,33 @@ static int get_sysfs_zoned_model(struct stat *st, BlockZoneModel *zoned) {
> >      return 0;
> >  }
> >
> > +/*
> > + * Get zoned device information (chunk_sectors, zoned_append_max_bytes,
> > + * max_open_zones, max_active_zones) through sysfs attribute files.
> > + */
>
> The comment here needs to be more generic since this helper is used in patch 2
> in hdev_get_max_segments(). So simply something like:
>
> /*
>  * Get a sysfs attribute value as a long integer.
>  */
>
> And since this helper is used in patch 2, this patch needs to go before patch 2
> (reverse patch 2 and 3 order).

Can I merge patch2 and patch 3 into one patch? Because in patch 2
hdev_get_max_segments -> get_sysfs_long_val(-> get_sysfs_str_val)
while in patch 3 get_sysfs_long_val-> get_sysfs_str_val,
hdev_get_max_segments is required for qemu setting up I guess so the
dependency is intertwined here. If we use separate patches, then the
last patch will modify the first patch's code, which I think is messy.

>
> > +static long get_sysfs_long_val(struct stat *st, const char *attribute) {
> > +#ifdef CONFIG_LINUX
> > +    g_autofree char *str = NULL;
> > +    const char *end;
> > +    long val;
> > +    int ret;
> > +
> > +    ret = get_sysfs_str_val(st, attribute, &str);
> > +    if (ret < 0) {
> > +        return ret;
> > +    }
> > +
> > +    /* The file is ended with '\n', pass 'end' to accept that. */
> > +    ret = qemu_strtol(str, &end, 10, &val);
> > +    if (ret == 0 && end && *end == '\n') {
> > +        ret = val;
> > +    }
> > +    return ret;
> > +#else
> > +    return -ENOTSUP;
> > +#endif
> > +}
> > +
> >  static int hdev_get_max_segments(int fd, struct stat *st) {
> >      int ret;
> >      if (S_ISCHR(st->st_mode)) {
>
>
> --
> Damien Le Moal
> Western Digital Research
Damien Le Moal Aug. 16, 2022, 5:55 p.m. UTC | #4 
On 2022/08/16 10:53, Sam Li wrote:
> Damien Le Moal <damien.lemoal@opensource.wdc.com> 于2022年8月17日周三 01:35写道：
>>
>> On 2022/08/15 23:25, Sam Li wrote:
>>> Use sysfs attribute files to get the long value of zoned device
>>> information.
>>>
>>> Signed-off-by: Sam Li <faithilikerun@gmail.com>
>>> Reviewed-by: Hannes Reinecke <hare@suse.de>
>>> Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>
>>> ---
>>>  block/file-posix.c | 27 +++++++++++++++++++++++++++
>>>  1 file changed, 27 insertions(+)
>>>
>>> diff --git a/block/file-posix.c b/block/file-posix.c
>>> index c07ac4c697..727389488c 100644
>>> --- a/block/file-posix.c
>>> +++ b/block/file-posix.c
>>> @@ -1258,6 +1258,33 @@ static int get_sysfs_zoned_model(struct stat *st, BlockZoneModel *zoned) {
>>>      return 0;
>>>  }
>>>
>>> +/*
>>> + * Get zoned device information (chunk_sectors, zoned_append_max_bytes,
>>> + * max_open_zones, max_active_zones) through sysfs attribute files.
>>> + */
>>
>> The comment here needs to be more generic since this helper is used in patch 2
>> in hdev_get_max_segments(). So simply something like:
>>
>> /*
>>  * Get a sysfs attribute value as a long integer.
>>  */
>>
>> And since this helper is used in patch 2, this patch needs to go before patch 2
>> (reverse patch 2 and 3 order).
> 
> Can I merge patch2 and patch 3 into one patch? Because in patch 2
> hdev_get_max_segments -> get_sysfs_long_val(-> get_sysfs_str_val)
> while in patch 3 get_sysfs_long_val-> get_sysfs_str_val,
> hdev_get_max_segments is required for qemu setting up I guess so the
> dependency is intertwined here. If we use separate patches, then the
> last patch will modify the first patch's code, which I think is messy.

Indeed. So merge the 2 patches to solve this. Rework the commit message too to
mention the introduction of the get_sysfs_long_val() helper.

> 
>>
>>> +static long get_sysfs_long_val(struct stat *st, const char *attribute) {
>>> +#ifdef CONFIG_LINUX
>>> +    g_autofree char *str = NULL;
>>> +    const char *end;
>>> +    long val;
>>> +    int ret;
>>> +
>>> +    ret = get_sysfs_str_val(st, attribute, &str);
>>> +    if (ret < 0) {
>>> +        return ret;
>>> +    }
>>> +
>>> +    /* The file is ended with '\n', pass 'end' to accept that. */
>>> +    ret = qemu_strtol(str, &end, 10, &val);
>>> +    if (ret == 0 && end && *end == '\n') {
>>> +        ret = val;
>>> +    }
>>> +    return ret;
>>> +#else
>>> +    return -ENOTSUP;
>>> +#endif
>>> +}
>>> +
>>>  static int hdev_get_max_segments(int fd, struct stat *st) {
>>>      int ret;
>>>      if (S_ISCHR(st->st_mode)) {
>>
>>
>> --
>> Damien Le Moal
>> Western Digital Research
diff mbox series

Patch

diff --git a/block/file-posix.c b/block/file-posix.c
index c07ac4c697..727389488c 100644
--- a/block/file-posix.c
+++ b/block/file-posix.c
@@ -1258,6 +1258,33 @@  static int get_sysfs_zoned_model(struct stat *st, BlockZoneModel *zoned) {
     return 0;
 }
 
+/*
+ * Get zoned device information (chunk_sectors, zoned_append_max_bytes,
+ * max_open_zones, max_active_zones) through sysfs attribute files.
+ */
+static long get_sysfs_long_val(struct stat *st, const char *attribute) {
+#ifdef CONFIG_LINUX
+    g_autofree char *str = NULL;
+    const char *end;
+    long val;
+    int ret;
+
+    ret = get_sysfs_str_val(st, attribute, &str);
+    if (ret < 0) {
+        return ret;
+    }
+
+    /* The file is ended with '\n', pass 'end' to accept that. */
+    ret = qemu_strtol(str, &end, 10, &val);
+    if (ret == 0 && end && *end == '\n') {
+        ret = val;
+    }
+    return ret;
+#else
+    return -ENOTSUP;
+#endif
+}
+
 static int hdev_get_max_segments(int fd, struct stat *st) {
     int ret;
     if (S_ISCHR(st->st_mode)) {