| Message ID | YzHyniCyf+G/2xI8@fedora (mailing list archive) |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | None | expand |
On Mon, 26 Sep 2022 19:42:38 +0100 Jules Irenge wrote: > Coccinnelle reports a warning > Warning: Use scnprintf or sprintf > Adding to that, there has been a slow migration from snprintf to scnprintf. > This LWN article explains the rationale for this change > https: //lwn.net/Articles/69419/ > Ie. snprintf() returns what *would* be the resulting length, > while scnprintf() returns the actual length. > > Signed-off-by: Jules Irenge <jbi.octave@gmail.com> Looks legit but please repost this separately. We only see patch 3 of the series.
On 27.09.22 02:33, Jakub Kicinski wrote: > On Mon, 26 Sep 2022 19:42:38 +0100 Jules Irenge wrote: >> Coccinnelle reports a warning >> Warning: Use scnprintf or sprintf >> Adding to that, there has been a slow migration from snprintf to scnprintf. >> This LWN article explains the rationale for this change >> https: //lwn.net/Articles/69419/ >> Ie. snprintf() returns what *would* be the resulting length, >> while scnprintf() returns the actual length. >> >> Signed-off-by: Jules Irenge <jbi.octave@gmail.com> > > Looks legit but please repost this separately. > We only see patch 3 of the series. When you repost, you can add Reviewed-by: Alexandra Winter <wintera@linux.ibm.com> Thank you Alexandra
On Mon, 2022-09-26 at 19:42 +0100, Jules Irenge wrote: > Coccinnelle reports a warning > Warning: Use scnprintf or sprintf > Adding to that, there has been a slow migration from snprintf to scnprintf. > This LWN article explains the rationale for this change > https: //lwn.net/Articles/69419/ > Ie. snprintf() returns what *would* be the resulting length, > while scnprintf() returns the actual length. [] > diff --git a/drivers/s390/net/qeth_core_sys.c b/drivers/s390/net/qeth_core_sys.c [] > @@ -500,9 +500,9 @@ static ssize_t qeth_hw_trap_show(struct device *dev, > struct qeth_card *card = dev_get_drvdata(dev); > > if (card->info.hwtrap) > - return snprintf(buf, 5, "arm\n"); > + return scnprintf(buf, 5, "arm\n"); > else > - return snprintf(buf, 8, "disarm\n"); > + return scnprintf(buf, 8, "disarm\n"); > } Use sysfs_emit instead. For the entire file, perhaps something like: (untested) --- drivers/s390/net/qeth_core_sys.c | 109 +++++++++++++++++++++------------------ 1 file changed, 60 insertions(+), 49 deletions(-) diff --git a/drivers/s390/net/qeth_core_sys.c b/drivers/s390/net/qeth_core_sys.c index 406be169173ce..d7d6fd78129b3 100644 --- a/drivers/s390/net/qeth_core_sys.c +++ b/drivers/s390/net/qeth_core_sys.c @@ -20,19 +20,21 @@ static ssize_t qeth_dev_state_show(struct device *dev, struct device_attribute *attr, char *buf) { struct qeth_card *card = dev_get_drvdata(dev); + const char *type = "UNKNOWN"; switch (card->state) { case CARD_STATE_DOWN: - return sprintf(buf, "DOWN\n"); + type = "DOWN"; + break; case CARD_STATE_SOFTSETUP: - if (card->dev->flags & IFF_UP) - return sprintf(buf, "UP (LAN %s)\n", - netif_carrier_ok(card->dev) ? "ONLINE" : - "OFFLINE"); - return sprintf(buf, "SOFTSETUP\n"); - default: - return sprintf(buf, "UNKNOWN\n"); + if (!(card->dev->flags & IFF_UP)) { + type = "SOFTSETUP"; + break; + } + return sysfs_emit(buf, "UP (LAN %sLINE)\n", + netif_carrier_ok(card->dev) ? "ON" : "OFF"); } + return sysfs_emit(buf, "%s\n", type); } static DEVICE_ATTR(state, 0444, qeth_dev_state_show, NULL); @@ -42,7 +44,7 @@ static ssize_t qeth_dev_chpid_show(struct device *dev, { struct qeth_card *card = dev_get_drvdata(dev); - return sprintf(buf, "%02X\n", card->info.chpid); + return sysfs_emit(buf, "%02X\n", card->info.chpid); } static DEVICE_ATTR(chpid, 0444, qeth_dev_chpid_show, NULL); @@ -52,7 +54,7 @@ static ssize_t qeth_dev_if_name_show(struct device *dev, { struct qeth_card *card = dev_get_drvdata(dev); - return sprintf(buf, "%s\n", netdev_name(card->dev)); + return sysfs_emit(buf, "%s\n", netdev_name(card->dev)); } static DEVICE_ATTR(if_name, 0444, qeth_dev_if_name_show, NULL); @@ -62,7 +64,7 @@ static ssize_t qeth_dev_card_type_show(struct device *dev, { struct qeth_card *card = dev_get_drvdata(dev); - return sprintf(buf, "%s\n", qeth_get_cardname_short(card)); + return sysfs_emit(buf, "%s\n", qeth_get_cardname_short(card)); } static DEVICE_ATTR(card_type, 0444, qeth_dev_card_type_show, NULL); @@ -86,7 +88,7 @@ static ssize_t qeth_dev_inbuf_size_show(struct device *dev, { struct qeth_card *card = dev_get_drvdata(dev); - return sprintf(buf, "%s\n", qeth_get_bufsize_str(card)); + return sysfs_emit(buf, "%s\n", qeth_get_bufsize_str(card)); } static DEVICE_ATTR(inbuf_size, 0444, qeth_dev_inbuf_size_show, NULL); @@ -96,7 +98,7 @@ static ssize_t qeth_dev_portno_show(struct device *dev, { struct qeth_card *card = dev_get_drvdata(dev); - return sprintf(buf, "%i\n", card->dev->dev_port); + return sysfs_emit(buf, "%i\n", card->dev->dev_port); } static ssize_t qeth_dev_portno_store(struct device *dev, @@ -134,7 +136,7 @@ static DEVICE_ATTR(portno, 0644, qeth_dev_portno_show, qeth_dev_portno_store); static ssize_t qeth_dev_portname_show(struct device *dev, struct device_attribute *attr, char *buf) { - return sprintf(buf, "no portname required\n"); + return sysfs_emit(buf, "no portname required\n"); } static ssize_t qeth_dev_portname_store(struct device *dev, @@ -154,22 +156,27 @@ static ssize_t qeth_dev_prioqing_show(struct device *dev, struct device_attribute *attr, char *buf) { struct qeth_card *card = dev_get_drvdata(dev); + const char *type = "disabled"; switch (card->qdio.do_prio_queueing) { case QETH_PRIO_Q_ING_PREC: - return sprintf(buf, "%s\n", "by precedence"); + type = "by precedence"; + break; case QETH_PRIO_Q_ING_TOS: - return sprintf(buf, "%s\n", "by type of service"); + type = "by type of service"; + break; case QETH_PRIO_Q_ING_SKB: - return sprintf(buf, "%s\n", "by skb-priority"); + type = "by skb-priority"; + break; case QETH_PRIO_Q_ING_VLAN: - return sprintf(buf, "%s\n", "by VLAN headers"); + type = "by VLAN headers"; + break; case QETH_PRIO_Q_ING_FIXED: - return sprintf(buf, "always queue %i\n", - card->qdio.default_out_queue); - default: - return sprintf(buf, "disabled\n"); + return sysfs_emit(buf, "always queue %i\n", + card->qdio.default_out_queue); } + + return sysfs_emit(buf, "%s\n", type); } static ssize_t qeth_dev_prioqing_store(struct device *dev, @@ -242,7 +249,7 @@ static ssize_t qeth_dev_bufcnt_show(struct device *dev, { struct qeth_card *card = dev_get_drvdata(dev); - return sprintf(buf, "%i\n", card->qdio.in_buf_pool.buf_count); + return sysfs_emit(buf, "%i\n", card->qdio.in_buf_pool.buf_count); } static ssize_t qeth_dev_bufcnt_store(struct device *dev, @@ -298,7 +305,7 @@ static DEVICE_ATTR(recover, 0200, NULL, qeth_dev_recover_store); static ssize_t qeth_dev_performance_stats_show(struct device *dev, struct device_attribute *attr, char *buf) { - return sprintf(buf, "1\n"); + return sysfs_emit(buf, "1\n"); } static ssize_t qeth_dev_performance_stats_store(struct device *dev, @@ -335,7 +342,7 @@ static ssize_t qeth_dev_layer2_show(struct device *dev, { struct qeth_card *card = dev_get_drvdata(dev); - return sprintf(buf, "%i\n", card->options.layer); + return sysfs_emit(buf, "%i\n", card->options.layer); } static ssize_t qeth_dev_layer2_store(struct device *dev, @@ -407,17 +414,21 @@ static ssize_t qeth_dev_isolation_show(struct device *dev, struct device_attribute *attr, char *buf) { struct qeth_card *card = dev_get_drvdata(dev); + const char *type = "N/A"; switch (card->options.isolation) { case ISOLATION_MODE_NONE: - return snprintf(buf, 6, "%s\n", ATTR_QETH_ISOLATION_NONE); + type = ATTR_QETH_ISOLATION_NONE; + break; case ISOLATION_MODE_FWD: - return snprintf(buf, 9, "%s\n", ATTR_QETH_ISOLATION_FWD); + type = ATTR_QETH_ISOLATION_FWD; + break; case ISOLATION_MODE_DROP: - return snprintf(buf, 6, "%s\n", ATTR_QETH_ISOLATION_DROP); - default: - return snprintf(buf, 5, "%s\n", "N/A"); + type = ATTR_QETH_ISOLATION_DROP; + break; } + + return sysfs_emit("%s\n", type); } static ssize_t qeth_dev_isolation_store(struct device *dev, @@ -467,28 +478,31 @@ static ssize_t qeth_dev_switch_attrs_show(struct device *dev, { struct qeth_card *card = dev_get_drvdata(dev); struct qeth_switch_info sw_info; - int rc = 0; + int len = 0; + int rc; if (!qeth_card_hw_is_reachable(card)) - return sprintf(buf, "n/a\n"); + return sysfs_emit(buf, "n/a\n"); rc = qeth_query_switch_attributes(card, &sw_info); if (rc) return rc; if (!sw_info.capabilities) - rc = sprintf(buf, "unknown"); + return sysfs_emit(buf, "unknown\n"); if (sw_info.capabilities & QETH_SWITCH_FORW_802_1) - rc = sprintf(buf, (sw_info.settings & QETH_SWITCH_FORW_802_1 ? - "[802.1]" : "802.1")); - if (sw_info.capabilities & QETH_SWITCH_FORW_REFL_RELAY) - rc += sprintf(buf + rc, - (sw_info.settings & QETH_SWITCH_FORW_REFL_RELAY ? - " [rr]" : " rr")); - rc += sprintf(buf + rc, "\n"); - - return rc; + len += sysfs_emit_at(buf, len, + sw_info.settings & QETH_SWITCH_FORW_802_1 ? + "[802.1]" : "802.1"); + if (sw_info.capabilities & QETH_SWITCH_FORW_REFL_RELAY) { + if (len) + len += sysfs_emit_at(buf, len, " "); + len += sysfs_emit_at(buf, len, + sw_info.settings & QETH_SWITCH_FORW_REFL_RELAY ? + "[rr]" : "rr"); + } + return sysfs_emit_at(buf, len, "\n"); } static DEVICE_ATTR(switch_attrs, 0444, @@ -499,10 +513,7 @@ static ssize_t qeth_hw_trap_show(struct device *dev, { struct qeth_card *card = dev_get_drvdata(dev); - if (card->info.hwtrap) - return snprintf(buf, 5, "arm\n"); - else - return snprintf(buf, 8, "disarm\n"); + return sysfs_emit(buf, "%s\n", card->info.hwtrap ? "arm" : "disarm"); } static ssize_t qeth_hw_trap_store(struct device *dev, @@ -573,7 +584,7 @@ static ssize_t qeth_dev_blkt_total_show(struct device *dev, { struct qeth_card *card = dev_get_drvdata(dev); - return sprintf(buf, "%i\n", card->info.blkt.time_total); + return sysfs_emit(buf, "%i\n", card->info.blkt.time_total); } static ssize_t qeth_dev_blkt_total_store(struct device *dev, @@ -593,7 +604,7 @@ static ssize_t qeth_dev_blkt_inter_show(struct device *dev, { struct qeth_card *card = dev_get_drvdata(dev); - return sprintf(buf, "%i\n", card->info.blkt.inter_packet); + return sysfs_emit(buf, "%i\n", card->info.blkt.inter_packet); } static ssize_t qeth_dev_blkt_inter_store(struct device *dev, @@ -613,7 +624,7 @@ static ssize_t qeth_dev_blkt_inter_jumbo_show(struct device *dev, { struct qeth_card *card = dev_get_drvdata(dev); - return sprintf(buf, "%i\n", card->info.blkt.inter_packet_jumbo); + return sysfs_emit(buf, "%i\n", card->info.blkt.inter_packet_jumbo); } static ssize_t qeth_dev_blkt_inter_jumbo_store(struct device *dev,
On 27.09.22 16:27, Joe Perches wrote: > On Mon, 2022-09-26 at 19:42 +0100, Jules Irenge wrote: >> Coccinnelle reports a warning >> Warning: Use scnprintf or sprintf >> Adding to that, there has been a slow migration from snprintf to scnprintf. >> This LWN article explains the rationale for this change >> https: //lwn.net/Articles/69419/ >> Ie. snprintf() returns what *would* be the resulting length, >> while scnprintf() returns the actual length. > [] >> diff --git a/drivers/s390/net/qeth_core_sys.c b/drivers/s390/net/qeth_core_sys.c > [] >> @@ -500,9 +500,9 @@ static ssize_t qeth_hw_trap_show(struct device *dev, >> struct qeth_card *card = dev_get_drvdata(dev); >> >> if (card->info.hwtrap) >> - return snprintf(buf, 5, "arm\n"); >> + return scnprintf(buf, 5, "arm\n"); >> else >> - return snprintf(buf, 8, "disarm\n"); >> + return scnprintf(buf, 8, "disarm\n"); >> } > > Use sysfs_emit instead. > Thank you Joe, that sounds like the best way to handle this. I propose that I take this onto my ToDo list and test it in the s390 environment. I will add Reported-by: Jules Irenge <jbi.octave@gmail.com> Suggested-by: Joe Perches <joe@perches.com>
On Wed, 2022-09-28 at 10:24 +0200, Alexandra Winter wrote: > On 27.09.22 16:27, Joe Perches wrote: > > On Mon, 2022-09-26 at 19:42 +0100, Jules Irenge wrote: > > > Coccinnelle reports a warning > > > Warning: Use scnprintf or sprintf > > > Adding to that, there has been a slow migration from snprintf to scnprintf. > > > This LWN article explains the rationale for this change > > > https: //lwn.net/Articles/69419/ > > > Ie. snprintf() returns what *would* be the resulting length, > > > while scnprintf() returns the actual length. > > [] > > > diff --git a/drivers/s390/net/qeth_core_sys.c b/drivers/s390/net/qeth_core_sys.c > > [] > > > @@ -500,9 +500,9 @@ static ssize_t qeth_hw_trap_show(struct device *dev, > > > struct qeth_card *card = dev_get_drvdata(dev); > > > > > > if (card->info.hwtrap) > > > - return snprintf(buf, 5, "arm\n"); > > > + return scnprintf(buf, 5, "arm\n"); > > > else > > > - return snprintf(buf, 8, "disarm\n"); > > > + return scnprintf(buf, 8, "disarm\n"); > > > } > > > > Use sysfs_emit instead. > > > > Thank you Joe, that sounds like the best way to handle this. > I propose that I take this onto my ToDo list and test it in the s390 environment. > I will add > Reported-by: Jules Irenge <jbi.octave@gmail.com> > Suggested-by: Joe Perches <joe@perches.com> > Thanks. btw: I was careless when I wrote one section of the proposed patch. In this patch block, @@ -467,28 +478,31 @@ static ssize_t qeth_dev_switch_attrs_show(struct device *dev, The last line + return sysfs_emit_at(buf, len, "\n"); is not correct It needs to be changed to: len += sysfs_emit_at(buf, len, "\n"); return len;
diff --git a/drivers/s390/net/qeth_core_sys.c b/drivers/s390/net/qeth_core_sys.c index 406be169173c..b40802d707a1 100644 --- a/drivers/s390/net/qeth_core_sys.c +++ b/drivers/s390/net/qeth_core_sys.c @@ -500,9 +500,9 @@ static ssize_t qeth_hw_trap_show(struct device *dev, struct qeth_card *card = dev_get_drvdata(dev); if (card->info.hwtrap) - return snprintf(buf, 5, "arm\n"); + return scnprintf(buf, 5, "arm\n"); else - return snprintf(buf, 8, "disarm\n"); + return scnprintf(buf, 8, "disarm\n"); } static ssize_t qeth_hw_trap_store(struct device *dev,
Coccinnelle reports a warning Warning: Use scnprintf or sprintf Adding to that, there has been a slow migration from snprintf to scnprintf. This LWN article explains the rationale for this change https: //lwn.net/Articles/69419/ Ie. snprintf() returns what *would* be the resulting length, while scnprintf() returns the actual length. Signed-off-by: Jules Irenge <jbi.octave@gmail.com> --- drivers/s390/net/qeth_core_sys.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)