Re: [PATCH] videodev2.h: add helper to validate colorspace

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



Hi Laurent and Niklas,

On Wed, Feb 14, 2018 at 12:23:05AM +0200, Laurent Pinchart wrote:
> Hi Niklas,
> 
> Thank you for the patch.
> 
> On Wednesday, 14 February 2018 00:08:47 EET Niklas Söderlund wrote:
> > There is no way for drivers to validate a colorspace value, which could
> > be provided by user-space by VIDIOC_S_FMT for example. Add a helper to
> > validate that the colorspace value is part of enum v4l2_colorspace.
> > 
> > Signed-off-by: Niklas Söderlund <niklas.soderlund+renesas@xxxxxxxxxxxx>
> > ---
> >  include/uapi/linux/videodev2.h | 5 +++++
> >  1 file changed, 5 insertions(+)
> > 
> > Hi,
> > 
> > I hope this is the correct header to add this helper to. I think it's
> > since if it's in uapi not only can v4l2 drivers use it but tools like
> > v4l-compliance gets access to it and can be updated to use this instead
> > of the hard-coded check of just < 0xff as it was last time I checked.
> > 
> > // Niklas
> > 
> > diff --git a/include/uapi/linux/videodev2.h b/include/uapi/linux/videodev2.h
> > index 9827189651801e12..843afd7c5b000553 100644
> > --- a/include/uapi/linux/videodev2.h
> > +++ b/include/uapi/linux/videodev2.h
> > @@ -238,6 +238,11 @@ enum v4l2_colorspace {
> >  	V4L2_COLORSPACE_DCI_P3        = 12,
> >  };
> > 
> > +/* Determine if a colorspace is defined in enum v4l2_colorspace */
> > +#define V4L2_COLORSPACE_IS_VALID(colorspace)		\
> > +	(((colorspace) >= V4L2_COLORSPACE_DEFAULT) &&	\
> > +	 ((colorspace) <= V4L2_COLORSPACE_DCI_P3))
> > +
> 
> This looks pretty good to me. I'd remove the parentheses around each test 
> though.

Agreed.

> 
> One potential issue is that if this macro operates on an unsigned value (for 
> instance an u32, which is the type used for the colorspace field in various 
> structures) the compiler will generate a warning:
> 
> enum.c: In function ‘test_4’:                                                                                                                                                                             
> enum.c:30:16: warning: comparison of unsigned expression >= 0 is always true 
> [-Wtype-limits]                                                                                                              
>   return V4L2_COLORSPACE_IS_VALID(colorspace);
> 
> Dropping the first check would fix that, but wouldn't catch invalid values 
> when operating on a signed type, such as int or enum v4l2_colorspace.

How about simply casting it to u32 first (and removing the first test)?

-- 
Regards,

Sakari Ailus
e-mail: sakari.ailus@xxxxxx



[Index of Archives]     [Linux Samsung SOC]     [Linux Wireless]     [Linux Kernel]     [ATH6KL]     [Linux Bluetooth]     [Linux Netdev]     [Kernel Newbies]     [IDE]     [Security]     [Git]     [Netfilter]     [Bugtraq]     [Yosemite News]     [MIPS Linux]     [ARM Linux]     [Linux Security]     [Linux RAID]     [Linux ATA RAID]     [Samba]     [Device Mapper]

  Powered by Linux