Re: [PATCH v2 1/1] lib/vsprintf: Add support for printing V4L2 and DRM fourccs

[Andy Shevchenko <andriy.shevchenko@linux.intel.com>]

On Fri, Apr 03, 2020 at 12:39:16PM +0300, Sakari Ailus wrote:
> On Fri, Apr 03, 2020 at 12:31:03PM +0300, Andy Shevchenko wrote:
> > On Fri, Apr 03, 2020 at 12:11:56PM +0300, Sakari Ailus wrote:
> > > Add a printk modifier %ppf (for pixel format) for printing V4L2 and DRM
> > > pixel formats denoted by 4ccs. The 4cc 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)
> > > +{
> > 
> > > +#define FOURCC_STRING_BE	"-BE"
> > > +	char s[sizeof(*fourcc) + sizeof(FOURCC_STRING_BE)] = { 0 };
> > 
> > I guess it makes it too complicated.
> 
> The above also clearly binds the size to the data that is expected to
> contain there. I'd prefer keeping it as-is. And yes, 8 would be correct,
> too.

OK.

> > 	char s[8];
> > 
> > > +	if (check_pointer(&buf, end, fourcc, spec))
> > > +		return buf;
> > > +
> > > +	if (fmt[1] != 'c' || fmt[2] != 'c')
> > > +		return error_string(buf, end, "(%p4?)", spec);
> > > +
> > 
> > > +	put_unaligned_le32(*fourcc & ~BIT(31), s);
> > 
> > Can you elaborate what the difference in output with this bit set over cleared?
> > I.o.w. why don't we need to put it as BE and for LE case addd "-LE"?
> 
> The established practice is that big endian formats have "-BE" suffix
> whereas the little endian ones have nothing. (At least when it comes to
> V4L2.)

What I meant by the first part of the question is ordering of the characters.
That ordering of characters is not related to that flag, correct? So, bit
actually defines the endianess of the data in the certain fourcc.

Probably you need to put a comment to explain this.

> > > +	if (*fourcc & BIT(31))
> > > +		strscpy(s + sizeof(*fourcc), FOURCC_STRING_BE,
> > > +			sizeof(FOURCC_STRING_BE));
> > 
> > We know the size, and we may put '\0' as well
> > 	if (*fourcc & BIT(31))
> > 		strscpy(&s[4], "-BE", sizeof("-BE"));
> > 	else
> > 		strscpy(&s[4], "", sizeof(""));
> 
> The rest of the struct memory has already been set to zero in variable
> declaration.

Which is bogus in my opinion. strscpy() or direct '\0' termination will put it
more explicit.

-- 
With Best Regards,
Andy Shevchenko