* [v4l-utils PATCH 1/1] v4l: libv4l2subdev: Precisely convert media bus string to code
@ 2015-12-09 15:14 Sakari Ailus
2015-12-13 21:33 ` Laurent Pinchart
0 siblings, 1 reply; 6+ messages in thread
From: Sakari Ailus @ 2015-12-09 15:14 UTC (permalink / raw)
To: linux-media; +Cc: laurent.pinchart, hverkuil
The length of the string was ignored, making it possible for the
conversion to fail due to extra characters in the string.
Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
---
This patch should be applied before the set "[v4l-utils PATCH v2 0/3] List
supported formats in libv4l2subdev":
<URL:http://www.spinics.net/lists/linux-media/msg95377.html>
utils/media-ctl/libv4l2subdev.c | 10 ++++------
1 file changed, 4 insertions(+), 6 deletions(-)
diff --git a/utils/media-ctl/libv4l2subdev.c b/utils/media-ctl/libv4l2subdev.c
index 33c1ee6..cce527d 100644
--- a/utils/media-ctl/libv4l2subdev.c
+++ b/utils/media-ctl/libv4l2subdev.c
@@ -769,14 +769,12 @@ enum v4l2_mbus_pixelcode v4l2_subdev_string_to_pixelcode(const char *string,
unsigned int i;
for (i = 0; i < ARRAY_SIZE(mbus_formats); ++i) {
- if (strncmp(mbus_formats[i].name, string, length) == 0)
- break;
+ if (strncmp(mbus_formats[i].name, string, length) == 0
+ && strlen(mbus_formats[i].name) == length)
+ return mbus_formats[i].code;
}
- if (i == ARRAY_SIZE(mbus_formats))
- return (enum v4l2_mbus_pixelcode)-1;
-
- return mbus_formats[i].code;
+ return (enum v4l2_mbus_pixelcode)-1;
}
static struct {
--
2.1.0.231.g7484e3b
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [v4l-utils PATCH 1/1] v4l: libv4l2subdev: Precisely convert media bus string to code
2015-12-09 15:14 [v4l-utils PATCH 1/1] v4l: libv4l2subdev: Precisely convert media bus string to code Sakari Ailus
@ 2015-12-13 21:33 ` Laurent Pinchart
2016-01-25 11:39 ` Sakari Ailus
0 siblings, 1 reply; 6+ messages in thread
From: Laurent Pinchart @ 2015-12-13 21:33 UTC (permalink / raw)
To: Sakari Ailus; +Cc: linux-media, hverkuil
Hi Sakari,
Thank you for the patch.
On Wednesday 09 December 2015 17:14:47 Sakari Ailus wrote:
> The length of the string was ignored, making it possible for the
> conversion to fail due to extra characters in the string.
I'm not sure to follow you there. Is the issue that passing a string such as
"SBGGR10" would match "SBGGR10_DPCM8" if it was listed before "SBGGR10" ? If
that's the case I'd write the commit message as
Any character beyond the fist `length' characters in the mbus_formats strings
are ignored, causing incorrect matches if the format entry starts with but
isn't equal to the passed format.
> Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
> ---
> This patch should be applied before the set "[v4l-utils PATCH v2 0/3] List
> supported formats in libv4l2subdev":
>
> <URL:http://www.spinics.net/lists/linux-media/msg95377.html>
>
> utils/media-ctl/libv4l2subdev.c | 10 ++++------
> 1 file changed, 4 insertions(+), 6 deletions(-)
>
> diff --git a/utils/media-ctl/libv4l2subdev.c
> b/utils/media-ctl/libv4l2subdev.c index 33c1ee6..cce527d 100644
> --- a/utils/media-ctl/libv4l2subdev.c
> +++ b/utils/media-ctl/libv4l2subdev.c
> @@ -769,14 +769,12 @@ enum v4l2_mbus_pixelcode
> v4l2_subdev_string_to_pixelcode(const char *string, unsigned int i;
>
> for (i = 0; i < ARRAY_SIZE(mbus_formats); ++i) {
> - if (strncmp(mbus_formats[i].name, string, length) == 0)
> - break;
> + if (strncmp(mbus_formats[i].name, string, length) == 0
> + && strlen(mbus_formats[i].name) == length)
How about mbus_formats[i].name[length] == '\0' instead ? That should be more
efficient.
I also wonder whether we shouldn't just get rid of the length argument and
force the passed format string to be zero-terminated.
> + return mbus_formats[i].code;
> }
>
> - if (i == ARRAY_SIZE(mbus_formats))
> - return (enum v4l2_mbus_pixelcode)-1;
> -
> - return mbus_formats[i].code;
> + return (enum v4l2_mbus_pixelcode)-1;
> }
>
> static struct {
--
Regards,
Laurent Pinchart
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [v4l-utils PATCH 1/1] v4l: libv4l2subdev: Precisely convert media bus string to code
2015-12-13 21:33 ` Laurent Pinchart
@ 2016-01-25 11:39 ` Sakari Ailus
2016-01-25 19:41 ` Laurent Pinchart
0 siblings, 1 reply; 6+ messages in thread
From: Sakari Ailus @ 2016-01-25 11:39 UTC (permalink / raw)
To: Laurent Pinchart; +Cc: Sakari Ailus, linux-media, hverkuil
Hi Laurent,
On Sun, Dec 13, 2015 at 11:33:45PM +0200, Laurent Pinchart wrote:
> Hi Sakari,
>
> Thank you for the patch.
>
> On Wednesday 09 December 2015 17:14:47 Sakari Ailus wrote:
> > The length of the string was ignored, making it possible for the
> > conversion to fail due to extra characters in the string.
>
> I'm not sure to follow you there. Is the issue that passing a string such as
> "SBGGR10" would match "SBGGR10_DPCM8" if it was listed before "SBGGR10" ? If
> that's the case I'd write the commit message as
Yes, that's the problem.
>
> Any character beyond the fist `length' characters in the mbus_formats strings
> are ignored, causing incorrect matches if the format entry starts with but
> isn't equal to the passed format.
I'll use this commit message then.
>
> > Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
> > ---
> > This patch should be applied before the set "[v4l-utils PATCH v2 0/3] List
> > supported formats in libv4l2subdev":
> >
> > <URL:http://www.spinics.net/lists/linux-media/msg95377.html>
> >
> > utils/media-ctl/libv4l2subdev.c | 10 ++++------
> > 1 file changed, 4 insertions(+), 6 deletions(-)
> >
> > diff --git a/utils/media-ctl/libv4l2subdev.c
> > b/utils/media-ctl/libv4l2subdev.c index 33c1ee6..cce527d 100644
> > --- a/utils/media-ctl/libv4l2subdev.c
> > +++ b/utils/media-ctl/libv4l2subdev.c
> > @@ -769,14 +769,12 @@ enum v4l2_mbus_pixelcode
> > v4l2_subdev_string_to_pixelcode(const char *string, unsigned int i;
> >
> > for (i = 0; i < ARRAY_SIZE(mbus_formats); ++i) {
> > - if (strncmp(mbus_formats[i].name, string, length) == 0)
> > - break;
> > + if (strncmp(mbus_formats[i].name, string, length) == 0
> > + && strlen(mbus_formats[i].name) == length)
>
> How about mbus_formats[i].name[length] == '\0' instead ? That should be more
> efficient.
Fine for me.
>
> I also wonder whether we shouldn't just get rid of the length argument and
> force the passed format string to be zero-terminated.
I believe the reason is that the current user (media-ctl test program)
parses the user input and passes a portion of that to this function to
convert the string to a numeric value. That'd be a bit more cumbersome as
we'd either require copying the string elsewhere or changing the input by
the program. I wouldn't change the behaviour, at least not now.
>
> > + return mbus_formats[i].code;
> > }
> >
> > - if (i == ARRAY_SIZE(mbus_formats))
> > - return (enum v4l2_mbus_pixelcode)-1;
> > -
> > - return mbus_formats[i].code;
> > + return (enum v4l2_mbus_pixelcode)-1;
> > }
> >
> > static struct {
>
--
Kind regards,
Sakari Ailus
e-mail: sakari.ailus@iki.fi XMPP: sailus@retiisi.org.uk
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [v4l-utils PATCH 1/1] v4l: libv4l2subdev: Precisely convert media bus string to code
2016-01-25 11:39 ` Sakari Ailus
@ 2016-01-25 19:41 ` Laurent Pinchart
2016-01-25 20:47 ` Sakari Ailus
0 siblings, 1 reply; 6+ messages in thread
From: Laurent Pinchart @ 2016-01-25 19:41 UTC (permalink / raw)
To: Sakari Ailus; +Cc: Sakari Ailus, linux-media, hverkuil
Hi Sakari,
On Monday 25 January 2016 13:39:10 Sakari Ailus wrote:
> On Sun, Dec 13, 2015 at 11:33:45PM +0200, Laurent Pinchart wrote:
> > On Wednesday 09 December 2015 17:14:47 Sakari Ailus wrote:
> >> The length of the string was ignored, making it possible for the
> >> conversion to fail due to extra characters in the string.
> >
> > I'm not sure to follow you there. Is the issue that passing a string such
> > as "SBGGR10" would match "SBGGR10_DPCM8" if it was listed before
> > "SBGGR10" ? If that's the case I'd write the commit message as
>
> Yes, that's the problem.
>
> > Any character beyond the fist `length' characters in the mbus_formats
> > strings are ignored, causing incorrect matches if the format entry starts
> > with but isn't equal to the passed format.
>
> I'll use this commit message then.
>
> >> Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
> >> ---
> >> This patch should be applied before the set "[v4l-utils PATCH v2 0/3]
> >> List supported formats in libv4l2subdev":
> >>
> >> <URL:http://www.spinics.net/lists/linux-media/msg95377.html>
> >>
> >> utils/media-ctl/libv4l2subdev.c | 10 ++++------
> >> 1 file changed, 4 insertions(+), 6 deletions(-)
> >>
> >> diff --git a/utils/media-ctl/libv4l2subdev.c
> >> b/utils/media-ctl/libv4l2subdev.c index 33c1ee6..cce527d 100644
> >> --- a/utils/media-ctl/libv4l2subdev.c
> >> +++ b/utils/media-ctl/libv4l2subdev.c
> >> @@ -769,14 +769,12 @@ enum v4l2_mbus_pixelcode
> >> v4l2_subdev_string_to_pixelcode(const char *string,
> >> unsigned int i;
> >>
> >> for (i = 0; i < ARRAY_SIZE(mbus_formats); ++i) {
> >> - if (strncmp(mbus_formats[i].name, string, length) == 0)
> >> - break;
> >> + if (strncmp(mbus_formats[i].name, string, length) == 0
> >> + && strlen(mbus_formats[i].name) == length)
> >
> > How about mbus_formats[i].name[length] == '\0' instead ? That should be
> > more efficient.
>
> Fine for me.
>
> > I also wonder whether we shouldn't just get rid of the length argument and
> > force the passed format string to be zero-terminated.
>
> I believe the reason is that the current user (media-ctl test program)
> parses the user input and passes a portion of that to this function to
> convert the string to a numeric value. That'd be a bit more cumbersome as
> we'd either require copying the string elsewhere or changing the input by
> the program. I wouldn't change the behaviour, at least not now.
Yes that's the reason, and I think it's an API design mistake (or just a lack
of proper API design :-)). Wouldn't it be better to copy the string in the
caller ?
> >> + return mbus_formats[i].code;
> >> }
> >>
> >> - if (i == ARRAY_SIZE(mbus_formats))
> >> - return (enum v4l2_mbus_pixelcode)-1;
> >> -
> >> - return mbus_formats[i].code;
> >> + return (enum v4l2_mbus_pixelcode)-1;
> >> }
> >>
> >> static struct {
--
Regards,
Laurent Pinchart
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [v4l-utils PATCH 1/1] v4l: libv4l2subdev: Precisely convert media bus string to code
2016-01-25 19:41 ` Laurent Pinchart
@ 2016-01-25 20:47 ` Sakari Ailus
2016-01-25 20:53 ` Laurent Pinchart
0 siblings, 1 reply; 6+ messages in thread
From: Sakari Ailus @ 2016-01-25 20:47 UTC (permalink / raw)
To: Laurent Pinchart; +Cc: Sakari Ailus, linux-media, hverkuil
Hi Laurent,
On Mon, Jan 25, 2016 at 09:41:12PM +0200, Laurent Pinchart wrote:
> Hi Sakari,
>
> On Monday 25 January 2016 13:39:10 Sakari Ailus wrote:
> > On Sun, Dec 13, 2015 at 11:33:45PM +0200, Laurent Pinchart wrote:
> > > On Wednesday 09 December 2015 17:14:47 Sakari Ailus wrote:
> > >> The length of the string was ignored, making it possible for the
> > >> conversion to fail due to extra characters in the string.
> > >
> > > I'm not sure to follow you there. Is the issue that passing a string such
> > > as "SBGGR10" would match "SBGGR10_DPCM8" if it was listed before
> > > "SBGGR10" ? If that's the case I'd write the commit message as
> >
> > Yes, that's the problem.
> >
> > > Any character beyond the fist `length' characters in the mbus_formats
> > > strings are ignored, causing incorrect matches if the format entry starts
> > > with but isn't equal to the passed format.
> >
> > I'll use this commit message then.
> >
> > >> Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
> > >> ---
> > >> This patch should be applied before the set "[v4l-utils PATCH v2 0/3]
> > >> List supported formats in libv4l2subdev":
> > >>
> > >> <URL:http://www.spinics.net/lists/linux-media/msg95377.html>
> > >>
> > >> utils/media-ctl/libv4l2subdev.c | 10 ++++------
> > >> 1 file changed, 4 insertions(+), 6 deletions(-)
> > >>
> > >> diff --git a/utils/media-ctl/libv4l2subdev.c
> > >> b/utils/media-ctl/libv4l2subdev.c index 33c1ee6..cce527d 100644
> > >> --- a/utils/media-ctl/libv4l2subdev.c
> > >> +++ b/utils/media-ctl/libv4l2subdev.c
> > >> @@ -769,14 +769,12 @@ enum v4l2_mbus_pixelcode
> > >> v4l2_subdev_string_to_pixelcode(const char *string,
> > >> unsigned int i;
> > >>
> > >> for (i = 0; i < ARRAY_SIZE(mbus_formats); ++i) {
> > >> - if (strncmp(mbus_formats[i].name, string, length) == 0)
> > >> - break;
> > >> + if (strncmp(mbus_formats[i].name, string, length) == 0
> > >> + && strlen(mbus_formats[i].name) == length)
> > >
> > > How about mbus_formats[i].name[length] == '\0' instead ? That should be
> > > more efficient.
> >
> > Fine for me.
> >
> > > I also wonder whether we shouldn't just get rid of the length argument and
> > > force the passed format string to be zero-terminated.
> >
> > I believe the reason is that the current user (media-ctl test program)
> > parses the user input and passes a portion of that to this function to
> > convert the string to a numeric value. That'd be a bit more cumbersome as
> > we'd either require copying the string elsewhere or changing the input by
> > the program. I wouldn't change the behaviour, at least not now.
>
> Yes that's the reason, and I think it's an API design mistake (or just a lack
> of proper API design :-)). Wouldn't it be better to copy the string in the
> caller ?
>From the API point of view, I don't disagree. But I do think this should be
done in a separate patch, not in this one --- this is a bugfix, really.
I'll submit one more patch for that.
> > >> + return mbus_formats[i].code;
> > >> }
> > >>
> > >> - if (i == ARRAY_SIZE(mbus_formats))
> > >> - return (enum v4l2_mbus_pixelcode)-1;
> > >> -
> > >> - return mbus_formats[i].code;
> > >> + return (enum v4l2_mbus_pixelcode)-1;
> > >> }
> > >>
> > >> static struct {
>
--
Regards,
Sakari Ailus
e-mail: sakari.ailus@iki.fi XMPP: sailus@retiisi.org.uk
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [v4l-utils PATCH 1/1] v4l: libv4l2subdev: Precisely convert media bus string to code
2016-01-25 20:47 ` Sakari Ailus
@ 2016-01-25 20:53 ` Laurent Pinchart
0 siblings, 0 replies; 6+ messages in thread
From: Laurent Pinchart @ 2016-01-25 20:53 UTC (permalink / raw)
To: Sakari Ailus; +Cc: Sakari Ailus, linux-media, hverkuil
Hi Sakari,
On Monday 25 January 2016 22:47:07 Sakari Ailus wrote:
> On Mon, Jan 25, 2016 at 09:41:12PM +0200, Laurent Pinchart wrote:
> > On Monday 25 January 2016 13:39:10 Sakari Ailus wrote:
> > > On Sun, Dec 13, 2015 at 11:33:45PM +0200, Laurent Pinchart wrote:
> > > > On Wednesday 09 December 2015 17:14:47 Sakari Ailus wrote:
> > > >> The length of the string was ignored, making it possible for the
> > > >> conversion to fail due to extra characters in the string.
> > > >
> > > > I'm not sure to follow you there. Is the issue that passing a string
> > > > such as "SBGGR10" would match "SBGGR10_DPCM8" if it was listed before
> > > > "SBGGR10" ? If that's the case I'd write the commit message as
> > >
> > > Yes, that's the problem.
> > >
> > > > Any character beyond the fist `length' characters in the mbus_formats
> > > > strings are ignored, causing incorrect matches if the format entry
> > > > starts
> > > > with but isn't equal to the passed format.
> > >
> > > I'll use this commit message then.
> > >
> > > >> Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
> > > >> ---
> > > >> This patch should be applied before the set "[v4l-utils PATCH v2 0/3]
> > > >> List supported formats in libv4l2subdev":
> > > >>
> > > >> <URL:http://www.spinics.net/lists/linux-media/msg95377.html>
> > > >>
> > > >> utils/media-ctl/libv4l2subdev.c | 10 ++++------
> > > >> 1 file changed, 4 insertions(+), 6 deletions(-)
> > > >>
> > > >> diff --git a/utils/media-ctl/libv4l2subdev.c
> > > >> b/utils/media-ctl/libv4l2subdev.c index 33c1ee6..cce527d 100644
> > > >> --- a/utils/media-ctl/libv4l2subdev.c
> > > >> +++ b/utils/media-ctl/libv4l2subdev.c
> > > >> @@ -769,14 +769,12 @@ enum v4l2_mbus_pixelcode
> > > >> v4l2_subdev_string_to_pixelcode(const char *string,
> > > >>
> > > >> unsigned int i;
> > > >>
> > > >> for (i = 0; i < ARRAY_SIZE(mbus_formats); ++i) {
> > > >>
> > > >> - if (strncmp(mbus_formats[i].name, string, length) == 0)
> > > >> - break;
> > > >> + if (strncmp(mbus_formats[i].name, string, length) == 0
> > > >> + && strlen(mbus_formats[i].name) == length)
> > > >
> > > > How about mbus_formats[i].name[length] == '\0' instead ? That should
> > > > be more efficient.
> > >
> > > Fine for me.
> > >
> > > > I also wonder whether we shouldn't just get rid of the length argument
> > > > and force the passed format string to be zero-terminated.
> > >
> > > I believe the reason is that the current user (media-ctl test program)
> > > parses the user input and passes a portion of that to this function to
> > > convert the string to a numeric value. That'd be a bit more cumbersome
> > > as we'd either require copying the string elsewhere or changing the
> > > input by the program. I wouldn't change the behaviour, at least not now.
> >
> > Yes that's the reason, and I think it's an API design mistake (or just a
> > lack of proper API design :-)). Wouldn't it be better to copy the string
> > in the caller ?
>
> From the API point of view, I don't disagree. But I do think this should be
> done in a separate patch, not in this one --- this is a bugfix, really.
Sure, I agree with that.
> I'll submit one more patch for that.
Thank you.
> > > >> + return mbus_formats[i].code;
> > > >>
> > > >> }
> > > >>
> > > >> - if (i == ARRAY_SIZE(mbus_formats))
> > > >> - return (enum v4l2_mbus_pixelcode)-1;
> > > >> -
> > > >> - return mbus_formats[i].code;
> > > >> + return (enum v4l2_mbus_pixelcode)-1;
> > > >>
> > > >> }
> > > >>
> > > >> static struct {
--
Regards,
Laurent Pinchart
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2016-01-25 20:53 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2015-12-09 15:14 [v4l-utils PATCH 1/1] v4l: libv4l2subdev: Precisely convert media bus string to code Sakari Ailus
2015-12-13 21:33 ` Laurent Pinchart
2016-01-25 11:39 ` Sakari Ailus
2016-01-25 19:41 ` Laurent Pinchart
2016-01-25 20:47 ` Sakari Ailus
2016-01-25 20:53 ` Laurent Pinchart
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox