ipc/shm.c:494:18: warning: unused variable ‘hs’ [-Wunused-variable]

PATCH In review Borislav Petkov 3 participants 6 messages May 8, 2013 – May 18, 2013 Memory management

lore

Share & tools

lore

Borislav Petkov

author

2013-05-08 14:33 UTC #0 lore raw

Hi,

looks like this warning came in with af73e4d9506d3b797509f3c030e7dcd554f7d9c4 and fires in the

# CONFIG_HUGETLB_PAGE is not set

case because

	struct hstate *hs = hstate_sizelog((shmflg >> SHM_HUGE_SHIFT)
                                        & SHM_HUGE_MASK);

becomes

	struct hstate *hs = ((void *)0)

due to

#define hstate_sizelog(s) NULL

Hmmm, I don't have a quick solution off the top of my head from staring at the code.

Signature · 5 lines

Regards/Gruss,
    Boris.

Sent from a fat crate under my desk. Formatting is fine.
--

Naoya Horiguchi

2013-05-08 16:12 UTC #1 lore raw

Hi Borislav,

Quoted 14 lines · Borislav Petkov

Hi,
looks like this warning came in with af73e4d9506d3b797509f3c030e7dcd554f7d9c4 and fires in the
# CONFIG_HUGETLB_PAGE is not set
case because
	struct hstate *hs = hstate_sizelog((shmflg >> SHM_HUGE_SHIFT)
                                        & SHM_HUGE_MASK);
becomes

Quoted 8 lines

	struct hstate *hs = ((void *)0)
due to
#define hstate_sizelog(s) NULL
Hmmm, I don't have a quick solution off the top of my head from staring at the code.

Thank you for the report.
I believe we can fix it with this one.
---
From: Naoya Horiguchi <[email protected]>
Date: Wed, 8 May 2013 11:48:01 -0400
Subject: [PATCH] ipc/shm.c: don't use auto variable hs in newseg()

This patch fixes "warning: unused variable 'hs'" when !CONFIG_HUGETLB_PAGE introduced by commit af73e4d9506d "hugetlbfs: fix mmap failure in unaligned size request".

Reported-by Borislav Petkov <[email protected]>

Signed-off-by Naoya Horiguchi <[email protected]>

---
 ipc/shm.c | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

ipc/shm.c +2 -3

index e316cb9..9ff741a 100644
@@ -491,9 +491,8 @@ static int newseg(struct ipc_namespace *ns, struct ipc_params *params)
 
 	sprintf (name, "SYSV%08x", key);
 	if (shmflg & SHM_HUGETLB) {
-		struct hstate *hs = hstate_sizelog((shmflg >> SHM_HUGE_SHIFT)
-						& SHM_HUGE_MASK);
-		size_t hugesize = ALIGN(size, huge_page_size(hs));
+		size_t hugesize = ALIGN(size, huge_page_size(hstate_sizelog(
+				(shmflg >> SHM_HUGE_SHIFT) & SHM_HUGE_MASK)));
 
 		/* hugetlb_file_setup applies strict accounting */
 		if (shmflg & SHM_NORESERVE)

Signature · 1 line

1.7.11.7

Borislav Petkov

author

2013-05-08 18:43 UTC #2 lore raw

Quoted 22 lines · Naoya Horiguchi

Thank you for the report.
I believe we can fix it with this one.
---
From: Naoya Horiguchi <[email protected]>
Date: Wed, 8 May 2013 11:48:01 -0400
Subject: [PATCH] ipc/shm.c: don't use auto variable hs in newseg()
This patch fixes "warning: unused variable 'hs'" when !CONFIG_HUGETLB_PAGE introduced by commit af73e4d9506d "hugetlbfs: fix mmap failure in unaligned size request".
Reported-by Borislav Petkov <[email protected]>
Signed-off-by Naoya Horiguchi <[email protected]>
---
 ipc/shm.c | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)
ipc/shm.c +0 -0
index e316cb9..9ff741a 100644

ipc/shm.c static int newseg(struct ipc_namespace *ns, struct ipc_params *params) · line 495

Quoted 8 lines

ipc/shm.c +2 -3

@@ -491,9 +491,8 @@ static int newseg(struct ipc_namespace *ns, struct ipc_params *params)
 
 	sprintf (name, "SYSV%08x", key);
 	if (shmflg & SHM_HUGETLB) {
-		struct hstate *hs = hstate_sizelog((shmflg >> SHM_HUGE_SHIFT)
-						& SHM_HUGE_MASK);
-		size_t hugesize = ALIGN(size, huge_page_size(hs));
+		size_t hugesize = ALIGN(size, huge_page_size(hstate_sizelog(
+				(shmflg >> SHM_HUGE_SHIFT) & SHM_HUGE_MASK)));

Yeah, it fixes the warning alright but makes the code more unreadable. Which makes me wonder which is worse - to have an innocuous warning or have unreadable code.

You could also do the below. The line sticks out but it kills the warning. Readability is hmm, not optimal still though. :)

ipc/shm.c +2 -3

@@ -491,9 +491,8 @@ static int newseg(struct ipc_namespace *ns, struct ipc_params *params)
 
 	sprintf (name, "SYSV%08x", key);
 	if (shmflg & SHM_HUGETLB) {
-		struct hstate *hs = hstate_sizelog((shmflg >> SHM_HUGE_SHIFT)
-						& SHM_HUGE_MASK);
-		size_t hugesize = ALIGN(size, huge_page_size(hs));
+		unsigned long hsz = huge_page_size(hstate_sizelog((shmflg >> SHM_HUGE_SHIFT) & SHM_HUGE_MASK));
+		size_t hugesize = ALIGN(size, hsz);
 
 		/* hugetlb_file_setup applies strict accounting */
 		if (shmflg & SHM_NORESERVE)

Yeah, you decide.

Thanks.

Signature · 5 lines

Regards/Gruss,
    Boris.

Sent from a fat crate under my desk. Formatting is fine.
--

Naoya Horiguchi

2013-05-08 20:48 UTC #3 lore raw

PATCH 1/1 ipc/shm.c: don't use auto variable hs in newseg()

Quoted 56 lines · Borislav Petkov

Quoted 22 lines · Naoya Horiguchi
Thank you for the report.
I believe we can fix it with this one.
---
From: Naoya Horiguchi <[email protected]>
Date: Wed, 8 May 2013 11:48:01 -0400
Subject: [PATCH] ipc/shm.c: don't use auto variable hs in newseg()
This patch fixes "warning: unused variable 'hs'" when !CONFIG_HUGETLB_PAGE introduced by commit af73e4d9506d "hugetlbfs: fix mmap failure in unaligned size request".
Reported-by Borislav Petkov <[email protected]>
Signed-off-by Naoya Horiguchi <[email protected]>
---
 ipc/shm.c | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)
ipc/shm.c +0 -0
index e316cb9..9ff741a 100644
ipc/shm.c static int newseg(struct ipc_namespace *ns, struct ipc_params *params) · line 495
Quoted 8 lines
ipc/shm.c +2 -3
@@ -491,9 +491,8 @@ static int newseg(struct ipc_namespace *ns, struct ipc_params *params)
 
 	sprintf (name, "SYSV%08x", key);
 	if (shmflg & SHM_HUGETLB) {
-		struct hstate *hs = hstate_sizelog((shmflg >> SHM_HUGE_SHIFT)
-						& SHM_HUGE_MASK);
-		size_t hugesize = ALIGN(size, huge_page_size(hs));
+		size_t hugesize = ALIGN(size, huge_page_size(hstate_sizelog(
+				(shmflg >> SHM_HUGE_SHIFT) & SHM_HUGE_MASK)));
Yeah, it fixes the warning alright but makes the code more unreadable. Which makes me wonder which is worse - to have an innocuous warning or have unreadable code.
You could also do the below. The line sticks out but it kills the warning. Readability is hmm, not optimal still though. :) --
ipc/shm.c +2 -4
@@ -491,9 +491,8 @@ static int newseg(struct ipc_namespace *ns, struct ipc_params *params)
 
 	sprintf (name, "SYSV%08x", key);
 	if (shmflg & SHM_HUGETLB) {
-		struct hstate *hs = hstate_sizelog((shmflg >> SHM_HUGE_SHIFT)
-						& SHM_HUGE_MASK);
-		size_t hugesize = ALIGN(size, huge_page_size(hs));
+		unsigned long hsz = huge_page_size(hstate_sizelog((shmflg >> SHM_HUGE_SHIFT) & SHM_HUGE_MASK));
+		size_t hugesize = ALIGN(size, hsz);
 
 		/* hugetlb_file_setup applies strict accounting */
 		if (shmflg & SHM_NORESERVE)
--

Quoted 1 line

Yeah, you decide.

(CCed: Andrew and linux-mm)
OK, personally both are OK (comparably bad) for me, so I'll do the same as af73e4d9506d3b does for SYSCALL_DEFINE6(mmap_pgoff), where we have a line break just after '('.

Andrew, could you pick up the following fix?
-----
From: Naoya Horiguchi <[email protected]>
Date: Wed, 8 May 2013 11:48:01 -0400
Subject: [PATCH] ipc/shm.c: don't use auto variable hs in newseg()

This patch fixes "warning: unused variable 'hs'" when !CONFIG_HUGETLB_PAGE introduced by commit af73e4d9506d "hugetlbfs: fix mmap failure in unaligned size request".

Reported-by Borislav Petkov <[email protected]>

Signed-off-by Naoya Horiguchi <[email protected]>

---
 ipc/shm.c | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

ipc/shm.c +2 -3

index e316cb9..9ff741a 100644
@@ -491,9 +491,8 @@ static int newseg(struct ipc_namespace *ns, struct ipc_params *params)
 
 	sprintf (name, "SYSV%08x", key);
 	if (shmflg & SHM_HUGETLB) {
-		struct hstate *hs = hstate_sizelog((shmflg >> SHM_HUGE_SHIFT)
-						& SHM_HUGE_MASK);
-		size_t hugesize = ALIGN(size, huge_page_size(hs));
+		size_t hugesize = ALIGN(size, huge_page_size(hstate_sizelog(
+				(shmflg >> SHM_HUGE_SHIFT) & SHM_HUGE_MASK)));
 
 		/* hugetlb_file_setup applies strict accounting */
 		if (shmflg & SHM_NORESERVE)

Signature · 7 lines

1.7.11.7

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to [email protected].  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"[email protected]"> [email protected] </a>

KOSAKI Motohiro

2013-05-17 20:31 UTC #4 lore raw

Re: [PATCH] ipc/shm.c: don't use auto variable hs in newseg()

Quoted 19 lines

From: Naoya Horiguchi <[email protected]>
Date: Wed, 8 May 2013 11:48:01 -0400
Subject: [PATCH] ipc/shm.c: don't use auto variable hs in newseg()
This patch fixes "warning: unused variable 'hs'" when !CONFIG_HUGETLB_PAGE introduced by commit af73e4d9506d "hugetlbfs: fix mmap failure in unaligned size request".
Reported-by Borislav Petkov <[email protected]>
Signed-off-by Naoya Horiguchi <[email protected]>
---
 ipc/shm.c | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)
ipc/shm.c +0 -0
index e316cb9..9ff741a 100644

ipc/shm.c static int newseg(struct ipc_namespace *ns, struct ipc_params *params) · line 495

Quoted 8 lines

ipc/shm.c +2 -3

@@ -491,9 +491,8 @@ static int newseg(struct ipc_namespace *ns, struct ipc_params *params)
 
 	sprintf (name, "SYSV%08x", key);
 	if (shmflg & SHM_HUGETLB) {
-		struct hstate *hs = hstate_sizelog((shmflg >> SHM_HUGE_SHIFT)
-						& SHM_HUGE_MASK);
-		size_t hugesize = ALIGN(size, huge_page_size(hs));
+		size_t hugesize = ALIGN(size, huge_page_size(hstate_sizelog(
+				(shmflg >> SHM_HUGE_SHIFT) & SHM_HUGE_MASK)));

NAK. This is uglier than before.
You should change !CONFIG_HUGETLB_PAGE specific code instead.

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in the body to [email protected]. For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"[email protected]"> [email protected] </a>

Naoya Horiguchi

2013-05-18 01:47 UTC #5 lore raw

Re: [PATCH] ipc/shm.c: don't use auto variable hs in newseg()

Quoted 20 lines · KOSAKI Motohiro

Quoted 20 lines
From: Naoya Horiguchi <[email protected]>
Date: Wed, 8 May 2013 11:48:01 -0400
Subject: [PATCH] ipc/shm.c: don't use auto variable hs in newseg()
This patch fixes "warning: unused variable 'hs'" when !CONFIG_HUGETLB_PAGE introduced by commit af73e4d9506d "hugetlbfs: fix mmap failure in unaligned size request".
Reported-by Borislav Petkov <[email protected]>
Signed-off-by Naoya Horiguchi <[email protected]>
---
 ipc/shm.c | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)
ipc/shm.c +0 -0
index e316cb9..9ff741a 100644
@@ -491,9 +491,8 @@ static int newseg(struct ipc_namespace *ns, struct ipc_params *params)
 

Quoted 10 lines

ipc/shm.c static int newseg(struct ipc_namespace *ns, struct ipc_params *params) · line 495
Quoted 7 lines
ipc/shm.c +2 -3
 	sprintf (name, "SYSV%08x", key);
 	if (shmflg & SHM_HUGETLB) {
-		struct hstate *hs = hstate_sizelog((shmflg >> SHM_HUGE_SHIFT)
-						& SHM_HUGE_MASK);
-		size_t hugesize = ALIGN(size, huge_page_size(hs));
+		size_t hugesize = ALIGN(size, huge_page_size(hstate_sizelog(
+				(shmflg >> SHM_HUGE_SHIFT) & SHM_HUGE_MASK)));
NAK. This is uglier than before.
You should change !CONFIG_HUGETLB_PAGE specific code instead.

This patch was dropped and replaced with Li Zefan's one which is available in current upstream.

commit 091d0d55b286c9340201b4ed4470be87fc568228
Author: Li Zefan <[email protected]>
Date: Thu May 9 15:08:15 2013 +0800

Thanks,
Naoya Horiguchi