On Sun, Aug 29, 2021 at 11:07:10PM +0000, Damien Le Moal wrote:
> On 2021/08/27 16:35, Naohiro Aota wrote:
> > Factor out a new function device_read_bytes() to read a sysfs path as bytes
> > for a preparation for the next commit and to reduce the code duplication.
> >
> > Signed-off-by: Naohiro Aota <naohiro.aota@xxxxxxx>
> > ---
> > misc-utils/lsblk.c | 45 ++++++++++++++++++++-------------------------
> > 1 file changed, 20 insertions(+), 25 deletions(-)
> >
> > diff --git a/misc-utils/lsblk.c b/misc-utils/lsblk.c
> > index 100eba0779f8..775a6d832076 100644
> > --- a/misc-utils/lsblk.c
> > +++ b/misc-utils/lsblk.c
> > @@ -708,6 +708,24 @@ static uint64_t device_get_discard_granularity(struct lsblk_device *dev)
> > return dev->discard_granularity;
> > }
> >
> > +static void device_read_bytes(struct lsblk_device *dev, char *path, char **str,
> > + uint64_t *sortdata)
> > +{
> > + if (lsblk->bytes) {
> > + ul_path_read_string(dev->sysfs, str, path);
> > + if (sortdata)
> > + str2u64(*str, sortdata);
>
> You could return early here to avoid the else...
Yes, that will be more simple. I'll send a new series with the fix to
the "max" description.
> > + } else {
> > + uint64_t x;
> > +
> > + if (ul_path_read_u64(dev->sysfs, &x, path) == 0) {
> > + *str = size_to_human_string(SIZE_SUFFIX_1LETTER, x);
> > + if (sortdata)
> > + *sortdata = x;
> > + }
> > + }
> > +}
> > +
> > /*
> > * Generates data (string) for column specified by column ID for specified device. If sortdata
> > * is not NULL then returns number usable to sort the column if the data are available for the
> > @@ -1033,18 +1051,7 @@ static char *device_get_data(
> > }
> > break;
> > case COL_DMAX:
> > - if (lsblk->bytes) {
> > - ul_path_read_string(dev->sysfs, &str, "queue/discard_max_bytes");
> > - if (sortdata)
> > - str2u64(str, sortdata);
> > - } else {
> > - uint64_t x;
> > - if (ul_path_read_u64(dev->sysfs, &x, "queue/discard_max_bytes") == 0) {
> > - str = size_to_human_string(SIZE_SUFFIX_1LETTER, x);
> > - if (sortdata)
> > - *sortdata = x;
> > - }
> > - }
> > + device_read_bytes(dev, "queue/discard_max_bytes", &str, sortdata);
> > break;
> > case COL_DZERO:
> > if (device_get_discard_granularity(dev) > 0)
> > @@ -1053,19 +1060,7 @@ static char *device_get_data(
> > str = xstrdup("0");
> > break;
> > case COL_WSAME:
> > - if (lsblk->bytes) {
> > - ul_path_read_string(dev->sysfs, &str, "queue/write_same_max_bytes");
> > - if (sortdata)
> > - str2u64(str, sortdata);
> > - } else {
> > - uint64_t x;
> > -
> > - if (ul_path_read_u64(dev->sysfs, &x, "queue/write_same_max_bytes") == 0) {
> > - str = size_to_human_string(SIZE_SUFFIX_1LETTER, x);
> > - if (sortdata)
> > - *sortdata = x;
> > - }
> > - }
> > + device_read_bytes(dev, "queue/write_same_max_bytes", &str, sortdata);
> > if (!str)
> > str = xstrdup("0");
> > break;
> >
>
> Apart from the optional nit above, looks good to me.
>
> Reviewed-by: Damien Le Moal <damien.lemoal@xxxxxxx>
>
>
> --
> Damien Le Moal
> Western Digital Research
>
