| Message ID | 20241225100949.930897-2-shinichiro.kawasaki@wdc.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | null_blk: improve write failure simulation | expand |
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@acm.org> > Signed-off-by: Shin'ichiro Kawasaki <shinichiro.kawasaki@wdc.com> Nice cleanup ! One nit below. And with that fixed, feel free to add: Reviewed-by: Damien Le Moal <dlemoal@kernel.org> > @@ -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" ? Note that we are nowhere near to hit this though, nor should we ever reach a 4K string length for null_blk features :) > + } > + left -= ret; > + written += ret; > + } > + > + return written; > } > > CONFIGFS_ATTR_RO(memb_group_, features);
On 12/25/24 2:09 AM, Shin'ichiro Kawasaki wrote: > Also, modify order of the > nullb_device_attrs table to not change the list order of the generated > string. No software should depend on the order of the names of the attributes in the help text, isn't it? Mentioning that the order has been changed into alphabetical order may be sufficient. > +/* > + * Place the elements in alphabetical order to keep the confingfs > + * 'features' string readable. > + */ confingfs -> configfs Thank you for having sorted this list in alphabetical order. Additionally, I don't think that a reference to the 'features' string is necessary here. It is a common practice in Linux kernel code to keeps list of things sorted if the order doesn't matter. > + if (!*(entry + 1)) > + fmt = "%s\n"; > + ret = snprintf(page + written, left, fmt, (*entry)->ca_name); Compilers do not support checking the format specifiers if the format specification is not constant. Please make the format specification constant, e.g. "%s%c" instead of fmt. Thanks, Bart.
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@acm.org> > > Signed-off-by: Shin'ichiro Kawasaki <shinichiro.kawasaki@wdc.com> > > Nice cleanup ! One nit below. And with that fixed, feel free to add: > > Reviewed-by: Damien Le Moal <dlemoal@kernel.org> 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.
On Jan 06, 2025 / 09:53, Bart Van Assche wrote: > On 12/25/24 2:09 AM, Shin'ichiro Kawasaki wrote: > > Also, modify order of the > > nullb_device_attrs table to not change the list order of the generated > > string. > > No software should depend on the order of the names of the attributes in > the help text, isn't it? Mentioning that the order has been changed into > alphabetical order may be sufficient. Thanks, refelcted to v3. > > > +/* > > + * Place the elements in alphabetical order to keep the confingfs > > + * 'features' string readable. > > + */ > > confingfs -> configfs > > Thank you for having sorted this list in alphabetical order. > > Additionally, I don't think that a reference to the 'features' string is > necessary here. It is a common practice in Linux kernel code to keeps > list of things sorted if the order doesn't matter. Thanks. Now I think the block comment is not meaningful, so I removed it in v3. > > > + if (!*(entry + 1)) > > + fmt = "%s\n"; > > + ret = snprintf(page + written, left, fmt, (*entry)->ca_name); > > Compilers do not support checking the format specifiers if the format > specification is not constant. Please make the format specification > constant, e.g. "%s%c" instead of fmt. Good idea. Reflected to v3. Thanks!
diff --git a/drivers/block/null_blk/main.c b/drivers/block/null_blk/main.c index 178e62cd9a9f..f720707b7cfb 100644 --- a/drivers/block/null_blk/main.c +++ b/drivers/block/null_blk/main.c @@ -591,42 +591,46 @@ static ssize_t nullb_device_zone_offline_store(struct config_item *item, } CONFIGFS_ATTR_WO(nullb_device_, zone_offline); +/* + * Place the elements in alphabetical order to keep the confingfs + * 'features' string readable. + */ static struct configfs_attribute *nullb_device_attrs[] = { - &nullb_device_attr_size, - &nullb_device_attr_completion_nsec, - &nullb_device_attr_submit_queues, - &nullb_device_attr_poll_queues, - &nullb_device_attr_home_node, - &nullb_device_attr_queue_mode, + &nullb_device_attr_badblocks, + &nullb_device_attr_blocking, &nullb_device_attr_blocksize, - &nullb_device_attr_max_sectors, - &nullb_device_attr_irqmode, + &nullb_device_attr_cache_size, + &nullb_device_attr_completion_nsec, + &nullb_device_attr_discard, + &nullb_device_attr_fua, + &nullb_device_attr_home_node, &nullb_device_attr_hw_queue_depth, &nullb_device_attr_index, - &nullb_device_attr_blocking, - &nullb_device_attr_use_per_node_hctx, - &nullb_device_attr_power, - &nullb_device_attr_memory_backed, - &nullb_device_attr_discard, + &nullb_device_attr_irqmode, + &nullb_device_attr_max_sectors, &nullb_device_attr_mbps, - &nullb_device_attr_cache_size, - &nullb_device_attr_badblocks, - &nullb_device_attr_zoned, - &nullb_device_attr_zone_size, - &nullb_device_attr_zone_capacity, - &nullb_device_attr_zone_nr_conv, - &nullb_device_attr_zone_max_open, - &nullb_device_attr_zone_max_active, - &nullb_device_attr_zone_append_max_sectors, - &nullb_device_attr_zone_readonly, - &nullb_device_attr_zone_offline, - &nullb_device_attr_zone_full, - &nullb_device_attr_virt_boundary, + &nullb_device_attr_memory_backed, &nullb_device_attr_no_sched, - &nullb_device_attr_shared_tags, - &nullb_device_attr_shared_tag_bitmap, - &nullb_device_attr_fua, + &nullb_device_attr_poll_queues, + &nullb_device_attr_power, + &nullb_device_attr_queue_mode, &nullb_device_attr_rotational, + &nullb_device_attr_shared_tag_bitmap, + &nullb_device_attr_shared_tags, + &nullb_device_attr_size, + &nullb_device_attr_submit_queues, + &nullb_device_attr_use_per_node_hctx, + &nullb_device_attr_virt_boundary, + &nullb_device_attr_zone_append_max_sectors, + &nullb_device_attr_zone_capacity, + &nullb_device_attr_zone_full, + &nullb_device_attr_zone_max_active, + &nullb_device_attr_zone_max_open, + &nullb_device_attr_zone_nr_conv, + &nullb_device_attr_zone_offline, + &nullb_device_attr_zone_readonly, + &nullb_device_attr_zone_size, + &nullb_device_attr_zoned, NULL, }; @@ -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; + } + left -= ret; + written += ret; + } + + return written; } CONFIGFS_ATTR_RO(memb_group_, features);
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@acm.org> Signed-off-by: Shin'ichiro Kawasaki <shinichiro.kawasaki@wdc.com> --- drivers/block/null_blk/main.c | 93 ++++++++++++++++++++--------------- 1 file changed, 54 insertions(+), 39 deletions(-)