All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH 4/4] fs/qnx4: decrement sizeof size in strncmp
@ 2009-11-12  7:49 ` Julia Lawall
  0 siblings, 0 replies; 22+ messages in thread
From: Julia Lawall @ 2009-11-12  7:49 UTC (permalink / raw)
  To: al, linux-kernel, kernel-janitors

From: Julia Lawall <julia@diku.dk>

As observed by Joe Perches, sizeof of a constant string includes the
trailing 0.  If what is wanted is to check the initial characters of
another string, this trailing 0 should not be taken into account.  If an
exact match is wanted, strcmp should be used instead.

The semantic patch that makes this change is as follows:
(http://coccinelle.lip6.fr/)

// <smpl>
@@
expression foo;
constant char *abc;
@@

strncmp(foo, abc, 
- sizeof(abc)
+ sizeof(abc)-1
 )
// </smpl>

Signed-off-by: Julia Lawall <julia@diku.dk>

---
 fs/qnx4/inode.c |    4 +++-
 1 files changed, 3 insertions(+), 1 deletions(-)

diff --git a/fs/qnx4/inode.c b/fs/qnx4/inode.c
index 449f5a6..0614b00 100644
--- a/fs/qnx4/inode.c
+++ b/fs/qnx4/inode.c
@@ -221,7 +221,9 @@ static const char *qnx4_checkroot(struct super_block *sb)
 				rootdir = (struct qnx4_inode_entry *) (bh->b_data + i * QNX4_DIR_ENTRY_SIZE);
 				if (rootdir->di_fname != NULL) {
 					QNX4DEBUG((KERN_INFO "rootdir entry found : [%s]\n", rootdir->di_fname));
-					if (!strncmp(rootdir->di_fname, QNX4_BMNAME, sizeof QNX4_BMNAME)) {
+					if (!strncmp(rootdir->di_fname,
+						     QNX4_BMNAME,
+						     sizeof QNX4_BMNAME - 1)) {
 						found = 1;
 						qnx4_sb(sb)->BitMap = kmalloc( sizeof( struct qnx4_inode_entry ), GFP_KERNEL );
 						if (!qnx4_sb(sb)->BitMap) {

^ permalink raw reply related	[flat|nested] 22+ messages in thread

* [PATCH 4/4] fs/qnx4: decrement sizeof size in strncmp
@ 2009-11-12  7:49 ` Julia Lawall
  0 siblings, 0 replies; 22+ messages in thread
From: Julia Lawall @ 2009-11-12  7:49 UTC (permalink / raw)
  To: al, linux-kernel, kernel-janitors

From: Julia Lawall <julia@diku.dk>

As observed by Joe Perches, sizeof of a constant string includes the
trailing 0.  If what is wanted is to check the initial characters of
another string, this trailing 0 should not be taken into account.  If an
exact match is wanted, strcmp should be used instead.

The semantic patch that makes this change is as follows:
(http://coccinelle.lip6.fr/)

// <smpl>
@@
expression foo;
constant char *abc;
@@

strncmp(foo, abc, 
- sizeof(abc)
+ sizeof(abc)-1
 )
// </smpl>

Signed-off-by: Julia Lawall <julia@diku.dk>

---
 fs/qnx4/inode.c |    4 +++-
 1 files changed, 3 insertions(+), 1 deletions(-)

diff --git a/fs/qnx4/inode.c b/fs/qnx4/inode.c
index 449f5a6..0614b00 100644
--- a/fs/qnx4/inode.c
+++ b/fs/qnx4/inode.c
@@ -221,7 +221,9 @@ static const char *qnx4_checkroot(struct super_block *sb)
 				rootdir = (struct qnx4_inode_entry *) (bh->b_data + i * QNX4_DIR_ENTRY_SIZE);
 				if (rootdir->di_fname != NULL) {
 					QNX4DEBUG((KERN_INFO "rootdir entry found : [%s]\n", rootdir->di_fname));
-					if (!strncmp(rootdir->di_fname, QNX4_BMNAME, sizeof QNX4_BMNAME)) {
+					if (!strncmp(rootdir->di_fname,
+						     QNX4_BMNAME,
+						     sizeof QNX4_BMNAME - 1)) {
 						found = 1;
 						qnx4_sb(sb)->BitMap = kmalloc( sizeof( struct qnx4_inode_entry ), GFP_KERNEL );
 						if (!qnx4_sb(sb)->BitMap) {

^ permalink raw reply related	[flat|nested] 22+ messages in thread

* Re: [PATCH 4/4] fs/qnx4: decrement sizeof size in strncmp
  2009-11-12  7:49 ` Julia Lawall
@ 2009-11-12  9:25   ` Anders Larsen
  -1 siblings, 0 replies; 22+ messages in thread
From: Anders Larsen @ 2009-11-12  9:25 UTC (permalink / raw)
  To: Julia Lawall; +Cc: linux-kernel, kernel-janitors

On 2009-11-12 08:49:44, Julia Lawall wrote:
> As observed by Joe Perches, sizeof of a constant string includes the
> trailing 0.  If what is wanted is to check the initial characters of
> another string, this trailing 0 should not be taken into account.  If an
> exact match is wanted, strcmp should be used instead.

Good catch.
However, in this case an exact match (with the string ".bitmap") is what
is needed, so

NAK

Will you cook up a new patch changing strncmp to strcmp?

Cheers
Anders


^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: [PATCH 4/4] fs/qnx4: decrement sizeof size in strncmp
@ 2009-11-12  9:25   ` Anders Larsen
  0 siblings, 0 replies; 22+ messages in thread
From: Anders Larsen @ 2009-11-12  9:25 UTC (permalink / raw)
  To: Julia Lawall; +Cc: linux-kernel, kernel-janitors

On 2009-11-12 08:49:44, Julia Lawall wrote:
> As observed by Joe Perches, sizeof of a constant string includes the
> trailing 0.  If what is wanted is to check the initial characters of
> another string, this trailing 0 should not be taken into account.  If an
> exact match is wanted, strcmp should be used instead.

Good catch.
However, in this case an exact match (with the string ".bitmap") is what
is needed, so

NAK

Will you cook up a new patch changing strncmp to strcmp?

Cheers
Anders


^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: [PATCH 4/4] fs/qnx4: decrement sizeof size in strncmp
  2009-11-12  9:25   ` Anders Larsen
@ 2009-11-12 10:05     ` Julia Lawall
  -1 siblings, 0 replies; 22+ messages in thread
From: Julia Lawall @ 2009-11-12 10:05 UTC (permalink / raw)
  To: Anders Larsen; +Cc: linux-kernel, kernel-janitors

On Thu, 12 Nov 2009, Anders Larsen wrote:

> On 2009-11-12 08:49:44, Julia Lawall wrote:
> > As observed by Joe Perches, sizeof of a constant string includes the
> > trailing 0.  If what is wanted is to check the initial characters of
> > another string, this trailing 0 should not be taken into account.  If an
> > exact match is wanted, strcmp should be used instead.
> 
> Good catch.
> However, in this case an exact match (with the string ".bitmap") is what
> is needed, so
> 
> NAK
> 
> Will you cook up a new patch changing strncmp to strcmp?

Sure.

julia

^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: [PATCH 4/4] fs/qnx4: decrement sizeof size in strncmp
@ 2009-11-12 10:05     ` Julia Lawall
  0 siblings, 0 replies; 22+ messages in thread
From: Julia Lawall @ 2009-11-12 10:05 UTC (permalink / raw)
  To: Anders Larsen; +Cc: linux-kernel, kernel-janitors

On Thu, 12 Nov 2009, Anders Larsen wrote:

> On 2009-11-12 08:49:44, Julia Lawall wrote:
> > As observed by Joe Perches, sizeof of a constant string includes the
> > trailing 0.  If what is wanted is to check the initial characters of
> > another string, this trailing 0 should not be taken into account.  If an
> > exact match is wanted, strcmp should be used instead.
> 
> Good catch.
> However, in this case an exact match (with the string ".bitmap") is what
> is needed, so
> 
> NAK
> 
> Will you cook up a new patch changing strncmp to strcmp?

Sure.

julia

^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: [PATCH 4/4] fs/qnx4: decrement sizeof size in strncmp
  2009-11-12  9:25   ` Anders Larsen
@ 2009-11-12 10:14     ` Julia Lawall
  -1 siblings, 0 replies; 22+ messages in thread
From: Julia Lawall @ 2009-11-12 10:14 UTC (permalink / raw)
  To: Anders Larsen; +Cc: linux-kernel, kernel-janitors

From: Julia Lawall <julia@diku.dk>

As an identical match is wanted in this case, strcmp can be used instead.

The semantic match that lead to detecting this problem is as follows:
(http://coccinelle.lip6.fr/)

// <smpl>
@@
expression foo;
constant char *abc;
@@

*strncmp(foo, abc, sizeof(abc))
// </smpl>

Signed-off-by: Julia Lawall <julia@diku.dk>

---
 fs/qnx4/inode.c |    3 ++-
 1 files changed, 2 insertions(+), 1 deletions(-)

diff --git a/fs/qnx4/inode.c b/fs/qnx4/inode.c
index 449f5a6..150f4af 100644
--- a/fs/qnx4/inode.c
+++ b/fs/qnx4/inode.c
@@ -221,7 +221,8 @@ static const char *qnx4_checkroot(struct super_block *sb)
 				rootdir = (struct qnx4_inode_entry *) (bh->b_data + i * QNX4_DIR_ENTRY_SIZE);
 				if (rootdir->di_fname != NULL) {
 					QNX4DEBUG((KERN_INFO "rootdir entry found : [%s]\n", rootdir->di_fname));
-					if (!strncmp(rootdir->di_fname, QNX4_BMNAME, sizeof QNX4_BMNAME)) {
+					if (!strcmp(rootdir->di_fname,
+						    QNX4_BMNAME)) {
 						found = 1;
 						qnx4_sb(sb)->BitMap = kmalloc( sizeof( struct qnx4_inode_entry ), GFP_KERNEL );
 						if (!qnx4_sb(sb)->BitMap) {

^ permalink raw reply related	[flat|nested] 22+ messages in thread

* Re: [PATCH 4/4] fs/qnx4: decrement sizeof size in strncmp
@ 2009-11-12 10:14     ` Julia Lawall
  0 siblings, 0 replies; 22+ messages in thread
From: Julia Lawall @ 2009-11-12 10:14 UTC (permalink / raw)
  To: Anders Larsen; +Cc: linux-kernel, kernel-janitors

From: Julia Lawall <julia@diku.dk>

As an identical match is wanted in this case, strcmp can be used instead.

The semantic match that lead to detecting this problem is as follows:
(http://coccinelle.lip6.fr/)

// <smpl>
@@
expression foo;
constant char *abc;
@@

*strncmp(foo, abc, sizeof(abc))
// </smpl>

Signed-off-by: Julia Lawall <julia@diku.dk>

---
 fs/qnx4/inode.c |    3 ++-
 1 files changed, 2 insertions(+), 1 deletions(-)

diff --git a/fs/qnx4/inode.c b/fs/qnx4/inode.c
index 449f5a6..150f4af 100644
--- a/fs/qnx4/inode.c
+++ b/fs/qnx4/inode.c
@@ -221,7 +221,8 @@ static const char *qnx4_checkroot(struct super_block *sb)
 				rootdir = (struct qnx4_inode_entry *) (bh->b_data + i * QNX4_DIR_ENTRY_SIZE);
 				if (rootdir->di_fname != NULL) {
 					QNX4DEBUG((KERN_INFO "rootdir entry found : [%s]\n", rootdir->di_fname));
-					if (!strncmp(rootdir->di_fname, QNX4_BMNAME, sizeof QNX4_BMNAME)) {
+					if (!strcmp(rootdir->di_fname,
+						    QNX4_BMNAME)) {
 						found = 1;
 						qnx4_sb(sb)->BitMap = kmalloc( sizeof( struct qnx4_inode_entry ), GFP_KERNEL );
 						if (!qnx4_sb(sb)->BitMap) {

^ permalink raw reply related	[flat|nested] 22+ messages in thread

* sizeof vs strlen (was Re: [PATCH 4/4] fs/qnx4: decrement sizeof size in strncmp)
  2009-11-12  7:49 ` Julia Lawall
@ 2009-11-12 10:22   ` Bernd Petrovitsch
  -1 siblings, 0 replies; 22+ messages in thread
From: Bernd Petrovitsch @ 2009-11-12 10:22 UTC (permalink / raw)
  To: Julia Lawall; +Cc: al, linux-kernel, kernel-janitors

On Thu, 2009-11-12 at 08:49 +0100, Julia Lawall wrote:
> From: Julia Lawall <julia@diku.dk>
> 
> As observed by Joe Perches, sizeof of a constant string includes the
> trailing 0.  If what is wanted is to check the initial characters of
> another string, this trailing 0 should not be taken into account.  If an
> exact match is wanted, strcmp should be used instead.
[...]
> strncmp(foo, abc, 
> - sizeof(abc)
> + sizeof(abc)-1
>  )
> // </smpl>
Am I the only one who find "strlen()" instead of "sizeof()-1" here much
more readable (and to the point).

As for run-time, it shouldn't make a difference for static/constant
strings as gcc should be able calculate the length at compile time. And
if the string is not constant, sizeof() is probably wrong anyways.

	Bernd
-- 
Firmix Software GmbH                   http://www.firmix.at/
mobil: +43 664 4416156                 fax: +43 1 7890849-55
          Embedded Linux Development and Services



^ permalink raw reply	[flat|nested] 22+ messages in thread

* sizeof vs strlen (was Re: [PATCH 4/4] fs/qnx4: decrement sizeof
@ 2009-11-12 10:22   ` Bernd Petrovitsch
  0 siblings, 0 replies; 22+ messages in thread
From: Bernd Petrovitsch @ 2009-11-12 10:22 UTC (permalink / raw)
  To: Julia Lawall; +Cc: al, linux-kernel, kernel-janitors

On Thu, 2009-11-12 at 08:49 +0100, Julia Lawall wrote:
> From: Julia Lawall <julia@diku.dk>
> 
> As observed by Joe Perches, sizeof of a constant string includes the
> trailing 0.  If what is wanted is to check the initial characters of
> another string, this trailing 0 should not be taken into account.  If an
> exact match is wanted, strcmp should be used instead.
[...]
> strncmp(foo, abc, 
> - sizeof(abc)
> + sizeof(abc)-1
>  )
> // </smpl>
Am I the only one who find "strlen()" instead of "sizeof()-1" here much
more readable (and to the point).

As for run-time, it shouldn't make a difference for static/constant
strings as gcc should be able calculate the length at compile time. And
if the string is not constant, sizeof() is probably wrong anyways.

	Bernd
-- 
Firmix Software GmbH                   http://www.firmix.at/
mobil: +43 664 4416156                 fax: +43 1 7890849-55
          Embedded Linux Development and Services



^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: sizeof vs strlen (was Re: [PATCH 4/4] fs/qnx4: decrement sizeof size in strncmp)
  2009-11-12 10:22   ` sizeof vs strlen (was Re: [PATCH 4/4] fs/qnx4: decrement sizeof Bernd Petrovitsch
@ 2009-11-12 10:37     ` Julia Lawall
  -1 siblings, 0 replies; 22+ messages in thread
From: Julia Lawall @ 2009-11-12 10:37 UTC (permalink / raw)
  To: Bernd Petrovitsch; +Cc: al, linux-kernel, kernel-janitors

On Thu, 12 Nov 2009, Bernd Petrovitsch wrote:

> On Thu, 2009-11-12 at 08:49 +0100, Julia Lawall wrote:
> > From: Julia Lawall <julia@diku.dk>
> > 
> > As observed by Joe Perches, sizeof of a constant string includes the
> > trailing 0.  If what is wanted is to check the initial characters of
> > another string, this trailing 0 should not be taken into account.  If an
> > exact match is wanted, strcmp should be used instead.
> [...]
> > strncmp(foo, abc, 
> > - sizeof(abc)
> > + sizeof(abc)-1
> >  )
> > // </smpl>
> Am I the only one who find "strlen()" instead of "sizeof()-1" here much
> more readable (and to the point).
> 
> As for run-time, it shouldn't make a difference for static/constant
> strings as gcc should be able calculate the length at compile time. And
> if the string is not constant, sizeof() is probably wrong anyways.

The rule specifies that the string is a constant.

julia

^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: sizeof vs strlen (was Re: [PATCH 4/4] fs/qnx4: decrement sizeof
@ 2009-11-12 10:37     ` Julia Lawall
  0 siblings, 0 replies; 22+ messages in thread
From: Julia Lawall @ 2009-11-12 10:37 UTC (permalink / raw)
  To: Bernd Petrovitsch; +Cc: al, linux-kernel, kernel-janitors

On Thu, 12 Nov 2009, Bernd Petrovitsch wrote:

> On Thu, 2009-11-12 at 08:49 +0100, Julia Lawall wrote:
> > From: Julia Lawall <julia@diku.dk>
> > 
> > As observed by Joe Perches, sizeof of a constant string includes the
> > trailing 0.  If what is wanted is to check the initial characters of
> > another string, this trailing 0 should not be taken into account.  If an
> > exact match is wanted, strcmp should be used instead.
> [...]
> > strncmp(foo, abc, 
> > - sizeof(abc)
> > + sizeof(abc)-1
> >  )
> > // </smpl>
> Am I the only one who find "strlen()" instead of "sizeof()-1" here much
> more readable (and to the point).
> 
> As for run-time, it shouldn't make a difference for static/constant
> strings as gcc should be able calculate the length at compile time. And
> if the string is not constant, sizeof() is probably wrong anyways.

The rule specifies that the string is a constant.

julia

^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: sizeof vs strlen (was Re: [PATCH 4/4] fs/qnx4: decrement sizeof size in strncmp)
  2009-11-12 10:22   ` sizeof vs strlen (was Re: [PATCH 4/4] fs/qnx4: decrement sizeof Bernd Petrovitsch
@ 2009-11-12 15:33     ` Julia Lawall
  -1 siblings, 0 replies; 22+ messages in thread
From: Julia Lawall @ 2009-11-12 15:33 UTC (permalink / raw)
  To: Bernd Petrovitsch; +Cc: al, linux-kernel, kernel-janitors

On Thu, 12 Nov 2009, Bernd Petrovitsch wrote:

> On Thu, 2009-11-12 at 08:49 +0100, Julia Lawall wrote:
> > From: Julia Lawall <julia@diku.dk>
> > 
> > As observed by Joe Perches, sizeof of a constant string includes the
> > trailing 0.  If what is wanted is to check the initial characters of
> > another string, this trailing 0 should not be taken into account.  If an
> > exact match is wanted, strcmp should be used instead.
> [...]
> > strncmp(foo, abc, 
> > - sizeof(abc)
> > + sizeof(abc)-1
> >  )
> > // </smpl>
> Am I the only one who find "strlen()" instead of "sizeof()-1" here much
> more readable (and to the point).
> 
> As for run-time, it shouldn't make a difference for static/constant
> strings as gcc should be able calculate the length at compile time. And
> if the string is not constant, sizeof() is probably wrong anyways.

Does gcc have access to the definition of strlen?  It does not seem to be 
an inlined function, eg in lib/string.c.

julia

^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: sizeof vs strlen (was Re: [PATCH 4/4] fs/qnx4: decrement sizeof
@ 2009-11-12 15:33     ` Julia Lawall
  0 siblings, 0 replies; 22+ messages in thread
From: Julia Lawall @ 2009-11-12 15:33 UTC (permalink / raw)
  To: Bernd Petrovitsch; +Cc: al, linux-kernel, kernel-janitors

On Thu, 12 Nov 2009, Bernd Petrovitsch wrote:

> On Thu, 2009-11-12 at 08:49 +0100, Julia Lawall wrote:
> > From: Julia Lawall <julia@diku.dk>
> > 
> > As observed by Joe Perches, sizeof of a constant string includes the
> > trailing 0.  If what is wanted is to check the initial characters of
> > another string, this trailing 0 should not be taken into account.  If an
> > exact match is wanted, strcmp should be used instead.
> [...]
> > strncmp(foo, abc, 
> > - sizeof(abc)
> > + sizeof(abc)-1
> >  )
> > // </smpl>
> Am I the only one who find "strlen()" instead of "sizeof()-1" here much
> more readable (and to the point).
> 
> As for run-time, it shouldn't make a difference for static/constant
> strings as gcc should be able calculate the length at compile time. And
> if the string is not constant, sizeof() is probably wrong anyways.

Does gcc have access to the definition of strlen?  It does not seem to be 
an inlined function, eg in lib/string.c.

julia

^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: [PATCH 4/4] fs/qnx4: decrement sizeof size in strncmp
  2009-11-12  7:49 ` Julia Lawall
@ 2009-11-12 22:08 ` Anders Larsen
  -1 siblings, 0 replies; 22+ messages in thread
From: Anders Larsen @ 2009-11-12 22:08 UTC (permalink / raw)
  To: Julia Lawall; +Cc: linux-kernel, kernel-janitors, Andrew Morton, trivial

From: Julia Lawall <julia@diku.dk>

As an identical match is wanted in this case, strcmp can be used instead.

The semantic match that lead to detecting this problem is as follows:
(http://coccinelle.lip6.fr/)

// <smpl>
@@
expression foo;
constant char *abc;
@@

*strncmp(foo, abc, sizeof(abc))
// </smpl>

Signed-off-by: Julia Lawall <julia@diku.dk>
Signed-off-by: Anders Larsen <al@alarsen.net>

---
 fs/qnx4/inode.c |    3 ++-
 1 files changed, 2 insertions(+), 1 deletions(-)

diff --git a/fs/qnx4/inode.c b/fs/qnx4/inode.c
index 449f5a6..150f4af 100644
--- a/fs/qnx4/inode.c
+++ b/fs/qnx4/inode.c
@@ -221,7 +221,8 @@ static const char *qnx4_checkroot(struct super_block *sb)
 				rootdir = (struct qnx4_inode_entry *) (bh->b_data + i * QNX4_DIR_ENTRY_SIZE);
 				if (rootdir->di_fname != NULL) {
 					QNX4DEBUG((KERN_INFO "rootdir entry found : [%s]\n", rootdir->di_fname));
-					if (!strncmp(rootdir->di_fname, QNX4_BMNAME, sizeof QNX4_BMNAME)) {
+					if (!strcmp(rootdir->di_fname,
+						    QNX4_BMNAME)) {
 						found = 1;
 						qnx4_sb(sb)->BitMap = kmalloc( sizeof( struct qnx4_inode_entry ), GFP_KERNEL );
 						if (!qnx4_sb(sb)->BitMap) {



^ permalink raw reply related	[flat|nested] 22+ messages in thread

* Re: [PATCH 4/4] fs/qnx4: decrement sizeof size in strncmp
@ 2009-11-12 22:08 ` Anders Larsen
  0 siblings, 0 replies; 22+ messages in thread
From: Anders Larsen @ 2009-11-12 22:08 UTC (permalink / raw)
  To: Julia Lawall; +Cc: linux-kernel, kernel-janitors, Andrew Morton, trivial

From: Julia Lawall <julia@diku.dk>

As an identical match is wanted in this case, strcmp can be used instead.

The semantic match that lead to detecting this problem is as follows:
(http://coccinelle.lip6.fr/)

// <smpl>
@@
expression foo;
constant char *abc;
@@

*strncmp(foo, abc, sizeof(abc))
// </smpl>

Signed-off-by: Julia Lawall <julia@diku.dk>
Signed-off-by: Anders Larsen <al@alarsen.net>

---
 fs/qnx4/inode.c |    3 ++-
 1 files changed, 2 insertions(+), 1 deletions(-)

diff --git a/fs/qnx4/inode.c b/fs/qnx4/inode.c
index 449f5a6..150f4af 100644
--- a/fs/qnx4/inode.c
+++ b/fs/qnx4/inode.c
@@ -221,7 +221,8 @@ static const char *qnx4_checkroot(struct super_block *sb)
 				rootdir = (struct qnx4_inode_entry *) (bh->b_data + i * QNX4_DIR_ENTRY_SIZE);
 				if (rootdir->di_fname != NULL) {
 					QNX4DEBUG((KERN_INFO "rootdir entry found : [%s]\n", rootdir->di_fname));
-					if (!strncmp(rootdir->di_fname, QNX4_BMNAME, sizeof QNX4_BMNAME)) {
+					if (!strcmp(rootdir->di_fname,
+						    QNX4_BMNAME)) {
 						found = 1;
 						qnx4_sb(sb)->BitMap = kmalloc( sizeof( struct qnx4_inode_entry ), GFP_KERNEL );
 						if (!qnx4_sb(sb)->BitMap) {



^ permalink raw reply related	[flat|nested] 22+ messages in thread

* Re: [PATCH 4/4] fs/qnx4: decrement sizeof size in strncmp
  2009-11-12  7:49 ` Julia Lawall
                   ` (2 preceding siblings ...)
  (?)
@ 2009-11-12 23:49 ` David Wagner
  2009-11-13  0:06   ` Joe Perches
  -1 siblings, 1 reply; 22+ messages in thread
From: David Wagner @ 2009-11-12 23:49 UTC (permalink / raw)
  To: linux-kernel

Julia Lawall  wrote:
> As observed by Joe Perches, sizeof of a constant string includes the
> trailing 0.  If what is wanted is to check the initial characters of
> another string, this trailing 0 should not be taken into account.  If an
> exact match is wanted, strcmp should be used instead.
>
> The semantic patch that makes this change is as follows:

A caution: Your patch changes behavior.  Is there a specific reason
to believe that the change in behavior is what is desired/intended in
this context?  Lacking any analysis that indicates that the change in
behavior is desired, I'm skeptical that a behavior-changing patch should
be applied.

^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: [PATCH 4/4] fs/qnx4: decrement sizeof size in strncmp
  2009-11-12 23:49 ` [PATCH 4/4] fs/qnx4: decrement sizeof size in strncmp David Wagner
@ 2009-11-13  0:06   ` Joe Perches
  0 siblings, 0 replies; 22+ messages in thread
From: Joe Perches @ 2009-11-13  0:06 UTC (permalink / raw)
  To: David Wagner; +Cc: linux-kernel

On Thu, 2009-11-12 at 23:49 +0000, David Wagner wrote:
> Julia Lawall  wrote:
> > As observed by Joe Perches, sizeof of a constant string includes the
> > trailing 0.  If what is wanted is to check the initial characters of
> > another string, this trailing 0 should not be taken into account.  If an
> > exact match is wanted, strcmp should be used instead.
> A caution: Your patch changes behavior.  Is there a specific reason
> to believe that the change in behavior is what is desired/intended in
> this context?  Lacking any analysis that indicates that the change in
> behavior is desired, I'm skeptical that a behavior-changing patch should
> be applied.

I agree that desired behavior should be known before
patches are applied.

Most likely, this instance should just be strcmp.

That gives better readability, trivially improved
performance, and easier verification.


^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: sizeof vs strlen (was Re: [PATCH 4/4] fs/qnx4: decrement sizeof size in strncmp)
  2009-11-12 15:33     ` sizeof vs strlen (was Re: [PATCH 4/4] fs/qnx4: decrement sizeof Julia Lawall
@ 2009-11-13 16:42       ` Bernd Petrovitsch
  -1 siblings, 0 replies; 22+ messages in thread
From: Bernd Petrovitsch @ 2009-11-13 16:42 UTC (permalink / raw)
  To: Julia Lawall; +Cc: al, linux-kernel, kernel-janitors

On Thu, 2009-11-12 at 16:33 +0100, Julia Lawall wrote: 
> On Thu, 12 Nov 2009, Bernd Petrovitsch wrote:
> 
> > On Thu, 2009-11-12 at 08:49 +0100, Julia Lawall wrote:
> > > From: Julia Lawall <julia@diku.dk>
> > > 
> > > As observed by Joe Perches, sizeof of a constant string includes the
> > > trailing 0.  If what is wanted is to check the initial characters of
> > > another string, this trailing 0 should not be taken into account.  If an
> > > exact match is wanted, strcmp should be used instead.
> > [...]
> > > strncmp(foo, abc, 
> > > - sizeof(abc)
> > > + sizeof(abc)-1
> > >  )
> > > // </smpl>
> > Am I the only one who find "strlen()" instead of "sizeof()-1" here much
> > more readable (and to the point).
> > 
> > As for run-time, it shouldn't make a difference for static/constant
> > strings as gcc should be able calculate the length at compile time. And
> > if the string is not constant, sizeof() is probably wrong anyways.
> 
> Does gcc have access to the definition of strlen?  It does not seem to be 
> an inlined function, eg in lib/string.c.
Since "strlen()" is defined in the C-Standard C-compilers could rely on
the defined behaviour (but I don't know exactly how gcc behaves with
-ffreestanding for all supported versions).
Then there is __builin_strlen() (see also
http://gcc.gnu.org/onlinedocs/gcc-4.4.2/gcc/Other-Builtins.html#Other-Builtins).

Stepping a quite small abstraction layer higher:
include/linux/string.h has at teh end:
----  snip  ----
static inline bool strstarts(const char *str, const char *prefix)
{
	return strncmp(str, prefix, strlen(prefix)) == 0;
}
---- snip  ----
seems to be what most uses of strnmcp() actually are: check if one
string is a prefix of another.

	Bernd
-- 
Firmix Software GmbH                   http://www.firmix.at/
mobil: +43 664 4416156                 fax: +43 1 7890849-55
          Embedded Linux Development and Services



^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: sizeof vs strlen (was Re: [PATCH 4/4] fs/qnx4: decrement
@ 2009-11-13 16:42       ` Bernd Petrovitsch
  0 siblings, 0 replies; 22+ messages in thread
From: Bernd Petrovitsch @ 2009-11-13 16:42 UTC (permalink / raw)
  To: Julia Lawall; +Cc: al, linux-kernel, kernel-janitors

On Thu, 2009-11-12 at 16:33 +0100, Julia Lawall wrote: 
> On Thu, 12 Nov 2009, Bernd Petrovitsch wrote:
> 
> > On Thu, 2009-11-12 at 08:49 +0100, Julia Lawall wrote:
> > > From: Julia Lawall <julia@diku.dk>
> > > 
> > > As observed by Joe Perches, sizeof of a constant string includes the
> > > trailing 0.  If what is wanted is to check the initial characters of
> > > another string, this trailing 0 should not be taken into account.  If an
> > > exact match is wanted, strcmp should be used instead.
> > [...]
> > > strncmp(foo, abc, 
> > > - sizeof(abc)
> > > + sizeof(abc)-1
> > >  )
> > > // </smpl>
> > Am I the only one who find "strlen()" instead of "sizeof()-1" here much
> > more readable (and to the point).
> > 
> > As for run-time, it shouldn't make a difference for static/constant
> > strings as gcc should be able calculate the length at compile time. And
> > if the string is not constant, sizeof() is probably wrong anyways.
> 
> Does gcc have access to the definition of strlen?  It does not seem to be 
> an inlined function, eg in lib/string.c.
Since "strlen()" is defined in the C-Standard C-compilers could rely on
the defined behaviour (but I don't know exactly how gcc behaves with
-ffreestanding for all supported versions).
Then there is __builin_strlen() (see also
http://gcc.gnu.org/onlinedocs/gcc-4.4.2/gcc/Other-Builtins.html#Other-Builtins).

Stepping a quite small abstraction layer higher:
include/linux/string.h has at teh end:
----  snip  ----
static inline bool strstarts(const char *str, const char *prefix)
{
	return strncmp(str, prefix, strlen(prefix)) = 0;
}
---- snip  ----
seems to be what most uses of strnmcp() actually are: check if one
string is a prefix of another.

	Bernd
-- 
Firmix Software GmbH                   http://www.firmix.at/
mobil: +43 664 4416156                 fax: +43 1 7890849-55
          Embedded Linux Development and Services



^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: [PATCH 4/4] fs/qnx4: decrement sizeof size in strncmp
  2009-11-12 22:08 ` Anders Larsen
@ 2009-12-18 14:26   ` Jiri Kosina
  -1 siblings, 0 replies; 22+ messages in thread
From: Jiri Kosina @ 2009-12-18 14:26 UTC (permalink / raw)
  To: Anders Larsen; +Cc: Julia Lawall, linux-kernel, kernel-janitors, Andrew Morton

On Thu, 12 Nov 2009, Anders Larsen wrote:

> From: Julia Lawall <julia@diku.dk>
> 
> As an identical match is wanted in this case, strcmp can be used instead.
> 
> The semantic match that lead to detecting this problem is as follows:
> (http://coccinelle.lip6.fr/)
> 
> // <smpl>
> @@
> expression foo;
> constant char *abc;
> @@
> 
> *strncmp(foo, abc, sizeof(abc))
> // </smpl>
> 
> Signed-off-by: Julia Lawall <julia@diku.dk>
> Signed-off-by: Anders Larsen <al@alarsen.net>
> 
> ---
>  fs/qnx4/inode.c |    3 ++-
>  1 files changed, 2 insertions(+), 1 deletions(-)
> 
> diff --git a/fs/qnx4/inode.c b/fs/qnx4/inode.c
> index 449f5a6..150f4af 100644
> --- a/fs/qnx4/inode.c
> +++ b/fs/qnx4/inode.c
> @@ -221,7 +221,8 @@ static const char *qnx4_checkroot(struct super_block *sb)
>  				rootdir = (struct qnx4_inode_entry *) (bh->b_data + i * QNX4_DIR_ENTRY_SIZE);
>  				if (rootdir->di_fname != NULL) {
>  					QNX4DEBUG((KERN_INFO "rootdir entry found : [%s]\n", rootdir->di_fname));
> -					if (!strncmp(rootdir->di_fname, QNX4_BMNAME, sizeof QNX4_BMNAME)) {
> +					if (!strcmp(rootdir->di_fname,
> +						    QNX4_BMNAME)) {
>  						found = 1;
>  						qnx4_sb(sb)->BitMap = kmalloc( sizeof( struct qnx4_inode_entry ), GFP_KERNEL );
>  						if (!qnx4_sb(sb)->BitMap) {

Doesn't seem to be present in 2.6.33-rc1.

Applied to my queue, thanks.

-- 
Jiri Kosina
SUSE Labs, Novell Inc.

^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: [PATCH 4/4] fs/qnx4: decrement sizeof size in strncmp
@ 2009-12-18 14:26   ` Jiri Kosina
  0 siblings, 0 replies; 22+ messages in thread
From: Jiri Kosina @ 2009-12-18 14:26 UTC (permalink / raw)
  To: Anders Larsen; +Cc: Julia Lawall, linux-kernel, kernel-janitors, Andrew Morton

On Thu, 12 Nov 2009, Anders Larsen wrote:

> From: Julia Lawall <julia@diku.dk>
> 
> As an identical match is wanted in this case, strcmp can be used instead.
> 
> The semantic match that lead to detecting this problem is as follows:
> (http://coccinelle.lip6.fr/)
> 
> // <smpl>
> @@
> expression foo;
> constant char *abc;
> @@
> 
> *strncmp(foo, abc, sizeof(abc))
> // </smpl>
> 
> Signed-off-by: Julia Lawall <julia@diku.dk>
> Signed-off-by: Anders Larsen <al@alarsen.net>
> 
> ---
>  fs/qnx4/inode.c |    3 ++-
>  1 files changed, 2 insertions(+), 1 deletions(-)
> 
> diff --git a/fs/qnx4/inode.c b/fs/qnx4/inode.c
> index 449f5a6..150f4af 100644
> --- a/fs/qnx4/inode.c
> +++ b/fs/qnx4/inode.c
> @@ -221,7 +221,8 @@ static const char *qnx4_checkroot(struct super_block *sb)
>  				rootdir = (struct qnx4_inode_entry *) (bh->b_data + i * QNX4_DIR_ENTRY_SIZE);
>  				if (rootdir->di_fname != NULL) {
>  					QNX4DEBUG((KERN_INFO "rootdir entry found : [%s]\n", rootdir->di_fname));
> -					if (!strncmp(rootdir->di_fname, QNX4_BMNAME, sizeof QNX4_BMNAME)) {
> +					if (!strcmp(rootdir->di_fname,
> +						    QNX4_BMNAME)) {
>  						found = 1;
>  						qnx4_sb(sb)->BitMap = kmalloc( sizeof( struct qnx4_inode_entry ), GFP_KERNEL );
>  						if (!qnx4_sb(sb)->BitMap) {

Doesn't seem to be present in 2.6.33-rc1.

Applied to my queue, thanks.

-- 
Jiri Kosina
SUSE Labs, Novell Inc.

^ permalink raw reply	[flat|nested] 22+ messages in thread

end of thread, other threads:[~2009-12-18 14:26 UTC | newest]

Thread overview: 22+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2009-11-12  7:49 [PATCH 4/4] fs/qnx4: decrement sizeof size in strncmp Julia Lawall
2009-11-12  7:49 ` Julia Lawall
2009-11-12  9:25 ` Anders Larsen
2009-11-12  9:25   ` Anders Larsen
2009-11-12 10:05   ` Julia Lawall
2009-11-12 10:05     ` Julia Lawall
2009-11-12 10:14   ` Julia Lawall
2009-11-12 10:14     ` Julia Lawall
2009-11-12 10:22 ` sizeof vs strlen (was Re: [PATCH 4/4] fs/qnx4: decrement sizeof size in strncmp) Bernd Petrovitsch
2009-11-12 10:22   ` sizeof vs strlen (was Re: [PATCH 4/4] fs/qnx4: decrement sizeof Bernd Petrovitsch
2009-11-12 10:37   ` sizeof vs strlen (was Re: [PATCH 4/4] fs/qnx4: decrement sizeof size in strncmp) Julia Lawall
2009-11-12 10:37     ` sizeof vs strlen (was Re: [PATCH 4/4] fs/qnx4: decrement sizeof Julia Lawall
2009-11-12 15:33   ` sizeof vs strlen (was Re: [PATCH 4/4] fs/qnx4: decrement sizeof size in strncmp) Julia Lawall
2009-11-12 15:33     ` sizeof vs strlen (was Re: [PATCH 4/4] fs/qnx4: decrement sizeof Julia Lawall
2009-11-13 16:42     ` sizeof vs strlen (was Re: [PATCH 4/4] fs/qnx4: decrement sizeof size in strncmp) Bernd Petrovitsch
2009-11-13 16:42       ` sizeof vs strlen (was Re: [PATCH 4/4] fs/qnx4: decrement Bernd Petrovitsch
2009-11-12 23:49 ` [PATCH 4/4] fs/qnx4: decrement sizeof size in strncmp David Wagner
2009-11-13  0:06   ` Joe Perches
2009-11-12 22:08 Anders Larsen
2009-11-12 22:08 ` Anders Larsen
2009-12-18 14:26 ` Jiri Kosina
2009-12-18 14:26   ` Jiri Kosina

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.