Re: [PATCH v3.1 1/2] v4l: subdev: tolerate null in media_entity_to_v4l2_subdev

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

 



Hi Laurent,

On Thu, May 18, 2017 at 07:08:00PM +0300, Laurent Pinchart wrote:
> Hi Sakari,
> 
> On Wednesday 17 May 2017 22:20:57 Sakari Ailus wrote:
> > On Wed, May 17, 2017 at 04:38:14PM +0100, Kieran Bingham wrote:
> > > From: Kieran Bingham <kieran.bingham+renesas@xxxxxxxxxxxxxxxx>
> > > 
> > > Return NULL, if a null entity is parsed for it's v4l2_subdev
> > > 
> > > Signed-off-by: Kieran Bingham <kieran.bingham+renesas@xxxxxxxxxxxxxxxx>
> > > ---
> > > 
> > >  include/media/v4l2-subdev.h | 2 +-
> > >  1 file changed, 1 insertion(+), 1 deletion(-)
> > > 
> > > diff --git a/include/media/v4l2-subdev.h b/include/media/v4l2-subdev.h
> > > index 5f1669c45642..72d7f28f38dc 100644
> > > --- a/include/media/v4l2-subdev.h
> > > +++ b/include/media/v4l2-subdev.h
> > > @@ -829,7 +829,7 @@ struct v4l2_subdev {
> > > 
> > >  };
> > >  
> > >  #define media_entity_to_v4l2_subdev(ent) \
> > > -	container_of(ent, struct v4l2_subdev, entity)
> > > +	(ent ? container_of(ent, struct v4l2_subdev, entity) : NULL)
> > > 
> > >  #define vdev_to_v4l2_subdev(vdev) \
> > >  	((struct v4l2_subdev *)video_get_drvdata(vdev))
> > 
> > The problem with this is that ent is now referenced twice. If the ent macro
> > argument has side effect, this would introduce bugs. It's unlikely, but
> > worth avoiding. Either use a macro or a function.
> > 
> > I think I'd use function for there's little use for supporting for const and
> > non-const arguments presumably. A simple static inline function should do.
> 
> Note that, if we want to keep using a macro, this could be written as
> 
> #define media_entity_to_v4l2_subdev(ent) ({ \
> 	typeof(ent) __ent = ent; \
> 	__ent ? container_of(__ent, struct v4l2_subdev, entity) : NULL; \
> })
> 
> Bonus point if you can come up with a way to return a const struct v4l2_subdev 
> pointer when then ent argument is const.

I can't think of a use case for that. I've never seen a const struct
v4l2_subdev anywhere. I could be just oblivious though. :-)

Better give a __ent a name that someone will not accidentally come up with.
That can lead to problems that are difficult to debug --- for the code
compiles, it just doesn't do what's expected.

-- 
Regards,

Sakari Ailus
e-mail: sakari.ailus@xxxxxx	XMPP: sailus@xxxxxxxxxxxxxx



[Index of Archives]     [Linux Input]     [Video for Linux]     [Gstreamer Embedded]     [Mplayer Users]     [Linux USB Devel]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [Yosemite Backpacking]
  Powered by Linux