| Message ID | 20231003120813.77726-6-sakari.ailus@linux.intel.com |
|---|---|
| State | New |
| Headers | show |
| Series | Generic line based metadata support, internal pads | expand |
On 03/10/2023 14:08, Sakari Ailus wrote: > As the user needs to know what went wrong for S_ROUTING, copy array > arguments back to the user. > > Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com> > --- > drivers/media/v4l2-core/v4l2-ioctl.c | 3 ++- > 1 file changed, 2 insertions(+), 1 deletion(-) > > diff --git a/drivers/media/v4l2-core/v4l2-ioctl.c b/drivers/media/v4l2-core/v4l2-ioctl.c > index fb453b7d0c91..6921a72566df 100644 > --- a/drivers/media/v4l2-core/v4l2-ioctl.c > +++ b/drivers/media/v4l2-core/v4l2-ioctl.c > @@ -3419,7 +3419,8 @@ video_usercopy(struct file *file, unsigned int orig_cmd, unsigned long arg, > * in case of failure, but it is not defined here as part of the > * 'v4l2_ioctls' array, insert an ad-hoc check to address that. > */ > - if (err < 0 && !always_copy && cmd != VIDIOC_SUBDEV_G_ROUTING) > + if (err < 0 && !always_copy && cmd != VIDIOC_SUBDEV_G_ROUTING && > + cmd != VIDIOC_SUBDEV_S_ROUTING) > goto out; > > if (has_array_args) { This is ugly. How about this: if (cmd == VIDIOC_SUBDEV_G_ROUTING || cmd == VIDIOC_SUBDEV_S_ROUTING) always_copy = true; if (err < 0 && !always_copy) goto out; The first 'if' can also be a 'switch', I have no preference. This cmd check can also be done earlier in the function, right after the call to video_get_user(). It might be a better place. Regards, Hans
Hi Hans, On Thu, Oct 05, 2023 at 12:37:41PM +0200, Hans Verkuil wrote: > On 03/10/2023 14:08, Sakari Ailus wrote: > > As the user needs to know what went wrong for S_ROUTING, copy array > > arguments back to the user. > > > > Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com> > > --- > > drivers/media/v4l2-core/v4l2-ioctl.c | 3 ++- > > 1 file changed, 2 insertions(+), 1 deletion(-) > > > > diff --git a/drivers/media/v4l2-core/v4l2-ioctl.c b/drivers/media/v4l2-core/v4l2-ioctl.c > > index fb453b7d0c91..6921a72566df 100644 > > --- a/drivers/media/v4l2-core/v4l2-ioctl.c > > +++ b/drivers/media/v4l2-core/v4l2-ioctl.c > > @@ -3419,7 +3419,8 @@ video_usercopy(struct file *file, unsigned int orig_cmd, unsigned long arg, > > * in case of failure, but it is not defined here as part of the > > * 'v4l2_ioctls' array, insert an ad-hoc check to address that. > > */ > > - if (err < 0 && !always_copy && cmd != VIDIOC_SUBDEV_G_ROUTING) > > + if (err < 0 && !always_copy && cmd != VIDIOC_SUBDEV_G_ROUTING && > > + cmd != VIDIOC_SUBDEV_S_ROUTING) > > goto out; > > > > if (has_array_args) { > > This is ugly. > > How about this: > > if (cmd == VIDIOC_SUBDEV_G_ROUTING || cmd == VIDIOC_SUBDEV_S_ROUTING) > always_copy = true; > > if (err < 0 && !always_copy) > goto out; > > The first 'if' can also be a 'switch', I have no preference. > > This cmd check can also be done earlier in the function, right after > the call to video_get_user(). It might be a better place. [GS]_ROUTING are already handled here (plus the related comment is right above) so I prefer this location, i.e. exact lines above you suggested. It's indeed cleaner.
diff --git a/drivers/media/v4l2-core/v4l2-ioctl.c b/drivers/media/v4l2-core/v4l2-ioctl.c index fb453b7d0c91..6921a72566df 100644 --- a/drivers/media/v4l2-core/v4l2-ioctl.c +++ b/drivers/media/v4l2-core/v4l2-ioctl.c @@ -3419,7 +3419,8 @@ video_usercopy(struct file *file, unsigned int orig_cmd, unsigned long arg, * in case of failure, but it is not defined here as part of the * 'v4l2_ioctls' array, insert an ad-hoc check to address that. */ - if (err < 0 && !always_copy && cmd != VIDIOC_SUBDEV_G_ROUTING) + if (err < 0 && !always_copy && cmd != VIDIOC_SUBDEV_G_ROUTING && + cmd != VIDIOC_SUBDEV_S_ROUTING) goto out; if (has_array_args) {
As the user needs to know what went wrong for S_ROUTING, copy array arguments back to the user. Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com> --- drivers/media/v4l2-core/v4l2-ioctl.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-)