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

[Andy Shevchenko]

On Fri, Apr 03, 2020 at 01:19:26PM +0200, Mauro Carvalho Chehab wrote:
> Em Fri, 3 Apr 2020 13:47:02 +0300
> Sakari Ailus <sakari.ailus@xxxxxxxxxxxxxxx> escreveu:
> 
> > > > +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 };
> > > > +
> > > > +	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);
> > > > +
> > > > +	if (*fourcc & BIT(31))
> > > > +		strscpy(s + sizeof(*fourcc), FOURCC_STRING_BE,
> > > > +			sizeof(FOURCC_STRING_BE));
> > > > +
> > > > +	return string(buf, end, s, spec);  
> > > 
> > > Taking V4L2_PIX_FMT_Y16_BE as an example, this will print 'Y16 -BE'
> > > (without quotes). There are other 4CCs that contain spaces and would
> > > suffer from a similar issue. Even in little-endian format, it would
> > > result in additional spaces in the output string. Is this what we want ?
> > > Should the caller always enclose the 4CC in quotes or brackets for
> > > clarity ? Or should still be done here ?  
> > 
> > Good question. Space is indeed a valid character in a 4cc code.
> > 
> > If I omit one or more spaces, it will no longer be a 4cc, but a 3cc or even
> > a 2cc. Jokes aside, there are probably fair arguments both ways.
> > 
> > I presume there's no 4cc code where the location of a space would make a
> > difference but all of the spaces are trailing spaces.
> 
> Yes. I guess it doesn't make any sense to allow a 4cc code with an
> space before or in the middle.
> 
> Btw, on a quick search at the Internet for non-Linux definitions,
> a Fourcc code "Y8  " is actually shown at the lists as just "Y8", 
> e. g. removing the leading spaces:
> 
> 	https://www.fourcc.org/codecs.php
> 	http://abcavi.kibi.ru/fourcc.php
> 	https://softron.zendesk.com/hc/en-us/articles/207695697-List-of-FourCC-codes-for-video-codecs
> 	https://www.free-codecs.com/guides/guides.php?f=fourcc
> 
> One interesting detail there is that some tables show some codes 
> like "BGR(15)". While I'm not sure how this is encoded, I suspect
> that the fourcc is actually "BGR\x15".
> 
> We don't do that on V4L, nor we have plans to do so. Not sure if
> DRM would accept something like that. Of so, then the logic should
> have some special handler if the code is below 32.	

It is easy to achieve I think, with help of string_escape*() functions.

> > It's also worth noting that the formats printed are mostly for debugging
> > purpose and thus even getting a hypothetical case wrong is not a grave
> > issue. This would also support just printing them as-is though.
> > 
> > I'm leaning slightly towards omitting any spaces if the code has them. 
> 
> I would just remove trailing spaces, and then use a loop from the end
> to remove trailing spaces (and optionally handle codes ending with a
> value below 32, if are there any such case with DRM fourcc codes).
> 
> On the other hand, I don't mind if you prefer to use just one for()
> loop and just trip any spaces inside it.
> 
> > This is something that couldn't be done by using a macro...
> 
> Well, I suspect that it might be possible to write a macro
> for doing that too, for example using preprocessor concatenation
> logic that could produce the same results. If you do something 
> like that, however, I suspect that te macro would face some 
> portability issues, as, as far as I know, not all C compilers
> would handle string concatenation the same way.
> 
> Thanks,
> Mauro

-- 
With Best Regards,
Andy Shevchenko