linux-kernel - Re: [PATCH v4 1/1] lib/vsprintf: Add support for printing V4L2 and DRM fourccs

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

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20201103145616.GJ26150@paasikivi.fi.intel.com>
Date:   Tue, 3 Nov 2020 16:56:16 +0200
From:   Sakari Ailus <sakari.ailus@...ux.intel.com>
To:     Andy Shevchenko <andriy.shevchenko@...ux.intel.com>
Cc:     linux-kernel@...r.kernel.org, linux-media@...r.kernel.org,
        Petr Mladek <pmladek@...e.com>,
        Dave Stevenson <dave.stevenson@...pberrypi.com>,
        dri-devel@...ts.freedesktop.org, hverkuil@...all.nl,
        laurent.pinchart@...asonboard.com, mchehab@...nel.org,
        Sergey Senozhatsky <sergey.senozhatsky@...il.com>,
        Steven Rostedt <rostedt@...dmis.org>,
        Joe Perches <joe@...ches.com>,
        Jani Nikula <jani.nikula@...ux.intel.com>,
        Rasmus Villemoes <linux@...musvillemoes.dk>
Subject: Re: [PATCH v4 1/1] lib/vsprintf: Add support for printing V4L2 and
 DRM fourccs

Hi Andy,

On Tue, Nov 03, 2020 at 04:47:47PM +0200, Andy Shevchenko wrote:
> On Tue, Nov 03, 2020 at 03:34:00PM +0200, Sakari Ailus wrote:
> > Add a printk modifier %p4cc (for pixel format) for printing V4L2 and DRM
> > pixel formats denoted by fourccs. The fourcc encoding is the same for both
> > so the same implementation can be used.
> 
> ...
> 
> > +static noinline_for_stack
> > +char *fourcc_string(char *buf, char *end, const u32 *fourcc,
> > +		    struct printf_spec spec, const char *fmt)
> > +{
> > +	char output[sizeof("(xx)(xx)(xx)(xx) little-endian (0x01234567)")];
> 
> I would add a comment that there is another possibility, i.e. big-endian, but
> it occupies less space.

This string is here to represent the longest possible output of the
function. Most of the time it's less. Saying that might make sense but it's
fairly clear already. Either way works for me though.

> 
> > +	char *p = output;
> > +	unsigned int i;
> > +	u32 val;
> > +
> > +	if (fmt[1] != 'c' || fmt[2] != 'c')
> > +		return error_string(output, end, "(%p4?)", spec);
> > +
> > +	if (check_pointer(&buf, end, fourcc, spec))
> > +		return buf;
> > +
> > +	val = *fourcc & ~BIT(31);
> > +
> > +	for (i = 0; i < sizeof(*fourcc); i++) {
> > +		unsigned char c = val >> (i * 8);
> > +
> > +		/* Weed out spaces */
> > +		if (c == ' ')
> > +			continue;
> > +
> > +		/* Print non-control ASCII characters as-is */
> > +		if (isascii(c) && isprint(c)) {
> > +			*p++ = c;
> > +			continue;
> > +		}
> > +
> > +		*p++ = '(';
> > +		p = hex_byte_pack(p, c);
> > +		*p++ = ')';
> > +	}
> 
> I guess above loop can be still optimized, but I have to think about it.
> 
> > +	strcpy(p, *fourcc & BIT(31) ? " big-endian" : " little-endian");
> > +	p += strlen(p);
> > +
> > +	*p++ = ' ';
> > +	*p++ = '(';
> > +	/* subtract parenthesis and the space from the size */
> 
> This comment misleading. What you are subtracting is a room for closing
> parenthesis and NUL.

Agreed, I'll update it for v5.

> 
> > +	p = special_hex_number(p, output + sizeof(output) - 2, *fourcc,
> > +			       sizeof(u32));
> 
> I would go with one line here.

It's wrapped since the result would be over 80 otherwise.

> 
> The (theoretical) problem is here that the case when buffer size is not enough
> to print a value will be like '(0xabc)' but should be rather '(0xabcd' like
> snprintf() does in general.
> 
> > +	*p++ = ')';
> > +	*p = '\0';
> > +
> > +	return string(buf, end, output, spec);
> > +}
> 

-- 
Regards,

Sakari Ailus