On Tue 10-04-18 10:44:43, Jeff Mahoney wrote:
> On 4/10/18 10:25 AM, Jan Kara wrote:
> > On Mon 09-04-18 14:16:02, jeffm@xxxxxxxx wrote:
> >> From: Jeff Mahoney <jeffm@xxxxxxxx>
> >>
> >> reiserfs_snprintf operates by handling the reiserfs-specific specifiers
> >> itself and passing everything else to the generic vsnprintf routine.
> >> That makes some assumptions about what the state of a va_list passed
> >> by value to another function will be after the function returns. The
> >> spec states that the contents of the va_list are undefined in that
> >> case. It's worked so far but really only by the grace of gcc's internal
> >> va_list implementation details.
> >>
> >> This patch reworks reiserfs_snprintf to process one % directive at
> >> a time. Since directives can consume more than one argument when field
> >> width and precision * operators are used, that means we need to interpret
> >> the format string more than we used to do. (The kernel vsnprintf
> >> specifically doesn't handle %n). Otherwise, we can use an unsigned long
> >> to hold the argument and let the generic snprintf do the type handling.
> >> The only exception is long long arguments on platforms where long long
> >> is larger than long, which need special handling.
> >
> > So I wonder why is this correct for architectures where sizeof(int) !=
> > sizeof(long). If you do va_arg(va, long) for an argument which is actually
> > int, you'll consume more bytes from the stack than you should. Or am I
> > missing some va_arg() subtlety?
>
> It's possible I'm incorrect here. I'm no expert on how variadics are
> constructed across architectures. My underlying assumption is that even
> if the arguments are passed on the stack, they're not the same as stack
> variables. There's no ABI between caller and callee, so I expect that
> the stack slot would be the size of a register, which is long. So the
> int returned by va_arg would only take up e.g. 4 bytes on the stack but
> the space va_arg uses would take e.g. 8 bytes. Yes, it's fragile.
Well, another equally sensible implementation could be that the compiler
just arranges for storing all arguments on stack using their native size by
the caller (who knows all the types) and va_start + va_arg know how to
consume these arguments from the stack. And with such implementation you'd
need to make sure you pass a proper type to va_arg()...
> >> The result is a va_list that is consistent within reiserfs_snprintf
> >> and isn't passed by value anymore.
> >>
> >> This isn't an ideal solution, but I didn't feel that teaching pointer()
> >> about a %pV variant that didn't use va_copy would get much traction for
> >> a number of reasons.
> >
> > Frankly, I don't like the format string parsing duplication in your patch
> > and I'm afraid it will be a maintenance issue. Can't we have a variant
> > of vsnprintf() that would take a pointer to va_list (like you use in
> > your patch for some functions) so that its contents is well defined after
> > the function returns?
>
> That would certainly solve the problem, but I didn't go that route
> because the interface would end up being pretty fragile. For any case
> other than consuming a single (potentially compound) format unit, it'd
> be pretty easy to get out of sync.
How come? You'd pass the format string containing only non-reiserfs
specifiers and va_list * to new_vsnprintf(). It would consume as many args
as it should. Then you process next reiserfs specifier consuming some arg,
etc. I don't see what can get out of sync there...
> What I'd really like to see is an extended pointer() that allows the
> caller to provide a callback or similar. ReiserFS isn't alone in
> wanting to consistently print its internal structures. That would also
> eliminate the racy format and error buffer in reiserfs as well. I've
> been operating under the impression that this route would be a big
> uphill battle.
Well, yes, that would be a nicer solution but I don't expect that to go
easily either (if nothing else due to bikeshedding).
Honza
> >> Reported-by: Rasmus Villemoes <linux@xxxxxxxxxxxxxxxxxx>
> >> Signed-off-by: Jeff Mahoney <jeffm@xxxxxxxx>
> >> ---
> >> fs/reiserfs/prints.c | 158 +++++++++++++++++++++++++++++++++++++++++----------
> >> 1 file changed, 128 insertions(+), 30 deletions(-)
> >>
> >> diff --git a/fs/reiserfs/prints.c b/fs/reiserfs/prints.c
> >> index aead27d11e45..03ebdea9ddc7 100644
> >> --- a/fs/reiserfs/prints.c
> >> +++ b/fs/reiserfs/prints.c
> >> @@ -6,6 +6,7 @@
> >> #include <linux/fs.h>
> >> #include "reiserfs.h"
> >> #include <linux/string.h>
> >> +#include <linux/ctype.h>
> >> #include <linux/buffer_head.h>
> >>
> >> #include <stdarg.h>
> >> @@ -195,19 +196,94 @@ static size_t snprintf_disk_child(char *buf, size_t size, struct disk_child *dc)
> >> dc_block_number(dc), dc_size(dc));
> >> }
> >>
> >> -static char *is_there_reiserfs_struct(char *fmt, int *what)
> >> -{
> >> - char *k = fmt;
> >> -
> >> - while ((k = strchr(k, '%')) != NULL) {
> >> - if (k[1] == 'k' || k[1] == 'K' || k[1] == 'h' || k[1] == 't' ||
> >> - k[1] == 'z' || k[1] == 'b' || k[1] == 'y' || k[1] == 'a') {
> >> - *what = k[1];
> >> +/*
> >> + * This should all just be %pV but pointer() does a va_copy and uses that
> >> + * instead of directly using args. That's the right thing to do pretty
> >> + * much every time, but it still forces us to identify how many arguments
> >> + * we need to pass for a single format spec. We need to be careful of u64
> >> + * on 32-bit since it'll need special handling. We can just use an
> >> + * unsigned long for everything else and vsnprintf will handling the
> >> + * typing itself.
> >> + */
> >> +static int handle_generic_snprintf(char *p, size_t left,
> >> + const char *fmt, va_list *args)
> >> +{
> >> + int width, precision, nargs = 1;
> >> + const char *spec = fmt;
> >> + unsigned long val;
> >> + int len = 0;
> >> +
> >> + /* Skip flags */
> >> + while (1) {
> >> + bool found = true;
> >> + ++spec;
> >> + switch (*spec) {
> >> + case '-':
> >> + case '+':
> >> + case ' ':
> >> + case '#':
> >> + case '0':
> >> break;
> >> + default:
> >> + found = false;
> >> }
> >> - k++;
> >> + if (!found)
> >> + break;
> >> + }
> >> +
> >> + /* Field width */
> >> + if (isdigit(*spec)) {
> >> + while (isdigit(*++spec));
> >> + } else if (*spec == '*') {
> >> + nargs++;
> >> + spec++;
> >> + }
> >> +
> >> + /* Precision */
> >> + if (*spec == '.') {
> >> + spec++;
> >> + if (isdigit(*spec)) {
> >> + while (isdigit(*++spec));
> >> + } else if (*spec == '*') {
> >> + nargs++;
> >> + spec++;
> >> + }
> >> + }
> >> +
> >> + if (*spec == 'L' || !strncmp(spec, "ll", 2)) {
> >> + unsigned long long ullval;
> >> + if (nargs == 3) {
> >> + width = va_arg(*args, int);
> >> + precision = va_arg(*args, int);
> >> + ullval = va_arg(*args, unsigned long long);
> >> + len = snprintf(p, left, fmt, width, precision,
> >> + ullval);
> >> + } else if (nargs == 2) {
> >> + width = va_arg(*args, int);
> >> + ullval = va_arg(*args, unsigned long long);
> >> + len = snprintf(p, left, fmt, width, ullval);
> >> + } else {
> >> + ullval = va_arg(*args, unsigned long long);
> >> + len = snprintf(p, left, fmt, ullval);
> >> + }
> >> + return len;
> >> + }
> >> +
> >> + if (nargs == 3) {
> >> + width = va_arg(*args, int);
> >> + precision = va_arg(*args, int);
> >> + val = va_arg(*args, unsigned long);
> >> + len = snprintf(p, left, fmt, width, precision, val);
> >> + } else if (nargs == 2) {
> >> + width = va_arg(*args, int);
> >> + val = va_arg(*args, unsigned long);
> >> + len = snprintf(p, left, fmt, width, val);
> >> + } else {
> >> + val = va_arg(*args, unsigned long);
> >> + len = snprintf(p, left, fmt, val);
> >> }
> >> - return k;
> >> +
> >> + return len;
> >> }
> >>
> >> /*
> >> @@ -237,23 +313,34 @@ static char fmt_buf[1024];
> >> static int reiserfs_snprintf(char *p, size_t left, const char *fmt,
> >> va_list *args)
> >> {
> >> + size_t fmtlen = strlen(fmt);
> >> + const char *fmtend = fmt + fmtlen;
> >> + const char *end;
> >> char *start = p;
> >> - char *fmt1 = fmt_buf;
> >> - char *k;
> >> - int what;
> >> - size_t len;
> >>
> >> - spin_lock(&fmt_lock);
> >> - strncpy(fmt1, fmt, sizeof(fmt_buf));
> >> + while (left && fmt != NULL && *fmt) {
> >> + bool reiserfs_spec = true;
> >> + int fmt_size;
> >> + size_t len;
> >>
> >> - while ((k = is_there_reiserfs_struct(fmt1, &what)) != NULL) {
> >> - *k = 0;
> >> + end = strchr(fmt + 1, '%');
> >>
> >> - len = vsnprintf(p, left, fmt1, *args);
> >> - p += len;
> >> - left -= len;
> >> + /* Skip over %% */
> >> + while (end && !strncmp(end, "%%", 2))
> >> + end = strchr(end + 2, '%');
> >> +
> >> + if (!end)
> >> + end = fmtend;
> >>
> >> - switch (what) {
> >> + fmt_size = end - fmt;
> >> +
> >> + /* Any text before the first % - could be entire string */
> >> + if (fmt[0] != '%') {
> >> + len = snprintf(p, left, "%.*s", fmt_size, fmt);
> >> + goto next;
> >> + }
> >> +
> >> + switch (fmt[1]) {
> >> case 'k':
> >> len = snprintf_le_key(p, left,
> >> va_arg(*args, struct reiserfs_key *));
> >> @@ -286,19 +373,30 @@ static int reiserfs_snprintf(char *p, size_t left, const char *fmt,
> >> len = snprintf_de_head(p, left,
> >> va_arg(*args, struct reiserfs_de_head *));
> >> break;
> >> - }
> >> + default:
> >> + spin_lock(&fmt_lock);
> >> + strncpy(fmt_buf, fmt, fmt_size);
> >> + fmt_buf[fmt_size] = 0;
> >>
> >> + len = handle_generic_snprintf(p, left, fmt_buf, args);
> >> + spin_unlock(&fmt_lock);
> >> +
> >> + reiserfs_spec = false;
> >> + break;
> >> + };
> >> +
> >> + if (reiserfs_spec) {
> >> + p += len;
> >> + left -= len;
> >> +
> >> + len = snprintf(p, left, "%.*s", fmt_size - 2, fmt + 2);
> >> + }
> >> +next:
> >> p += len;
> >> left -= len;
> >> - fmt1 = k + 2;
> >> + fmt = end;
> >> }
> >>
> >> - len = vsnprintf(p, left, fmt1, *args);
> >> - p += len;
> >> - left -= len;
> >> -
> >> - spin_unlock(&fmt_lock);
> >> -
> >> return p - start;
> >> }
> >>
> >> --
> >> 2.12.3
> >>
>
>
> --
> Jeff Mahoney
> SUSE Labs
--
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
