From: Kent Overstreet <kent.overstreet@linux.dev>
The new flags parameter allows controlling
- Whether or not the units suffix is separated by a space, for
compatibility with sort -h
- Whether or not to append a B suffix - we're not always printing
bytes.
Signed-off-by: Kent Overstreet <kent.overstreet@linux.dev>
Signed-off-by: Suren Baghdasaryan <surenb@google.com>
Cc: Andy Shevchenko <andy@kernel.org>
Cc: Michael Ellerman <mpe@ellerman.id.au>
Cc: Benjamin Herrenschmidt <benh@kernel.crashing.org>
Cc: Paul Mackerras <paulus@samba.org>
Cc: "Michael S. Tsirkin" <mst@redhat.com>
Cc: Jason Wang <jasowang@redhat.com>
Cc: "Noralf Trønnes" <noralf@tronnes.org>
Cc: Jens Axboe <axboe@kernel.dk>
---
arch/powerpc/mm/book3s64/radix_pgtable.c | 2 +-
drivers/block/virtio_blk.c | 4 ++--
drivers/gpu/drm/gud/gud_drv.c | 2 +-
drivers/mmc/core/block.c | 4 ++--
drivers/mtd/spi-nor/debugfs.c | 6 ++---
.../ethernet/chelsio/cxgb4/cxgb4_debugfs.c | 4 ++--
drivers/scsi/sd.c | 8 +++----
include/linux/string_helpers.h | 11 +++++-----
lib/string_helpers.c | 22 ++++++++++++++-----
lib/test-string_helpers.c | 4 ++--
mm/hugetlb.c | 8 +++----
11 files changed, 42 insertions(+), 33 deletions(-)
diff --git a/arch/powerpc/mm/book3s64/radix_pgtable.c b/arch/powerpc/mm/book3s64/radix_pgtable.c
index c6a4ac766b2b..27aa5a083ff0 100644
--- a/arch/powerpc/mm/book3s64/radix_pgtable.c
+++ b/arch/powerpc/mm/book3s64/radix_pgtable.c
@@ -260,7 +260,7 @@ print_mapping(unsigned long start, unsigned long end, unsigned long size, bool e
if (end <= start)
return;
- string_get_size(size, 1, STRING_UNITS_2, buf, sizeof(buf));
+ string_get_size(size, 1, STRING_SIZE_BASE2, buf, sizeof(buf));
pr_info("Mapped 0x%016lx-0x%016lx with %s pages%s\n", start, end, buf,
exec ? " (exec)" : "");
diff --git a/drivers/block/virtio_blk.c b/drivers/block/virtio_blk.c
index 2bf14a0e2815..94fba7f57079 100644
--- a/drivers/block/virtio_blk.c
+++ b/drivers/block/virtio_blk.c
@@ -934,9 +934,9 @@ static void virtblk_update_capacity(struct virtio_blk *vblk, bool resize)
nblocks = DIV_ROUND_UP_ULL(capacity, queue_logical_block_size(q) >> 9);
string_get_size(nblocks, queue_logical_block_size(q),
- STRING_UNITS_2, cap_str_2, sizeof(cap_str_2));
+ STRING_SIZE_BASE2, cap_str_2, sizeof(cap_str_2));
string_get_size(nblocks, queue_logical_block_size(q),
- STRING_UNITS_10, cap_str_10, sizeof(cap_str_10));
+ 0, cap_str_10, sizeof(cap_str_10));
dev_notice(&vdev->dev,
"[%s] %s%llu %d-byte logical blocks (%s/%s)\n",
diff --git a/drivers/gpu/drm/gud/gud_drv.c b/drivers/gpu/drm/gud/gud_drv.c
index 9d7bf8ee45f1..6b1748e1f666 100644
--- a/drivers/gpu/drm/gud/gud_drv.c
+++ b/drivers/gpu/drm/gud/gud_drv.c
@@ -329,7 +329,7 @@ static int gud_stats_debugfs(struct seq_file *m, void *data)
struct gud_device *gdrm = to_gud_device(entry->dev);
char buf[10];
- string_get_size(gdrm->bulk_len, 1, STRING_UNITS_2, buf, sizeof(buf));
+ string_get_size(gdrm->bulk_len, 1, STRING_SIZE_BASE2, buf, sizeof(buf));
seq_printf(m, "Max buffer size: %s\n", buf);
seq_printf(m, "Number of errors: %u\n", gdrm->stats_num_errors);
diff --git a/drivers/mmc/core/block.c b/drivers/mmc/core/block.c
index 32d49100dff5..1cded1e9aca4 100644
--- a/drivers/mmc/core/block.c
+++ b/drivers/mmc/core/block.c
@@ -2557,7 +2557,7 @@ static struct mmc_blk_data *mmc_blk_alloc_req(struct mmc_card *card,
blk_queue_write_cache(md->queue.queue, cache_enabled, fua_enabled);
- string_get_size((u64)size, 512, STRING_UNITS_2,
+ string_get_size((u64)size, 512, STRING_SIZE_BASE2,
cap_str, sizeof(cap_str));
pr_info("%s: %s %s %s%s\n",
md->disk->disk_name, mmc_card_id(card), mmc_card_name(card),
@@ -2753,7 +2753,7 @@ static int mmc_blk_alloc_rpmb_part(struct mmc_card *card,
list_add(&rpmb->node, &md->rpmbs);
- string_get_size((u64)size, 512, STRING_UNITS_2,
+ string_get_size((u64)size, 512, STRING_SIZE_BASE2,
cap_str, sizeof(cap_str));
pr_info("%s: %s %s %s, chardev (%d:%d)\n",
diff --git a/drivers/mtd/spi-nor/debugfs.c b/drivers/mtd/spi-nor/debugfs.c
index 2dbda6b6938a..f6c3ca430df1 100644
--- a/drivers/mtd/spi-nor/debugfs.c
+++ b/drivers/mtd/spi-nor/debugfs.c
@@ -85,7 +85,7 @@ static int spi_nor_params_show(struct seq_file *s, void *data)
seq_printf(s, "name\t\t%s\n", info->name);
seq_printf(s, "id\t\t%*ph\n", SPI_NOR_MAX_ID_LEN, nor->id);
- string_get_size(params->size, 1, STRING_UNITS_2, buf, sizeof(buf));
+ string_get_size(params->size, 1, STRING_SIZE_BASE2, buf, sizeof(buf));
seq_printf(s, "size\t\t%s\n", buf);
seq_printf(s, "write size\t%u\n", params->writesize);
seq_printf(s, "page size\t%u\n", params->page_size);
@@ -130,14 +130,14 @@ static int spi_nor_params_show(struct seq_file *s, void *data)
struct spi_nor_erase_type *et = &erase_map->erase_type[i];
if (et->size) {
- string_get_size(et->size, 1, STRING_UNITS_2, buf,
+ string_get_size(et->size, 1, STRING_SIZE_BASE2, buf,
sizeof(buf));
seq_printf(s, " %02x (%s) [%d]\n", et->opcode, buf, i);
}
}
if (!(nor->flags & SNOR_F_NO_OP_CHIP_ERASE)) {
- string_get_size(params->size, 1, STRING_UNITS_2, buf, sizeof(buf));
+ string_get_size(params->size, 1, STRING_SIZE_BASE2, buf, sizeof(buf));
seq_printf(s, " %02x (%s)\n", nor->params->die_erase_opcode, buf);
}
diff --git a/drivers/net/ethernet/chelsio/cxgb4/cxgb4_debugfs.c b/drivers/net/ethernet/chelsio/cxgb4/cxgb4_debugfs.c
index 14e0d989c3ba..7d5fbebd36fc 100644
--- a/drivers/net/ethernet/chelsio/cxgb4/cxgb4_debugfs.c
+++ b/drivers/net/ethernet/chelsio/cxgb4/cxgb4_debugfs.c
@@ -3457,8 +3457,8 @@ static void mem_region_show(struct seq_file *seq, const char *name,
{
char buf[40];
- string_get_size((u64)to - from + 1, 1, STRING_UNITS_2, buf,
- sizeof(buf));
+ string_get_size((u64)to - from + 1, 1, STRING_SIZE_BASE2,
+ buf, sizeof(buf));
seq_printf(seq, "%-15s %#x-%#x [%s]\n", name, from, to, buf);
}
diff --git a/drivers/scsi/sd.c b/drivers/scsi/sd.c
index 0833b3e6aa6e..e23bcb1d1ffa 100644
--- a/drivers/scsi/sd.c
+++ b/drivers/scsi/sd.c
@@ -2731,10 +2731,10 @@ sd_print_capacity(struct scsi_disk *sdkp,
if (!sdkp->first_scan && old_capacity == sdkp->capacity)
return;
- string_get_size(sdkp->capacity, sector_size,
- STRING_UNITS_2, cap_str_2, sizeof(cap_str_2));
- string_get_size(sdkp->capacity, sector_size,
- STRING_UNITS_10, cap_str_10, sizeof(cap_str_10));
+ string_get_size(sdkp->capacity, sector_size, STRING_SIZE_BASE2,
+ cap_str_2, sizeof(cap_str_2));
+ string_get_size(sdkp->capacity, sector_size, 0,
+ cap_str_10, sizeof(cap_str_10));
sd_printk(KERN_NOTICE, sdkp,
"%llu %d-byte logical blocks: (%s/%s)\n",
diff --git a/include/linux/string_helpers.h b/include/linux/string_helpers.h
index 58fb1f90eda5..a54467d891db 100644
--- a/include/linux/string_helpers.h
+++ b/include/linux/string_helpers.h
@@ -17,14 +17,13 @@ static inline bool string_is_terminated(const char *s, int len)
return memchr(s, '\0', len) ? true : false;
}
-/* Descriptions of the types of units to
- * print in */
-enum string_size_units {
- STRING_UNITS_10, /* use powers of 10^3 (standard SI) */
- STRING_UNITS_2, /* use binary powers of 2^10 */
+enum string_size_flags {
+ STRING_SIZE_BASE2 = (1 << 0),
+ STRING_SIZE_NOSPACE = (1 << 1),
+ STRING_SIZE_NOBYTES = (1 << 2),
};
-int string_get_size(u64 size, u64 blk_size, enum string_size_units units,
+int string_get_size(u64 size, u64 blk_size, enum string_size_flags flags,
char *buf, int len);
int parse_int_array_user(const char __user *from, size_t count, int **array);
diff --git a/lib/string_helpers.c b/lib/string_helpers.c
index 7713f73e66b0..a5d7d1caed70 100644
--- a/lib/string_helpers.c
+++ b/lib/string_helpers.c
@@ -19,11 +19,17 @@
#include <linux/string.h>
#include <linux/string_helpers.h>
+enum string_size_units {
+ STRING_UNITS_10, /* use powers of 10^3 (standard SI) */
+ STRING_UNITS_2, /* use binary powers of 2^10 */
+};
+
/**
* string_get_size - get the size in the specified units
* @size: The size to be converted in blocks
* @blk_size: Size of the block (use 1 for size in bytes)
- * @units: units to use (powers of 1000 or 1024)
+ * @flags: units to use (powers of 1000 or 1024), whether to include space
+ * separator
* @buf: buffer to format to
* @len: length of buffer
*
@@ -34,14 +40,16 @@
* Return value: number of characters of output that would have been written
* (which may be greater than len, if output was truncated).
*/
-int string_get_size(u64 size, u64 blk_size, const enum string_size_units units,
+int string_get_size(u64 size, u64 blk_size, enum string_size_flags flags,
char *buf, int len)
{
+ enum string_size_units units = flags & flags & STRING_SIZE_BASE2
+ ? STRING_UNITS_2 : STRING_UNITS_10;
static const char *const units_10[] = {
- "B", "kB", "MB", "GB", "TB", "PB", "EB", "ZB", "YB"
+ "", "k", "M", "G", "T", "P", "E", "Z", "Y"
};
static const char *const units_2[] = {
- "B", "KiB", "MiB", "GiB", "TiB", "PiB", "EiB", "ZiB", "YiB"
+ "", "Ki", "Mi", "Gi", "Ti", "Pi", "Ei", "Zi", "Yi"
};
static const char *const *const units_str[] = {
[STRING_UNITS_10] = units_10,
@@ -128,8 +136,10 @@ int string_get_size(u64 size, u64 blk_size, const enum string_size_units units,
else
unit = units_str[units][i];
- return snprintf(buf, len, "%u%s %s", (u32)size,
- tmp, unit);
+ return snprintf(buf, len, "%u%s%s%s%s", (u32)size, tmp,
+ (flags & STRING_SIZE_NOSPACE) ? "" : " ",
+ unit,
+ (flags & STRING_SIZE_NOBYTES) ? "" : "B");
}
EXPORT_SYMBOL(string_get_size);
diff --git a/lib/test-string_helpers.c b/lib/test-string_helpers.c
index 9a68849a5d55..0b01ffca96fb 100644
--- a/lib/test-string_helpers.c
+++ b/lib/test-string_helpers.c
@@ -507,8 +507,8 @@ static __init void __test_string_get_size(const u64 size, const u64 blk_size,
char buf10[string_get_size_maxbuf];
char buf2[string_get_size_maxbuf];
- string_get_size(size, blk_size, STRING_UNITS_10, buf10, sizeof(buf10));
- string_get_size(size, blk_size, STRING_UNITS_2, buf2, sizeof(buf2));
+ string_get_size(size, blk_size, 0, buf10, sizeof(buf10));
+ string_get_size(size, blk_size, STRING_SIZE_BASE2, buf2, sizeof(buf2));
test_string_get_size_check("STRING_UNITS_10", exp_result10, buf10,
size, blk_size);
diff --git a/mm/hugetlb.c b/mm/hugetlb.c
index ed1581b670d4..26a8028e4bb7 100644
--- a/mm/hugetlb.c
+++ b/mm/hugetlb.c
@@ -3475,7 +3475,7 @@ static void __init hugetlb_hstate_alloc_pages_onenode(struct hstate *h, int nid)
if (i == h->max_huge_pages_node[nid])
return;
- string_get_size(huge_page_size(h), 1, STRING_UNITS_2, buf, 32);
+ string_get_size(huge_page_size(h), 1, STRING_SIZE_BASE2, buf, 32);
pr_warn("HugeTLB: allocating %u of page size %s failed node%d. Only allocated %lu hugepages.\n",
h->max_huge_pages_node[nid], buf, nid, i);
h->max_huge_pages -= (h->max_huge_pages_node[nid] - i);
@@ -3561,7 +3561,7 @@ static void __init hugetlb_hstate_alloc_pages(struct hstate *h)
if (i < h->max_huge_pages) {
char buf[32];
- string_get_size(huge_page_size(h), 1, STRING_UNITS_2, buf, 32);
+ string_get_size(huge_page_size(h), 1, STRING_SIZE_BASE2, buf, 32);
pr_warn("HugeTLB: allocating %lu of page size %s failed. Only allocated %lu hugepages.\n",
h->max_huge_pages, buf, i);
h->max_huge_pages = i;
@@ -3607,7 +3607,7 @@ static void __init report_hugepages(void)
for_each_hstate(h) {
char buf[32];
- string_get_size(huge_page_size(h), 1, STRING_UNITS_2, buf, 32);
+ string_get_size(huge_page_size(h), 1, STRING_SIZE_BASE2, buf, 32);
pr_info("HugeTLB: registered %s page size, pre-allocated %ld pages\n",
buf, h->free_huge_pages);
pr_info("HugeTLB: %d KiB vmemmap can be freed for a %s page\n",
@@ -4527,7 +4527,7 @@ static int __init hugetlb_init(void)
char buf[32];
string_get_size(huge_page_size(&default_hstate),
- 1, STRING_UNITS_2, buf, 32);
+ 1, STRING_SIZE_BASE2, buf, 32);
pr_warn("HugeTLB: Ignoring hugepages=%lu associated with %s page size\n",
default_hstate.max_huge_pages, buf);
pr_warn("HugeTLB: Using hugepages=%lu for number of default huge pages\n",
--
2.43.0.687.g38aa6559b0-goog
On Mon, Feb 12, 2024 at 01:38:47PM -0800, Suren Baghdasaryan wrote: > - string_get_size(size, 1, STRING_UNITS_2, buf, sizeof(buf)); > + string_get_size(size, 1, STRING_SIZE_BASE2, buf, sizeof(buf)); This patch could be a whole lot smaller if ... > +++ b/include/linux/string_helpers.h > @@ -17,14 +17,13 @@ static inline bool string_is_terminated(const char *s, int len) > return memchr(s, '\0', len) ? true : false; > } > > -/* Descriptions of the types of units to > - * print in */ > -enum string_size_units { > - STRING_UNITS_10, /* use powers of 10^3 (standard SI) */ > - STRING_UNITS_2, /* use binary powers of 2^10 */ > +enum string_size_flags { > + STRING_SIZE_BASE2 = (1 << 0), > + STRING_SIZE_NOSPACE = (1 << 1), > + STRING_SIZE_NOBYTES = (1 << 2), you just added: #define STRING_UNITS_10 0 #define STRING_UNITS_2 STRING_SIZE_BASE2 and you wouldn't need to change any of the callers.
On Mon, Feb 12, 2024 at 11:39 PM Suren Baghdasaryan <surenb@google.com> wrote: > > From: Kent Overstreet <kent.overstreet@linux.dev> > > The new flags parameter allows controlling > - Whether or not the units suffix is separated by a space, for > compatibility with sort -h > - Whether or not to append a B suffix - we're not always printing > bytes. > > Signed-off-by: Kent Overstreet <kent.overstreet@linux.dev> > Signed-off-by: Suren Baghdasaryan <surenb@google.com> It seems most of my points from the previous review were refused... ... You can move the below under --- cutter, so it won't pollute the git history. > Cc: Andy Shevchenko <andy@kernel.org> > Cc: Michael Ellerman <mpe@ellerman.id.au> > Cc: Benjamin Herrenschmidt <benh@kernel.crashing.org> > Cc: Paul Mackerras <paulus@samba.org> > Cc: "Michael S. Tsirkin" <mst@redhat.com> > Cc: Jason Wang <jasowang@redhat.com> > Cc: "Noralf Trønnes" <noralf@tronnes.org> > Cc: Jens Axboe <axboe@kernel.dk> > --- ... > --- a/include/linux/string_helpers.h > +++ b/include/linux/string_helpers.h > @@ -17,14 +17,13 @@ static inline bool string_is_terminated(const char *s, int len) ... > -/* Descriptions of the types of units to > - * print in */ > -enum string_size_units { > - STRING_UNITS_10, /* use powers of 10^3 (standard SI) */ > - STRING_UNITS_2, /* use binary powers of 2^10 */ > +enum string_size_flags { > + STRING_SIZE_BASE2 = (1 << 0), > + STRING_SIZE_NOSPACE = (1 << 1), > + STRING_SIZE_NOBYTES = (1 << 2), > }; Do not kill documentation, I already said that. Or i.o.w. document this. Also the _SIZE is ambigous (if you don't want UNITS, use SIZE_FORMAT. Also why did you kill BASE10 here? (see below as well) ... > --- a/lib/string_helpers.c > +++ b/lib/string_helpers.c > @@ -19,11 +19,17 @@ > #include <linux/string.h> > #include <linux/string_helpers.h> > > +enum string_size_units { > + STRING_UNITS_10, /* use powers of 10^3 (standard SI) */ > + STRING_UNITS_2, /* use binary powers of 2^10 */ > +}; Why do we need this duplication? ... > + enum string_size_units units = flags & flags & STRING_SIZE_BASE2 > + ? STRING_UNITS_2 : STRING_UNITS_10; Double flags check is redundant. -- With Best Regards, Andy Shevchenko
On Tue, Feb 13, 2024 at 10:26:48AM +0200, Andy Shevchenko wrote: > On Mon, Feb 12, 2024 at 11:39 PM Suren Baghdasaryan <surenb@google.com> wrote: > > > > From: Kent Overstreet <kent.overstreet@linux.dev> > > > > The new flags parameter allows controlling > > - Whether or not the units suffix is separated by a space, for > > compatibility with sort -h > > - Whether or not to append a B suffix - we're not always printing > > bytes. > > > > Signed-off-by: Kent Overstreet <kent.overstreet@linux.dev> > > Signed-off-by: Suren Baghdasaryan <surenb@google.com> > > ... > > You can move the below under --- cutter, so it won't pollute the git history. > > > Cc: Andy Shevchenko <andy@kernel.org> > > Cc: Michael Ellerman <mpe@ellerman.id.au> > > Cc: Benjamin Herrenschmidt <benh@kernel.crashing.org> > > Cc: Paul Mackerras <paulus@samba.org> > > Cc: "Michael S. Tsirkin" <mst@redhat.com> > > Cc: Jason Wang <jasowang@redhat.com> > > Cc: "Noralf Trønnes" <noralf@tronnes.org> > > Cc: Jens Axboe <axboe@kernel.dk> > > --- > > ... > > > --- a/include/linux/string_helpers.h > > +++ b/include/linux/string_helpers.h > > @@ -17,14 +17,13 @@ static inline bool string_is_terminated(const char *s, int len) > > ... > > > -/* Descriptions of the types of units to > > - * print in */ > > -enum string_size_units { > > - STRING_UNITS_10, /* use powers of 10^3 (standard SI) */ > > - STRING_UNITS_2, /* use binary powers of 2^10 */ > > +enum string_size_flags { > > + STRING_SIZE_BASE2 = (1 << 0), > > + STRING_SIZE_NOSPACE = (1 << 1), > > + STRING_SIZE_NOBYTES = (1 << 2), > > }; > > Do not kill documentation, I already said that. Or i.o.w. document this. > Also the _SIZE is ambigous (if you don't want UNITS, use SIZE_FORMAT. > > Also why did you kill BASE10 here? (see below as well) As you should be able to tell from the name, it's a set of flags. > > --- a/lib/string_helpers.c > > +++ b/lib/string_helpers.c > > @@ -19,11 +19,17 @@ > > #include <linux/string.h> > > #include <linux/string_helpers.h> > > > > +enum string_size_units { > > + STRING_UNITS_10, /* use powers of 10^3 (standard SI) */ > > + STRING_UNITS_2, /* use binary powers of 2^10 */ > > +}; > > Why do we need this duplication? Because otherwise a lot more code would have to change. > > It seems most of my points from the previous review were refused... Look, Andy, this is a pretty tiny part of the patchset, yet it's been eating up a pretty disproprortionate amount of time and your review feedback has been pretty unhelpful - asking for things to be broken up in ways that would not be bisectable, or (as here) re-asking the same things that I've already answered and that should've been obvious. The code works. If you wish to complain about anything being broken, or if you can come up with anything more actionable than what you've got here, I will absolutely respond to that, but otherwise I'm just going to leave things where they sit.
On Tue, Feb 13, 2024 at 05:06:24PM -0500, Kent Overstreet wrote: > On Tue, Feb 13, 2024 at 10:26:48AM +0200, Andy Shevchenko wrote: > > On Mon, Feb 12, 2024 at 11:39 PM Suren Baghdasaryan <surenb@google.com> wrote: > > It seems most of my points from the previous review were refused... > > Look, Andy, this is a pretty tiny part of the patchset, yet it's been > eating up a pretty disproprortionate amount of time and your review > feedback has been pretty unhelpful - asking for things to be broken up > in ways that would not be bisectable, or (as here) re-asking the same > things that I've already answered and that should've been obvious. > > The code works. If you wish to complain about anything being broken, or > if you can come up with anything more actionable than what you've got > here, I will absolutely respond to that, but otherwise I'm just going to > leave things where they sit. I do not understand why I should do *your* job. Nevertheless, I have just sent my version of this change. Enjoy! -- With Best Regards, Andy Shevchenko
On Tue, Feb 13, 2024 at 10:26 AM Andy Shevchenko <andy.shevchenko@gmail.com> wrote: > > On Mon, Feb 12, 2024 at 11:39 PM Suren Baghdasaryan <surenb@google.com> wrote: > > > > From: Kent Overstreet <kent.overstreet@linux.dev> > > > > The new flags parameter allows controlling > > - Whether or not the units suffix is separated by a space, for > > compatibility with sort -h > > - Whether or not to append a B suffix - we're not always printing > > bytes. And you effectively missed to _add_ the test cases for the modified code. Formal NAK for this, the rest is discussable, the absence of tests is not. > > Signed-off-by: Kent Overstreet <kent.overstreet@linux.dev> > > Signed-off-by: Suren Baghdasaryan <surenb@google.com> > > It seems most of my points from the previous review were refused... > > ... > > You can move the below under --- cutter, so it won't pollute the git history. > > > Cc: Andy Shevchenko <andy@kernel.org> > > Cc: Michael Ellerman <mpe@ellerman.id.au> > > Cc: Benjamin Herrenschmidt <benh@kernel.crashing.org> > > Cc: Paul Mackerras <paulus@samba.org> > > Cc: "Michael S. Tsirkin" <mst@redhat.com> > > Cc: Jason Wang <jasowang@redhat.com> > > Cc: "Noralf Trønnes" <noralf@tronnes.org> > > Cc: Jens Axboe <axboe@kernel.dk> > > --- > > ... > > > --- a/include/linux/string_helpers.h > > +++ b/include/linux/string_helpers.h > > @@ -17,14 +17,13 @@ static inline bool string_is_terminated(const char *s, int len) > > ... > > > -/* Descriptions of the types of units to > > - * print in */ > > -enum string_size_units { > > - STRING_UNITS_10, /* use powers of 10^3 (standard SI) */ > > - STRING_UNITS_2, /* use binary powers of 2^10 */ > > +enum string_size_flags { > > + STRING_SIZE_BASE2 = (1 << 0), > > + STRING_SIZE_NOSPACE = (1 << 1), > > + STRING_SIZE_NOBYTES = (1 << 2), > > }; > > Do not kill documentation, I already said that. Or i.o.w. document this. > Also the _SIZE is ambigous (if you don't want UNITS, use SIZE_FORMAT. > > Also why did you kill BASE10 here? (see below as well) > > ... > > > --- a/lib/string_helpers.c > > +++ b/lib/string_helpers.c > > @@ -19,11 +19,17 @@ > > #include <linux/string.h> > > #include <linux/string_helpers.h> > > > > +enum string_size_units { > > + STRING_UNITS_10, /* use powers of 10^3 (standard SI) */ > > + STRING_UNITS_2, /* use binary powers of 2^10 */ > > +}; > > Why do we need this duplication? > > ... > > > + enum string_size_units units = flags & flags & STRING_SIZE_BASE2 > > + ? STRING_UNITS_2 : STRING_UNITS_10; > > Double flags check is redundant. -- With Best Regards, Andy Shevchenko
On Tue, Feb 13, 2024 at 10:29:34AM +0200, Andy Shevchenko wrote: > On Tue, Feb 13, 2024 at 10:26 AM Andy Shevchenko > <andy.shevchenko@gmail.com> wrote: > > > > On Mon, Feb 12, 2024 at 11:39 PM Suren Baghdasaryan <surenb@google.com> wrote: > > > > > > From: Kent Overstreet <kent.overstreet@linux.dev> > > > > > > The new flags parameter allows controlling > > > - Whether or not the units suffix is separated by a space, for > > > compatibility with sort -h > > > - Whether or not to append a B suffix - we're not always printing > > > bytes. > > And you effectively missed to _add_ the test cases for the modified code. > Formal NAK for this, the rest is discussable, the absence of tests is not. Eh? The core algorihtm for printing out a number in human readable units; that's definitely worth a test, and I assume there already is one - I didn't touch that. But whether or not the units suffix has a space, or a B suffix? That's not going to break in subtle ways; that either works or it doesn't.
On Mon, Feb 12, 2024 at 01:38:47PM -0800, Suren Baghdasaryan wrote: > From: Kent Overstreet <kent.overstreet@linux.dev> > > The new flags parameter allows controlling > - Whether or not the units suffix is separated by a space, for > compatibility with sort -h > - Whether or not to append a B suffix - we're not always printing > bytes. > > Signed-off-by: Kent Overstreet <kent.overstreet@linux.dev> If there is a v4, please include some short examples with the .h bit field documentation. It's pretty obvious right now but these kinds of things have a habit of growing, so let's set a good example for documenting the flags. Reviewed-by: Kees Cook <keescook@chromium.org> -- Kees Cook
© 2016 - 2024 Red Hat, Inc.