Hi Sergio,
On Sunday 29 April 2012 17:36:43 Aguirre, Sergio wrote:
> On Sun, Apr 29, 2012 at 12:40 PM, Laurent Pinchart wrote:
> > On Saturday 28 April 2012 10:04:01 Aguirre, Sergio wrote:
> >> On Wed, Apr 25, 2012 at 8:57 AM, Sergio Aguirre <saaguirre@xxxxxx> wrote:
> >> > Otherwise, some false positives might arise when
> >> > having 2 subdevices with similar names, like:
> >> >
> >> > "OMAP4 ISS ISP IPIPEIF"
> >> > "OMAP4 ISS ISP IPIPE"
> >> >
> >> > Before this patch, trying to find "OMAP4 ISS ISP IPIPE", resulted
> >> > in a false entity match, retrieving "OMAP4 ISS ISP IPIPEIF"
> >> > information instead.
> >> >
> >> > Checking length should ensure such cases are handled well.
> >>
> >> Any feedback about this?
> >
> > Thanks for the patch, and sorry for the delay.
>
> No problem. :)
>
> >> > Signed-off-by: Sergio Aguirre <saaguirre@xxxxxx>
> >> > ---
> >> > src/mediactl.c | 3 ++-
> >> > 1 files changed, 2 insertions(+), 1 deletions(-)
> >> >
> >> > diff --git a/src/mediactl.c b/src/mediactl.c
> >> > index 5b8c587..451a386 100644
> >> > --- a/src/mediactl.c
> >> > +++ b/src/mediactl.c
> >> > @@ -66,7 +66,8 @@ struct media_entity *media_get_entity_by_name(struct
> >> > media_device *media, for (i = 0; i < media->entities_count; ++i) {
> >> > struct media_entity *entity = &media->entities[i];
> >> >
> >> > - if (strncmp(entity->info.name, name, length) == 0)
> >> > + if ((strncmp(entity->info.name, name, length) == 0) &&
> >> > + (strlen(entity->info.name) == length))
> >> > return entity;
> >> > }
> >
> > Instead of calling strlen() which has a O(n) complexity, what about just
> > checking that the entity name has a '\0' in the length'th position ?
> > Something like the following patch:
> >
> > From 46bec667b675573cf1ce698c68112e3dbd31930e Mon Sep 17 00:00:00 2001
> > From: Sergio Aguirre <saaguirre@xxxxxx>
> > Date: Wed, 25 Apr 2012 08:57:13 -0500
> > Subject: [PATCH] Compare name length to avoid false positives in
> > media_get_entity_by_name
> >
> > If two subdevice have names that only differ by a suffix (such as "OMAP4
> > ISS ISP IPIPE" and "OMAP4 ISS ISP IPIPEIF") the media_get_entity_by_name
> > function might return a pointer to the entity with the longest name when
> > called with the shortest name. Fix this by verifying that the candidate
> > entity name length is equal to the requested name length.
> >
> > Signed-off-by: Sergio Aguirre <saaguirre@xxxxxx>
> > Signed-off-by: Laurent Pinchart <laurent.pinchart@xxxxxxxxxxxxxxxx>
> > ---
> > src/mediactl.c | 9 ++++++++-
> > src/tools.h | 1 +
> > 2 files changed, 9 insertions(+), 1 deletions(-)
> >
> > diff --git a/src/mediactl.c b/src/mediactl.c
> > index 5b8c587..bc6a713 100644
> > --- a/src/mediactl.c
> > +++ b/src/mediactl.c
> > @@ -63,10 +63,17 @@ struct media_entity *media_get_entity_by_name(struct
> > media_device *media,
> > {
> > unsigned int i;
> >
> > + /* A match is impossible if the entity name is longer than the
> > maximum + * size we can get from the kernel.
> > + */
> > + if (length >= FIELD_SIZEOF(struct media_entity_desc, name))
> > + return NULL;
> > +
>
> Good idea.
>
> > for (i = 0; i < media->entities_count; ++i) {
> > struct media_entity *entity = &media->entities[i];
> >
> > - if (strncmp(entity->info.name, name, length) == 0)
> > + if (strncmp(entity->info.name, name, length) == 0 &&
> > + entity->info.name[length] == '\0')
>
> ACK.
>
> Your patch is definitely better.
>
> IMHO, I think it'll be more fair if you put yourself as the author of this
> new patch, and me as: "Reported-by:".
It's such a small patch, it's fine :-) Thank you for the proposal nonetheless.
I've pushed the patch to the repository.
> > return entity;
> > }
> >
> > diff --git a/src/tools.h b/src/tools.h
> > index e56edb2..de06cb3 100644
> > --- a/src/tools.h
> > +++ b/src/tools.h
> > @@ -23,6 +23,7 @@
> > #define __TOOLS_H__
> >
> > #define ARRAY_SIZE(array) (sizeof(array) / sizeof((array)[0]))
> > +#define FIELD_SIZEOF(t, f) (sizeof(((t*)0)->f))
> >
> > #endif /* __TOOLS_H__ */
> >
--
Regards,
Laurent Pinchart
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
