On Jan 06, 2025 / 14:43, Damien Le Moal wrote:
> On 12/25/24 7:09 PM, Shin'ichiro Kawasaki wrote:
> > The null_blk configfs file 'features' provides a string that lists
> > available null_blk features for userspace programs to reference.
> > The string is defined as a long constant in the code, which tends to be
> > forgotten for updates. It also causes checkpatch.pl to report
> > "WARNING: quoted string split across lines".
> >
> > To avoid these drawbacks, generate the feature string on the fly. Refer
> > to the ca_name field of each element in the nullb_device_attrs table and
> > concatenate them in the given buffer. Also, modify order of the
> > nullb_device_attrs table to not change the list order of the generated
> > string.
> >
> > Of note is that the feature "index" was missing before this commit.
> > This commit adds it to the generated string.
> >
> > Suggested-by: Bart Van Assche <bvanassche@xxxxxxx>
> > Signed-off-by: Shin'ichiro Kawasaki <shinichiro.kawasaki@xxxxxxx>
>
> Nice cleanup ! One nit below. And with that fixed, feel free to add:
>
> Reviewed-by: Damien Le Moal <dlemoal@xxxxxxxxxx>
Thanks, but I modified the patch for v3 based on Bart's comment, so still
have not yet added your Reviewed-by tag.
>
> > @@ -704,16 +708,27 @@ nullb_group_drop_item(struct config_group *group, struct config_item *item)
> >
> > static ssize_t memb_group_features_show(struct config_item *item, char *page)
> > {
> > - return snprintf(page, PAGE_SIZE,
> > - "badblocks,blocking,blocksize,cache_size,fua,"
> > - "completion_nsec,discard,home_node,hw_queue_depth,"
> > - "irqmode,max_sectors,mbps,memory_backed,no_sched,"
> > - "poll_queues,power,queue_mode,shared_tag_bitmap,"
> > - "shared_tags,size,submit_queues,use_per_node_hctx,"
> > - "virt_boundary,zoned,zone_capacity,zone_max_active,"
> > - "zone_max_open,zone_nr_conv,zone_offline,zone_readonly,"
> > - "zone_size,zone_append_max_sectors,zone_full,"
> > - "rotational\n");
> > +
> > + struct configfs_attribute **entry;
> > + const char *fmt = "%s,";
> > + size_t left = PAGE_SIZE;
> > + size_t written = 0;
> > + int ret;
> > +
> > + for (entry = &nullb_device_attrs[0]; *entry && left > 0; entry++) {
> > + if (!*(entry + 1))
> > + fmt = "%s\n";
> > + ret = snprintf(page + written, left, fmt, (*entry)->ca_name);
> > + if (ret >= left) {
> > + WARN_ONCE(1, "Too many null_blk features to print\n");
> > + memzero_explicit(page, PAGE_SIZE);
> > + return 0;
>
> Let's return an error here. Maybe "-EFBIG" ?
Thanks. I checked errno.h and found -ENOBUFS looks appropriate.
> Note that we are nowhere near to hit this though, nor should we ever reach a 4K
> string length for null_blk features :)
Right, I have same understanding.
