| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <1451401664.30729.368.camel@linux.intel.com>
Date: Tue, 29 Dec 2015 17:07:44 +0200
From: Andy Shevchenko <andriy.shevchenko@...ux.intel.com>
To: Andy Shevchenko <andy.shevchenko@...il.com>,
Rasmus Villemoes <linux@...musvillemoes.dk>
Cc: Joe Perches <joe@...ches.com>,
Andrew Morton <akpm@...ux-foundation.org>,
"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH v1 1/1] lib/vsprintf: refactor duplicate code to
xnumber()
On Tue, 2015-12-29 at 00:20 +0200, Andy Shevchenko wrote:
> On Mon, Dec 28, 2015 at 11:42 PM, Rasmus Villemoes
> <linux@...musvillemoes.dk> wrote:
> > On Mon, Dec 28 2015, Joe Perches <joe@...ches.com> wrote:
> >
> > > On Mon, 2015-12-28 at 20:18 +0200, Andy Shevchenko wrote:
> > > > xnumber() is a special helper to print a fixed size type in a
> > > > hex format with
> > > > '0x' prefix with padding and reduced size. In the module we
> > > > have already
> > > > several copies of such code. Consolidate them under xnumber()
> > > > helper.
> > > >
> > > > There are couple of differences though.
> > > >
> > > > It seems nobody cared about the output in case of
> > > > CONFIG_KALLSYMS=n when
> > > > printing symbol address because the asked width is not enough
> > > > to care either
> > > > prefix or last byte. Fixed here.
> >
> > ok, though I'm curious what 'last byte' refers to here?
>
> The last byte ('78') as it appears in the string carrying the number
> '0x12345678'. Yeah, might be confusing, I'm open for suggestion how
> to
> phrase it.
>
> >
> > > > The %pNF specifier used to be allowed with a specific field
> > > > width, though there
> > > > is neither any user of it nor mention in the documentation.
> > > >
> > > > Signed-off-by: Andy Shevchenko <andriy.shevchenko@...ux.intel.c
> > > > om>
> > > > ---
> > > > lib/vsprintf.c | 43 +++++++++++++++---------------------------
> > > > -
> > > > 1 file changed, 15 insertions(+), 28 deletions(-)
> > > >
> > > > diff --git a/lib/vsprintf.c b/lib/vsprintf.c
> > > > index dcf5646..e971549 100644
> > > > --- a/lib/vsprintf.c
> > > > +++ b/lib/vsprintf.c
> > > > @@ -514,6 +514,16 @@ char *number(char *buf, char *end,
> > > > unsigned long long num,
> > > > return buf;
> > > > }
> > > >
> > > > +static noinline_for_stack
> > > > +char *xnumber(char *buf, char *end, unsigned long long value,
> > > > unsigned int type,
> > > > + struct printf_spec spec)
> >
> > Is there any aspect of the passed-through printf_spec which isn't
> > overridden in xnumber? The users are/will be various %p extensions,
> > which probably means that no-one passes a non-default precision
> > (gcc
> > complains about %.*p), and the remaining possible flags (PLUS,
> > LEFT,
> > SPACE) are useless and/or impossible to pass to %p without gcc
> > complaining. In other words, why pass the spec at all instead of
> > just
> > building it inside xnumber?
>
> Wow, good catch!
> I slightly suspected something like that, but didn't made up my mind
> to check this.
>
> >
> > > xnumber isn't a great name.
> >
> > Maybe 'hexnumber'.
>
> We already have similar for %*ph. And as you noticed below…
>
> > That's a bit further away from 'number', and 'x'
> > might stand for something other than hex.
>
> …isn't only about hex. I don't know how to play on words the all
> three
> flags including 16 base.
>
> >
> > > unsigned int type should probably be size_t size
> >
> > Compromise: 'unsigned int size'. The name should be size since it's
> > supposed to be the size of the actual type being printed. But the
> > type
> > carrying that information need not be 8 bytes wide on 64bits.
>
> Exactly, the result anyway as for now only 8 bits as a part of
> unsigned int.
Oops, 24 bits of signed int. Incorrectly caught wrong line.
So, I will change this to be int size then.
>
> >
> > > > static noinline_for_stack
> > > > char *address_val(char *buf, char *end, const void *addr,
> > > > struct printf_spec spec, const char *fmt)
> > > > {
> > > > - unsigned long long num;
> > > > -
> > > > - spec.flags |= SPECIAL | SMALL | ZEROPAD;
> > > > - spec.base = 16;
> > > > -
> > > > switch (fmt[1]) {
> > > > case 'd':
> > > > - num = *(const dma_addr_t *)addr;
> > > > - spec.field_width = sizeof(dma_addr_t) * 2 + 2;
> > > > - break;
> > > > + return xnumber(buf, end, *(const dma_addr_t
> > > > *)addr, sizeof(dma_addr_t), spec);
> > > > case 'p':
> > > > default:
> > > > - num = *(const phys_addr_t *)addr;
> > > > - spec.field_width = sizeof(phys_addr_t) * 2 + 2;
> > > > - break;
> > > > + return xnumber(buf, end, *(const phys_addr_t
> > > > *)addr, sizeof(phys_addr_t), spec);
> > > > }
> > > > -
> > > > - return number(buf, end, num, spec);
> > > > }
> >
> > Nit: I think it would be a bit easier to read if the
> > cast+dereference
> > are kept outside the function calls. I'd suggest just introducing
> > 'unsigned int size', assign the appropriate value in the two cases,
> > and
> > fall through to a common 'xnumber(buf, end, num, size);'. It'll
> > even
> > line up nicely ;-)
>
> Will try that.
>
> >
> > num = *(const dma_addr_t *)addr;
> > size = sizeof(dma_addr_t);
>
> Thanks, Rasmus, for review.
>
--
Andy Shevchenko <andriy.shevchenko@...ux.intel.com>
Intel Finland Oy
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Powered by blists - more mailing lists