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

[Damien Le Moal]

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


-- 
Damien Le Moal
Western Digital Research