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.
Rasmus
--
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
