Hi Laurent,
On Sun, Apr 29, 2012 at 12:40 PM, Laurent Pinchart
<laurent.pinchart@xxxxxxxxxxxxxxxx> wrote:
> Hi Sergio,
>
> 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:".
Regards,
Sergio
> 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
