Re: [PATCH v7 3/8] file-posix: introduce get_sysfs_long_val for the long sysfs attribute
Damien Le Moal 于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
> > Reviewed-by: Hannes Reinecke
> > Reviewed-by: Stefan Hajnoczi
> > ---
> > 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
Re: [PATCH v7 3/8] file-posix: introduce get_sysfs_long_val for the long sysfs attribute
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
> Reviewed-by: Hannes Reinecke
> Reviewed-by: Stefan Hajnoczi
> ---
> 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)) {
--
Damien Le Moal
Western Digital Research
Re: [PATCH v7 3/8] file-posix: introduce get_sysfs_long_val for the long sysfs attribute
On 2022/08/16 10:53, Sam Li wrote:
> Damien Le Moal 于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
>>> Reviewed-by: Hannes Reinecke
>>> Reviewed-by: Stefan Hajnoczi
>>> ---
>>> 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
--
Damien Le Moal
Western Digital Research
Re: [PATCH v7 3/8] file-posix: introduce get_sysfs_long_val for the long sysfs attribute
Sam Li 于2022年8月16日周二 14:25写道：
>
> Use sysfs attribute files to get the long value of zoned device
> information.
>
> Signed-off-by: Sam Li
> Reviewed-by: Hannes Reinecke
> Reviewed-by: Stefan Hajnoczi
> ---
> 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
>
[PATCH v7 3/8] file-posix: introduce get_sysfs_long_val for the long sysfs attribute
Use sysfs attribute files to get the long value of zoned device
information.
Signed-off-by: Sam Li
Reviewed-by: Hannes Reinecke
Reviewed-by: Stefan Hajnoczi
---
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') {
+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
