On Wed 11-04-18 12:49:25, Rasmus Villemoes wrote:
> On 2018-04-11 12:01, Jan Kara wrote:
> > On Tue 10-04-18 19:56:14, Rasmus Villemoes wrote:
> >> Here's an idea that does require rewriting all the existing
> >> reiserfs_warning etc. calls, but avoids all the fragile vararg handling
> >> and adds proper compile-time format string checking:
> >>
> >> Have reiserfs_warning (and friends) be a macro that expands to
> >>
> >> reiserfs_printf_start();
> >> __reiserfs_warning(the arguments);
> >> reiserfs_printf_stop();
> >>
> >> where printf_start would take a spinlock and reset some bookkeeping
> >> variables. Then all the sprintf_de_head etc. helpers would have a
> >> prototype like
> >>
> >> const char *sprintf_de_head(struct reiserfs_de_head *deh)
> >>
> >> and they would print their output to the current error_buf, updating the
> >> above-mentioned bookkeeping variable(s) and return a pointer to the
> >> start of the area they printed to. So
> >>
> >> reiserfs_warning("bla %a", deh)
> >>
> >> would get changed to
> >>
> >> reiserfs_warning("bla %s", sprintf_de_head(deh))
> >>
> >> Basically, the current error_buf would only be used for holding the
> >> result of the custom specifiers, everything else goes directly to
> >> printk's output buffer, so no need to make error_buf larger (a single
> >> printk is limited to 1024 bytes anyway...), and fmt_buf goes away
> >> completely. That reduction in static footprint should make up for at
> >> least some of the bloat caused by the extra code the new
> >> reiserfs_warning would generate. Also, I don't think this should make us
> >> hold the spinlock (much) longer than is currently the case, and
> >> trivially solves the problem of not actually holding the spinlock when
> >> passing the error_buf to printk.
> >
> > So I don't see how this would work for more complex warnings like:
> >
> > reiserfs_warning("bla %*d foo %a bar %llx %a", width, number, deh,
> > another_number, deh2)
> >
>
> all the %a and other current custom specifiers would be rewritten as %s,
> and the corresponding arguments would be wrapped with an appropriate
> call to the custom formatter, so that example would be
>
> reiserfs_warning("bla %*d foo %s bar %llx %s", width, number,
> sprintf_de_head(deh), another_number, sprintf_de_head(deh2))
>
> And sprintf_de_head would be something like
>
> char *sprintf_de_head(struct reiserfs_de_head *deh)
> {
> buf = cur_err_buf;
> len = snprintf(buf, compute-whats-left, "whatever", deh->this, deh->that);
> cur_err_buf += len+1;
> return buf;
> }
>
> except one would need to handle the case of overflowing err_buf a little
> better. In reiserfs_printf_start, we'd take the spinlock and set
> cur_err_buf = err_buf. reiserfs_printf_stop would simply unlock the
> spinlock. It doesn't matter what order gcc evaluates the
> reiserfs_warning arguments.
>
> Internally, __reiserfs_warning is mostly just a pr_warn. Well, maybe for
> the _warning case one wants to add another level of macro machinery to
> handle the __func__ and unique-id strings, but that's simple enough.
> Actually, perhaps __reiserfs_warning-the-function would go away
> completely and just be a direct pr_warn call.
>
> Yes, as I said, it does require rewriting all (well, those using custom
> specifiers) existing reiserfs_warning calls, but considering the
> diffstat of the current patch set, I don't think it's that bad. And we'd
> get proper type checking and format string checking, so there's not much
> risk. Sure, the rewriting may require some clever coccinelle script, but
> it's definitely doable. We don't even need to match the specifiers with
> the arguments; any struct reiserfs_de_head * that is passed to a
> reiserfs_warning should just be wrapped in sprintf_de_head(), and after
> that it's just s/%a/%s/. The __printf attribute will catch anything this
> misses.
OK, I understand now. So basically ripping out all the smarts (except for
buffer space handling) out of reiserfs_warning() and putting them into the
caller. That certainly works although it's quite some work - 175 warnings to
go through.
Honza
--
Jan Kara <jack@xxxxxxxx>
SUSE Labs, CR
--
To unsubscribe from this list: send the line "unsubscribe reiserfs-devel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
