[PATCH resend] OMAP: DSS2: Replace strncmp() with sysfs_streq() in overlay_manager_store()

[PATCH resend] OMAP: DSS2: Replace strncmp() with sysfs_streq() in overlay_manager_store()

[Archit Taneja]

In the function overlay_manager_store, the length of the string
buf is used to comapre the 2 strings, there is a possibility of
false positive match if buf is a prefix string of another manager.

The use of sysfs_streq() fixes this.

Signed-off-by: Archit Taneja <archit@ti.com>
---
 drivers/video/omap2/dss/overlay.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/drivers/video/omap2/dss/overlay.c b/drivers/video/omap2/dss/overlay.c
index 8233658..244dca8
--- a/drivers/video/omap2/dss/overlay.c
+++ b/drivers/video/omap2/dss/overlay.c
@@ -65,7 +65,7 @@ static ssize_t overlay_manager_store(struct omap_overlay *ovl, const char *buf,
 		for (i = 0; i < omap_dss_get_num_overlay_managers(); ++i) {
 			mgr = omap_dss_get_overlay_manager(i);
 
-			if (strncmp(buf, mgr->name, len) == 0)
+			if (sysfs_streq(buf, mgr->name))
 				break;
 
 			mgr = NULL;
-- 
1.5.4.7

RE: [PATCH resend] OMAP: DSS2: Replace strncmp() with sysfs_streq() in overlay_manager_store()

[Premi, Sanjeev]

> -----Original Message-----
> From: linux-omap-owner@vger.kernel.org 
> [mailto:linux-omap-owner@vger.kernel.org] On Behalf Of Taneja, Archit
> Sent: Wednesday, July 28, 2010 11:52 AM
> To: tomi.valkeinen@nokia.com
> Cc: linux-omap@vger.kernel.org; Taneja, Archit
> Subject: [PATCH resend] OMAP: DSS2: Replace strncmp() with 
> sysfs_streq() in overlay_manager_store()
> 
> In the function overlay_manager_store, the length of the string
> buf is used to comapre the 2 strings, there is a possibility of
> false positive match if buf is a prefix string of another manager.
> 
> The use of sysfs_streq() fixes this.
> 
> Signed-off-by: Archit Taneja <archit@ti.com>
> ---
>  drivers/video/omap2/dss/overlay.c |    2 +-
>  1 files changed, 1 insertions(+), 1 deletions(-)
> 
> diff --git a/drivers/video/omap2/dss/overlay.c 
> b/drivers/video/omap2/dss/overlay.c
> index 8233658..244dca8
> --- a/drivers/video/omap2/dss/overlay.c
> +++ b/drivers/video/omap2/dss/overlay.c
> @@ -65,7 +65,7 @@ static ssize_t overlay_manager_store(struct 
> omap_overlay *ovl, const char *buf,
>  		for (i = 0; i < 
> omap_dss_get_num_overlay_managers(); ++i) {
>  			mgr = omap_dss_get_overlay_manager(i);
>  
> -			if (strncmp(buf, mgr->name, len) == 0)
> +			if (sysfs_streq(buf, mgr->name))

[sp] sysfs_streq() ignores trailing newlines during comparison. So,
     the possibility mentioned in the description still stays.

     I am not familiar with overall context; but wouldn't srtcmp()
     be the right choice? unless, of course, either of strings
     being compared are not null terminated.

>  				break;
>  
>  			mgr = NULL;
> -- 
> 1.5.4.7
> 
> --
> To unsubscribe from this list: send the line "unsubscribe 
> linux-omap" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 

RE: [PATCH resend] OMAP: DSS2: Replace strncmp() with sysfs_streq() in overlay_manager_store()

[Taneja, Archit]

 

> -----Original Message-----
> From: Premi, Sanjeev 
> Sent: Wednesday, July 28, 2010 7:28 PM
> To: Taneja, Archit; tomi.valkeinen@nokia.com
> Cc: linux-omap@vger.kernel.org
> Subject: RE: [PATCH resend] OMAP: DSS2: Replace strncmp() 
> with sysfs_streq() in overlay_manager_store()
> 
> > -----Original Message-----
> > From: linux-omap-owner@vger.kernel.org 
> > [mailto:linux-omap-owner@vger.kernel.org] On Behalf Of 
> Taneja, Archit
> > Sent: Wednesday, July 28, 2010 11:52 AM
> > To: tomi.valkeinen@nokia.com
> > Cc: linux-omap@vger.kernel.org; Taneja, Archit
> > Subject: [PATCH resend] OMAP: DSS2: Replace strncmp() with
> > sysfs_streq() in overlay_manager_store()
> > 
> > In the function overlay_manager_store, the length of the 
> string buf is 
> > used to comapre the 2 strings, there is a possibility of false 
> > positive match if buf is a prefix string of another manager.
> > 
> > The use of sysfs_streq() fixes this.
> > 
> > Signed-off-by: Archit Taneja <archit@ti.com>
> > ---
> >  drivers/video/omap2/dss/overlay.c |    2 +-
> >  1 files changed, 1 insertions(+), 1 deletions(-)
> > 
> > diff --git a/drivers/video/omap2/dss/overlay.c
> > b/drivers/video/omap2/dss/overlay.c
> > index 8233658..244dca8
> > --- a/drivers/video/omap2/dss/overlay.c
> > +++ b/drivers/video/omap2/dss/overlay.c
> > @@ -65,7 +65,7 @@ static ssize_t overlay_manager_store(struct 
> > omap_overlay *ovl, const char *buf,
> >  		for (i = 0; i <
> > omap_dss_get_num_overlay_managers(); ++i) {
> >  			mgr = omap_dss_get_overlay_manager(i);
> >  
> > -			if (strncmp(buf, mgr->name, len) == 0)
> > +			if (sysfs_streq(buf, mgr->name))
> 
> [sp] sysfs_streq() ignores trailing newlines during comparison. So,
>      the possibility mentioned in the description still stays.

The aim is to compare one string which is a sysfs input and the other
which is in the kernel. 

> 
>      I am not familiar with overall context; but wouldn't srtcmp()
>      be the right choice? unless, of course, either of strings
>      being compared are not null terminated.

The sysfs input will have a newline and null at the end whereas the
other string will only have null, strcmp will fail when we want the two
strings to match.

Eg. Sysfs input "lcd\n\0"
Kernel string "lcd\0"

strcmp will fail here

> 
> >  				break;
> >  
> >  			mgr = NULL;
> > --
> > 1.5.4.7
> > 
> > --
> > To unsubscribe from this list: send the line "unsubscribe 
> linux-omap" 
> > in the body of a message to majordomo@vger.kernel.org More 
> majordomo 
> > info at  http://vger.kernel.org/majordomo-info.html
> > 

RE: [PATCH resend] OMAP: DSS2: Replace strncmp() with sysfs_streq() in overlay_manager_store()

[Premi, Sanjeev]

 

> -----Original Message-----
> From: Taneja, Archit 
> Sent: Wednesday, July 28, 2010 8:06 PM
> To: Premi, Sanjeev; tomi.valkeinen@nokia.com
> Cc: linux-omap@vger.kernel.org
> Subject: RE: [PATCH resend] OMAP: DSS2: Replace strncmp() 
> with sysfs_streq() in overlay_manager_store()
> 
>  
> 
> > -----Original Message-----
> > From: Premi, Sanjeev 
> > Sent: Wednesday, July 28, 2010 7:28 PM
> > To: Taneja, Archit; tomi.valkeinen@nokia.com
> > Cc: linux-omap@vger.kernel.org
> > Subject: RE: [PATCH resend] OMAP: DSS2: Replace strncmp() 
> > with sysfs_streq() in overlay_manager_store()
> > 
> > > -----Original Message-----
> > > From: linux-omap-owner@vger.kernel.org 
> > > [mailto:linux-omap-owner@vger.kernel.org] On Behalf Of 
> > Taneja, Archit
> > > Sent: Wednesday, July 28, 2010 11:52 AM
> > > To: tomi.valkeinen@nokia.com
> > > Cc: linux-omap@vger.kernel.org; Taneja, Archit
> > > Subject: [PATCH resend] OMAP: DSS2: Replace strncmp() with
> > > sysfs_streq() in overlay_manager_store()
> > > 
> > > In the function overlay_manager_store, the length of the 
> > string buf is 
> > > used to comapre the 2 strings, there is a possibility of false 
> > > positive match if buf is a prefix string of another manager.
> > > 
> > > The use of sysfs_streq() fixes this.
> > > 
> > > Signed-off-by: Archit Taneja <archit@ti.com>
> > > ---
> > >  drivers/video/omap2/dss/overlay.c |    2 +-
> > >  1 files changed, 1 insertions(+), 1 deletions(-)
> > > 
> > > diff --git a/drivers/video/omap2/dss/overlay.c
> > > b/drivers/video/omap2/dss/overlay.c
> > > index 8233658..244dca8
> > > --- a/drivers/video/omap2/dss/overlay.c
> > > +++ b/drivers/video/omap2/dss/overlay.c
> > > @@ -65,7 +65,7 @@ static ssize_t overlay_manager_store(struct 
> > > omap_overlay *ovl, const char *buf,
> > >  		for (i = 0; i <
> > > omap_dss_get_num_overlay_managers(); ++i) {
> > >  			mgr = omap_dss_get_overlay_manager(i);
> > >  
> > > -			if (strncmp(buf, mgr->name, len) == 0)
> > > +			if (sysfs_streq(buf, mgr->name))
> > 
> > [sp] sysfs_streq() ignores trailing newlines during comparison. So,
> >      the possibility mentioned in the description still stays.
> 
> The aim is to compare one string which is a sysfs input and the other
> which is in the kernel. 
> 
> > 
> >      I am not familiar with overall context; but wouldn't srtcmp()
> >      be the right choice? unless, of course, either of strings
> >      being compared are not null terminated.
> 
> The sysfs input will have a newline and null at the end whereas the
> other string will only have null, strcmp will fail when we 
> want the two
> strings to match.
> 
> Eg. Sysfs input "lcd\n\0"
> Kernel string "lcd\0"
> 
> strcmp will fail here

[sp] If the patch is intending to fix this condition, then it isn't
     evident from the description. You should consider updating the
     description.

> 
> > 
> > >  				break;
> > >  
> > >  			mgr = NULL;
> > > --
> > > 1.5.4.7
> > > 
> > > --
> > > To unsubscribe from this list: send the line "unsubscribe 
> > linux-omap" 
> > > in the body of a message to majordomo@vger.kernel.org More 
> > majordomo 
> > > info at  http://vger.kernel.org/majordomo-info.html
> > > 

RE: [PATCH resend] OMAP: DSS2: Replace strncmp() with sysfs_streq() in overlay_manager_store()

[Taneja, Archit]

> > > > -----Original Message-----
> > > > From: linux-omap-owner@vger.kernel.org 
> > > > [mailto:linux-omap-owner@vger.kernel.org] On Behalf Of
> > > Taneja, Archit
> > > > Sent: Wednesday, July 28, 2010 11:52 AM
> > > > To: tomi.valkeinen@nokia.com
> > > > Cc: linux-omap@vger.kernel.org; Taneja, Archit
> > > > Subject: [PATCH resend] OMAP: DSS2: Replace strncmp() with
> > > > sysfs_streq() in overlay_manager_store()
> > > > 
> > > > In the function overlay_manager_store, the length of the
> > > string buf is
> > > > used to comapre the 2 strings, there is a possibility of false 
> > > > positive match if buf is a prefix string of another manager.
> > > > 
> > > > The use of sysfs_streq() fixes this.
> > > > 
> > > > Signed-off-by: Archit Taneja <archit@ti.com>
> > > > ---
> > > >  drivers/video/omap2/dss/overlay.c |    2 +-
> > > >  1 files changed, 1 insertions(+), 1 deletions(-)
> > > > 
> > > > diff --git a/drivers/video/omap2/dss/overlay.c
> > > > b/drivers/video/omap2/dss/overlay.c
> > > > index 8233658..244dca8
> > > > --- a/drivers/video/omap2/dss/overlay.c
> > > > +++ b/drivers/video/omap2/dss/overlay.c
> > > > @@ -65,7 +65,7 @@ static ssize_t overlay_manager_store(struct 
> > > > omap_overlay *ovl, const char *buf,
> > > >  		for (i = 0; i <
> > > > omap_dss_get_num_overlay_managers(); ++i) {
> > > >  			mgr = omap_dss_get_overlay_manager(i);
> > > >  
> > > > -			if (strncmp(buf, mgr->name, len) == 0)
> > > > +			if (sysfs_streq(buf, mgr->name))
> > > 
> > > [sp] sysfs_streq() ignores trailing newlines during 
> comparison. So,
> > >      the possibility mentioned in the description still stays.
> > 
> > The aim is to compare one string which is a sysfs input and 
> the other 
> > which is in the kernel.
> > 
> > > 
> > >      I am not familiar with overall context; but wouldn't srtcmp()
> > >      be the right choice? unless, of course, either of strings
> > >      being compared are not null terminated.
> > 
> > The sysfs input will have a newline and null at the end whereas the 
> > other string will only have null, strcmp will fail when we want the 
> > two strings to match.
> > 
> > Eg. Sysfs input "lcd\n\0"
> > Kernel string "lcd\0"
> > 
> > strcmp will fail here
> 
> [sp] If the patch is intending to fix this condition, then it isn't
>      evident from the description. You should consider updating the
>      description.

The patch isn't intending to fix this condition, this condition doesn't
occur at all in the existing code. I explained this to tell you why strcmp
is a bad choice, the patch description tells why sysfs_streq is a better
choice over strncmp.

> 
> > 
> > > 
> > > >  				break;
> > > >  
> > > >  			mgr = NULL;
> > > > --
> > > > 1.5.4.7
> > > > 
> > > > --
> > > > To unsubscribe from this list: send the line "unsubscribe
> > > linux-omap" 
> > > > in the body of a message to majordomo@vger.kernel.org More
> > > majordomo
> > > > info at  http://vger.kernel.org/majordomo-info.html
> > > > 

RE: [PATCH resend] OMAP: DSS2: Replace strncmp() with sysfs_streq() in overlay_manager_store()

[Premi, Sanjeev]

> -----Original Message-----
> From: Taneja, Archit 
> Sent: Thursday, July 29, 2010 9:16 AM
> To: Premi, Sanjeev; tomi.valkeinen@nokia.com
> Cc: linux-omap@vger.kernel.org
> Subject: RE: [PATCH resend] OMAP: DSS2: Replace strncmp() 
> with sysfs_streq() in overlay_manager_store()
> 
> 
> > > > > -----Original Message-----
> > > > > From: linux-omap-owner@vger.kernel.org 
> > > > > [mailto:linux-omap-owner@vger.kernel.org] On Behalf Of
> > > > Taneja, Archit
> > > > > Sent: Wednesday, July 28, 2010 11:52 AM
> > > > > To: tomi.valkeinen@nokia.com
> > > > > Cc: linux-omap@vger.kernel.org; Taneja, Archit
> > > > > Subject: [PATCH resend] OMAP: DSS2: Replace strncmp() with
> > > > > sysfs_streq() in overlay_manager_store()
> > > > > 
> > > > > In the function overlay_manager_store, the length of the
> > > > string buf is
> > > > > used to comapre the 2 strings, there is a possibility 
> of false 
> > > > > positive match if buf is a prefix string of another manager.
> > > > > 
> > > > > The use of sysfs_streq() fixes this.
> > > > > 
> > > > > Signed-off-by: Archit Taneja <archit@ti.com>
> > > > > ---
> > > > >  drivers/video/omap2/dss/overlay.c |    2 +-
> > > > >  1 files changed, 1 insertions(+), 1 deletions(-)
> > > > > 
> > > > > diff --git a/drivers/video/omap2/dss/overlay.c
> > > > > b/drivers/video/omap2/dss/overlay.c
> > > > > index 8233658..244dca8
> > > > > --- a/drivers/video/omap2/dss/overlay.c
> > > > > +++ b/drivers/video/omap2/dss/overlay.c
> > > > > @@ -65,7 +65,7 @@ static ssize_t overlay_manager_store(struct 
> > > > > omap_overlay *ovl, const char *buf,
> > > > >  		for (i = 0; i <
> > > > > omap_dss_get_num_overlay_managers(); ++i) {
> > > > >  			mgr = omap_dss_get_overlay_manager(i);
> > > > >  
> > > > > -			if (strncmp(buf, mgr->name, len) == 0)
> > > > > +			if (sysfs_streq(buf, mgr->name))
> > > > 
> > > > [sp] sysfs_streq() ignores trailing newlines during 
> > comparison. So,
> > > >      the possibility mentioned in the description still stays.
> > > 
> > > The aim is to compare one string which is a sysfs input and 
> > the other 
> > > which is in the kernel.
> > > 
> > > > 
> > > >      I am not familiar with overall context; but 
> wouldn't srtcmp()
> > > >      be the right choice? unless, of course, either of strings
> > > >      being compared are not null terminated.
> > > 
> > > The sysfs input will have a newline and null at the end 
> whereas the 
> > > other string will only have null, strcmp will fail when 
> we want the 
> > > two strings to match.
> > > 
> > > Eg. Sysfs input "lcd\n\0"
> > > Kernel string "lcd\0"
> > > 
> > > strcmp will fail here
> > 
> > [sp] If the patch is intending to fix this condition, then it isn't
> >      evident from the description. You should consider updating the
> >      description.
> 
> The patch isn't intending to fix this condition, this 
> condition doesn't
> occur at all in the existing code. I explained this to tell 
> you why strcmp
> is a bad choice, the patch description tells why sysfs_streq 
> is a better
> choice over strncmp.

[sp] Can you explain the real condition and how/why it can't be handled
     by strcmp()

> 
> > 
> > > 
> > > > 
> > > > >  				break;
> > > > >  
> > > > >  			mgr = NULL;
> > > > > --
> > > > > 1.5.4.7
> > > > > 
> > > > > --
> > > > > To unsubscribe from this list: send the line "unsubscribe
> > > > linux-omap" 
> > > > > in the body of a message to majordomo@vger.kernel.org More
> > > > majordomo
> > > > > info at  http://vger.kernel.org/majordomo-info.html
> > > > > 

RE: [PATCH resend] OMAP: DSS2: Replace strncmp() with sysfs_streq() in overlay_manager_store()

[Taneja, Archit]

> > 
> > > > > > -----Original Message-----
> > > > > > From: linux-omap-owner@vger.kernel.org 
> > > > > > [mailto:linux-omap-owner@vger.kernel.org] On Behalf Of
> > > > > Taneja, Archit
> > > > > > Sent: Wednesday, July 28, 2010 11:52 AM
> > > > > > To: tomi.valkeinen@nokia.com
> > > > > > Cc: linux-omap@vger.kernel.org; Taneja, Archit
> > > > > > Subject: [PATCH resend] OMAP: DSS2: Replace strncmp() with
> > > > > > sysfs_streq() in overlay_manager_store()
> > > > > > 
> > > > > > In the function overlay_manager_store, the length of the
> > > > > string buf is
> > > > > > used to comapre the 2 strings, there is a possibility
> > of false
> > > > > > positive match if buf is a prefix string of another manager.
> > > > > > 
> > > > > > The use of sysfs_streq() fixes this.
> > > > > > 
> > > > > > Signed-off-by: Archit Taneja <archit@ti.com>
> > > > > > ---
> > > > > >  drivers/video/omap2/dss/overlay.c |    2 +-
> > > > > >  1 files changed, 1 insertions(+), 1 deletions(-)
> > > > > > 
> > > > > > diff --git a/drivers/video/omap2/dss/overlay.c
> > > > > > b/drivers/video/omap2/dss/overlay.c
> > > > > > index 8233658..244dca8
> > > > > > --- a/drivers/video/omap2/dss/overlay.c
> > > > > > +++ b/drivers/video/omap2/dss/overlay.c
> > > > > > @@ -65,7 +65,7 @@ static ssize_t 
> overlay_manager_store(struct 
> > > > > > omap_overlay *ovl, const char *buf,
> > > > > >  		for (i = 0; i <
> > > > > > omap_dss_get_num_overlay_managers(); ++i) {
> > > > > >  			mgr = omap_dss_get_overlay_manager(i);
> > > > > >  
> > > > > > -			if (strncmp(buf, mgr->name, len) == 0)
> > > > > > +			if (sysfs_streq(buf, mgr->name))
> > > > > 
> > > > > [sp] sysfs_streq() ignores trailing newlines during
> > > comparison. So,
> > > > >      the possibility mentioned in the description still stays.
> > > > 
> > > > The aim is to compare one string which is a sysfs input and
> > > the other
> > > > which is in the kernel.
> > > > 
> > > > > 
> > > > >      I am not familiar with overall context; but
> > wouldn't srtcmp()
> > > > >      be the right choice? unless, of course, either of strings
> > > > >      being compared are not null terminated.
> > > > 
> > > > The sysfs input will have a newline and null at the end
> > whereas the
> > > > other string will only have null, strcmp will fail when
> > we want the
> > > > two strings to match.
> > > > 
> > > > Eg. Sysfs input "lcd\n\0"
> > > > Kernel string "lcd\0"
> > > > 
> > > > strcmp will fail here
> > > 
> > > [sp] If the patch is intending to fix this condition, 
> then it isn't
> > >      evident from the description. You should consider 
> updating the
> > >      description.
> > 
> > The patch isn't intending to fix this condition, this condition 
> > doesn't occur at all in the existing code. I explained this to tell 
> > you why strcmp is a bad choice, the patch description tells why 
> > sysfs_streq is a better choice over strncmp.
> 
> [sp] Can you explain the real condition and how/why it can't 
> be handled
>      by strcmp()

Okay, the real condition (why this patch was sent) is this (please have a look
at the whole function body while reading the explanation):

The user enters "lcd" via sysfs input to change the overlay's manager to "lcd",
this input will be converted to "lcd\n\0", the function will try to
comapre this name with all the existing managers names. Consider the case where
buf (sysfs input) is "lcd\n\0" and mgr->name is "lcd2\0".

Now, len is calculated as 3 and is passed as the parameter to stncmp, in this
case, "lcd" and "lcd2" will match because only the first 3 characters are compared.
This is what I meant by buf being a prefix string of mgr->name.

This above is the condition which the patch tries to resolve.

strcmp will work like a charm here and ignore false positives, but it will generate
a false negative which didn't occur previously at all, an example:

If buf is "lcd\n\0" and mgr->name is "lcd\0" the code should match this, but strcmp
won't.

Hence, to prevent both false positives and false negatives sysfs_streq is used.

If you want to use strcmp, you will have to manually strip off the newlines.

I have also shared the patch link below which intrioduces sysfs_streq in the kernel
and explains why it has been introduced in the first place:

http://kerneltrap.org/mailarchive/git-commits-head/2008/5/1/1688084

I hope this explanation elaborates things clearly.

Thanks,

Archit

RE: [PATCH resend] OMAP: DSS2: Replace strncmp() with sysfs_streq() in overlay_manager_store()

[Premi, Sanjeev]

 

> -----Original Message-----
> From: Taneja, Archit 
> Sent: Thursday, July 29, 2010 11:27 AM
> To: Premi, Sanjeev; tomi.valkeinen@nokia.com
> Cc: linux-omap@vger.kernel.org
> Subject: RE: [PATCH resend] OMAP: DSS2: Replace strncmp() 
> with sysfs_streq() in overlay_manager_store()
> 

[snip]...[snip]

> > > > > > [sp] sysfs_streq() ignores trailing newlines during
> > > > comparison. So,
> > > > > >      the possibility mentioned in the description 
> still stays.
> > > > > 
> > > > > The aim is to compare one string which is a sysfs input and
> > > > the other
> > > > > which is in the kernel.
> > > > > 
> > > > > > 
> > > > > >      I am not familiar with overall context; but
> > > wouldn't srtcmp()
> > > > > >      be the right choice? unless, of course, either 
> of strings
> > > > > >      being compared are not null terminated.
> > > > > 
> > > > > The sysfs input will have a newline and null at the end
> > > whereas the
> > > > > other string will only have null, strcmp will fail when
> > > we want the
> > > > > two strings to match.
> > > > > 
> > > > > Eg. Sysfs input "lcd\n\0"
> > > > > Kernel string "lcd\0"
> > > > > 
> > > > > strcmp will fail here
> > > > 
> > > > [sp] If the patch is intending to fix this condition, 
> > then it isn't
> > > >      evident from the description. You should consider 
> > updating the
> > > >      description.
> > > 
> > > The patch isn't intending to fix this condition, this condition 
> > > doesn't occur at all in the existing code. I explained 
> this to tell 
> > > you why strcmp is a bad choice, the patch description tells why 
> > > sysfs_streq is a better choice over strncmp.
> > 
> > [sp] Can you explain the real condition and how/why it can't 
> > be handled
> >      by strcmp()
> 
> Okay, the real condition (why this patch was sent) is this 
> (please have a look
> at the whole function body while reading the explanation):
> 
> The user enters "lcd" via sysfs input to change the overlay's 
> manager to "lcd",
> this input will be converted to "lcd\n\0", the function will try to
> comapre this name with all the existing managers names. 
> Consider the case where
> buf (sysfs input) is "lcd\n\0" and mgr->name is "lcd2\0".
> 
> Now, len is calculated as 3 and is passed as the parameter to 
> stncmp, in this
> case, "lcd" and "lcd2" will match because only the first 3 
> characters are compared.
> This is what I meant by buf being a prefix string of mgr->name.
> 
> This above is the condition which the patch tries to resolve.
> 
> strcmp will work like a charm here and ignore false 
> positives, but it will generate
> a false negative which didn't occur previously at all, an example:
> 
> If buf is "lcd\n\0" and mgr->name is "lcd\0" the code should 
> match this, but strcmp
> won't.
> 

[sp] This explains. And my earlier comment was to update the
     description with need for change. I believe quick summary
     the discussion above will be good for the patch.

> Hence, to prevent both false positives and false negatives 
> sysfs_streq is used.
> 
> If you want to use strcmp, you will have to manually strip 
> off the newlines.
> 
> I have also shared the patch link below which intrioduces 
> sysfs_streq in the kernel
> and explains why it has been introduced in the first place:
> 
> http://kerneltrap.org/mailarchive/git-commits-head/2008/5/1/1688084
> 

[sp] I am aware of it. See my first comment.

> I hope this explanation elaborates things clearly.
> 
> Thanks,
> 
> Archit
> 

RE: [PATCH resend] OMAP: DSS2: Replace strncmp() with sysfs_streq() in overlay_manager_store()

[Taneja, Archit]

 
[snip]
> > 
> 
> [sp] This explains. And my earlier comment was to update the
>      description with need for change. I believe quick summary
>      the discussion above will be good for the patch.
>

Okay, will do. But what am is supposed to add more in the summary?
Should I add how sysfs_streq is a better choice over strcmp? Or
should I emphasize more how the false positive scenario
can occur with strncmp?

Thanks,

Archit

RE: [PATCH resend] OMAP: DSS2: Replace strncmp() with sysfs_streq() in overlay_manager_store()

[Premi, Sanjeev]

> -----Original Message-----
> From: Taneja, Archit 
> Sent: Thursday, July 29, 2010 4:40 PM
> To: Premi, Sanjeev; tomi.valkeinen@nokia.com
> Cc: linux-omap@vger.kernel.org
> Subject: RE: [PATCH resend] OMAP: DSS2: Replace strncmp() 
> with sysfs_streq() in overlay_manager_store()
> 
>  
> [snip]
> > > 
> > 
> > [sp] This explains. And my earlier comment was to update the
> >      description with need for change. I believe quick summary
> >      the discussion above will be good for the patch.
> >
> 
> Okay, will do. But what am is supposed to add more in the summary?
> Should I add how sysfs_streq is a better choice over strcmp? Or
> should I emphasize more how the false positive scenario
> can occur with strncmp?

I believe it should indicate problem being fixed. sysfs_streq() vs
strncmp() is means of arriving at solution.

> 
> Thanks,
> 
> Archit