bcachefs: Use snprintf() instead of scnprintf() when appropriate
Commit Message
snprintf() and scnprintf() are the same, except for the returned value.
When this value is not used, it is more logical to use snprintf() which is
slightly simpler.
Signed-off-by: Christophe JAILLET <christophe.jaillet@wanadoo.fr>
---
fs/bcachefs/super.c | 2 +-
fs/bcachefs/tests.c | 2 +-
fs/bcachefs/trace.h | 2 +-
3 files changed, 3 insertions(+), 3 deletions(-)
Comments
On Sat, Sep 16, 2023 at 09:30:19AM +0200, Christophe JAILLET wrote:
> snprintf() and scnprintf() are the same, except for the returned value.
> When this value is not used, it is more logical to use snprintf() which is
> slightly simpler.
>
> Signed-off-by: Christophe JAILLET <christophe.jaillet@wanadoo.fr>
> ---
Seems reasonable:
Reviewed-by: Brian Foster <bfoster@redhat.com>
> fs/bcachefs/super.c | 2 +-
> fs/bcachefs/tests.c | 2 +-
> fs/bcachefs/trace.h | 2 +-
> 3 files changed, 3 insertions(+), 3 deletions(-)
>
> diff --git a/fs/bcachefs/super.c b/fs/bcachefs/super.c
> index 2990eed85adf..773ea93e44c1 100644
> --- a/fs/bcachefs/super.c
> +++ b/fs/bcachefs/super.c
> @@ -1180,7 +1180,7 @@ static void bch2_dev_attach(struct bch_fs *c, struct bch_dev *ca,
> {
> ca->dev_idx = dev_idx;
> __set_bit(ca->dev_idx, ca->self.d);
> - scnprintf(ca->name, sizeof(ca->name), "dev-%u", dev_idx);
> + snprintf(ca->name, sizeof(ca->name), "dev-%u", dev_idx);
>
> ca->fs = c;
> rcu_assign_pointer(c->devs[ca->dev_idx], ca);
> diff --git a/fs/bcachefs/tests.c b/fs/bcachefs/tests.c
> index c907b3e00176..72f9bf186f9c 100644
> --- a/fs/bcachefs/tests.c
> +++ b/fs/bcachefs/tests.c
> @@ -926,7 +926,7 @@ int bch2_btree_perf_test(struct bch_fs *c, const char *testname,
>
> time = j.finish - j.start;
>
> - scnprintf(name_buf, sizeof(name_buf), "%s:", testname);
> + snprintf(name_buf, sizeof(name_buf), "%s:", testname);
> prt_human_readable_u64(&nr_buf, nr);
> prt_human_readable_u64(&per_sec_buf, div64_u64(nr * NSEC_PER_SEC, time));
> printk(KERN_INFO "%-12s %s with %u threads in %5llu sec, %5llu nsec per iter, %5s per sec\n",
> diff --git a/fs/bcachefs/trace.h b/fs/bcachefs/trace.h
> index 19264492151b..da303dd4b71c 100644
> --- a/fs/bcachefs/trace.h
> +++ b/fs/bcachefs/trace.h
> @@ -450,7 +450,7 @@ TRACE_EVENT(btree_path_relock_fail,
> c = six_lock_counts(&path->l[level].b->c.lock);
> __entry->read_count = c.n[SIX_LOCK_read];
> __entry->intent_count = c.n[SIX_LOCK_intent];
> - scnprintf(__entry->node, sizeof(__entry->node), "%px", b);
> + snprintf(__entry->node, sizeof(__entry->node), "%px", b);
> }
> __entry->iter_lock_seq = path->l[level].lock_seq;
> __entry->node_lock_seq = is_btree_node(path, level)
> --
> 2.34.1
>
On Tue, Sep 19, 2023 at 09:17:27AM -0400, Brian Foster wrote:
> On Sat, Sep 16, 2023 at 09:30:19AM +0200, Christophe JAILLET wrote:
> > snprintf() and scnprintf() are the same, except for the returned value.
> > When this value is not used, it is more logical to use snprintf() which is
> > slightly simpler.
> >
> > Signed-off-by: Christophe JAILLET <christophe.jaillet@wanadoo.fr>
> > ---
>
> Seems reasonable:
>
> Reviewed-by: Brian Foster <bfoster@redhat.com>
No, let's stay with scnprintf as the default - snprintf should be
deprecated except for when its return value is actually needed, using it
incorrectly has been a source of buffer overruns in the past.
Le 19/09/2023 à 21:02, Kent Overstreet a écrit :
> On Tue, Sep 19, 2023 at 09:17:27AM -0400, Brian Foster wrote:
>> On Sat, Sep 16, 2023 at 09:30:19AM +0200, Christophe JAILLET wrote:
>>> snprintf() and scnprintf() are the same, except for the returned value.
>>> When this value is not used, it is more logical to use snprintf() which is
>>> slightly simpler.
>>>
>>> Signed-off-by: Christophe JAILLET <christophe.jaillet@wanadoo.fr>
>>> ---
>>
>> Seems reasonable:
>>
>> Reviewed-by: Brian Foster <bfoster@redhat.com>
>
> No, let's stay with scnprintf as the default - snprintf should be
> deprecated except for when its return value is actually needed, using it
> incorrectly has been a source of buffer overruns in the past.
>
Ok, I was not aware of it.
In this case, there are also some s/snprintf/scnprintf/ opportunities in
fs/bcachefs
Does it make sense to update them or is it too low value changes?
CJ
On Tue, Sep 19, 2023 at 09:23:47PM +0200, Christophe JAILLET wrote:
> Le 19/09/2023 à 21:02, Kent Overstreet a écrit :
> > On Tue, Sep 19, 2023 at 09:17:27AM -0400, Brian Foster wrote:
> > > On Sat, Sep 16, 2023 at 09:30:19AM +0200, Christophe JAILLET wrote:
> > > > snprintf() and scnprintf() are the same, except for the returned value.
> > > > When this value is not used, it is more logical to use snprintf() which is
> > > > slightly simpler.
> > > >
> > > > Signed-off-by: Christophe JAILLET <christophe.jaillet@wanadoo.fr>
> > > > ---
> > >
> > > Seems reasonable:
> > >
> > > Reviewed-by: Brian Foster <bfoster@redhat.com>
> >
> > No, let's stay with scnprintf as the default - snprintf should be
> > deprecated except for when its return value is actually needed, using it
> > incorrectly has been a source of buffer overruns in the past.
> >
>
> Ok, I was not aware of it.
>
> In this case, there are also some s/snprintf/scnprintf/ opportunities in
> fs/bcachefs
>
> Does it make sense to update them or is it too low value changes?
Not terribly important - long term, I want to depracate both snprintf
and scnprintf and convert everything to printbufs.
@@ -1180,7 +1180,7 @@ static void bch2_dev_attach(struct bch_fs *c, struct bch_dev *ca,
{
ca->dev_idx = dev_idx;
__set_bit(ca->dev_idx, ca->self.d);
- scnprintf(ca->name, sizeof(ca->name), "dev-%u", dev_idx);
+ snprintf(ca->name, sizeof(ca->name), "dev-%u", dev_idx);
ca->fs = c;
rcu_assign_pointer(c->devs[ca->dev_idx], ca);
@@ -926,7 +926,7 @@ int bch2_btree_perf_test(struct bch_fs *c, const char *testname,
time = j.finish - j.start;
- scnprintf(name_buf, sizeof(name_buf), "%s:", testname);
+ snprintf(name_buf, sizeof(name_buf), "%s:", testname);
prt_human_readable_u64(&nr_buf, nr);
prt_human_readable_u64(&per_sec_buf, div64_u64(nr * NSEC_PER_SEC, time));
printk(KERN_INFO "%-12s %s with %u threads in %5llu sec, %5llu nsec per iter, %5s per sec\n",
@@ -450,7 +450,7 @@ TRACE_EVENT(btree_path_relock_fail,
c = six_lock_counts(&path->l[level].b->c.lock);
__entry->read_count = c.n[SIX_LOCK_read];
__entry->intent_count = c.n[SIX_LOCK_intent];
- scnprintf(__entry->node, sizeof(__entry->node), "%px", b);
+ snprintf(__entry->node, sizeof(__entry->node), "%px", b);
}
__entry->iter_lock_seq = path->l[level].lock_seq;
__entry->node_lock_seq = is_btree_node(path, level)