replace magic numbers with enum (Re: Warning in kprobes.c)

PATCH New Masami Hiramatsu 1 participant 1 message Jan 25, 2007 3 messages not in the archive kprobes

lore

Share & tools

lore

A message that is not in the archive (sent off-list or not yet received): [email protected]

Masami Hiramatsu

author

2007-01-25 01:03 UTC #3 lore raw

PATCH 1/1 replace magic numbers with enum (Re: Warning in kprobes.c)

Ananth N Mavinakayanahalli wrote:
[snip]

Quoted 18 lines

Quoted 18 lines
Quoted 6 lines
I get this warning:
kernel/kprobes.c: In function ‘collect_garbage_slots’:
kernel/kprobes.c:215: warning: comparison is always false due to limited range of data type
Quoted 8 lines
when building 2.6.20-rc5 on powerpc. Not strange though, since the powerpc compiler is more unforgiving than most other arch compilers. The line in question is:
                        if (kip->slot_used[i] == -1 &&
and kip->slot_used is a char array and -1 is apparently an invalid char assignment. Is there a way to get rid of the warning?
As far as I searched, on powerpc and arm, the default type of char is 'unsigned char' whereas that is 'signed' on most other arch. So, your compiler warned "due to limited range of data type".

Quoted 7 lines

Quoted 2 lines
I think a good solution is that we just replace -1 with 2. Could you test the patch attached to this mail?
Thanks for taking care of this, the patch fixes the warning.
I think however, instead of having magic numbers 0, 1, 2, why not an enum? It makes the code easier to read. Comments?

It's a good idea. Same thing was pointed out by my boss too. Here is a patch which uses an enum to express the slot status.

Thanks,

Signed-off-by Masami Hiramatsu <[email protected]>

---
This patch replaces the magic numbers with an enum, and gets rid of a warning on the specific architectures (ex. powerpc) on which the compiler considers 'char' as 'unsigned char'.

 kernel/kprobes.c |   20 +++++++++++++-------
 1 file changed, 13 insertions(+), 7 deletions(-)

kernel/kprobes.c +13 -7

Index: linux-2.6.20-rc5/kernel/kprobes.c
===================================================================
@@ -87,6 +87,12 @@ struct kprobe_insn_page {
 	int ngarbage;
 };
 
+enum kprobe_slot_state {
+	SLOT_CLEAN = 0,
+	SLOT_DIRTY = 1,
+	SLOT_USED = 2,
+};
+
 static struct hlist_head kprobe_insn_pages;
 static int kprobe_garbage_slots;
 static int collect_garbage_slots(void);

@@ -130,8 +136,8 @@ kprobe_opcode_t __kprobes *get_insn_slot
 		if (kip->nused < INSNS_PER_PAGE) {
 			int i;
 			for (i = 0; i < INSNS_PER_PAGE; i++) {
-				if (!kip->slot_used[i]) {
-					kip->slot_used[i] = 1;
+				if (kip->slot_used[i] == SLOT_CLEAN) {
+					kip->slot_used[i] = SLOT_USED;
 					kip->nused++;
 					return kip->insns + (i * MAX_INSN_SIZE);
 				}

@@ -163,8 +169,8 @@ kprobe_opcode_t __kprobes *get_insn_slot
 	}
 	INIT_HLIST_NODE(&kip->hlist);
 	hlist_add_head(&kip->hlist, &kprobe_insn_pages);
-	memset(kip->slot_used, 0, INSNS_PER_PAGE);
-	kip->slot_used[0] = 1;
+	memset(kip->slot_used, SLOT_CLEAN, INSNS_PER_PAGE);
+	kip->slot_used[0] = SLOT_USED;
 	kip->nused = 1;
 	kip->ngarbage = 0;
 	return kip->insns;

@@ -173,7 +179,7 @@ kprobe_opcode_t __kprobes *get_insn_slot
 /* Return 1 if all garbages are collected, otherwise 0. */
 static int __kprobes collect_one_slot(struct kprobe_insn_page *kip, int idx)
 {
-	kip->slot_used[idx] = 0;
+	kip->slot_used[idx] = SLOT_CLEAN;
 	kip->nused--;
 	if (kip->nused == 0) {
 		/*

@@ -212,7 +218,7 @@ static int __kprobes collect_garbage_slo
 			continue;
 		kip->ngarbage = 0;	/* we will collect all garbages */
 		for (i = 0; i < INSNS_PER_PAGE; i++) {
-			if (kip->slot_used[i] == -1 &&
+			if (kip->slot_used[i] == SLOT_DIRTY &&
 			    collect_one_slot(kip, i))
 				break;
 		}

@@ -232,7 +238,7 @@ void __kprobes free_insn_slot(kprobe_opc
 		    slot < kip->insns + (INSNS_PER_PAGE * MAX_INSN_SIZE)) {
 			int i = (slot - kip->insns) / MAX_INSN_SIZE;
 			if (dirty) {
-				kip->slot_used[i] = -1;
+				kip->slot_used[i] = SLOT_DIRTY;
 				kip->ngarbage++;
 			} else {
 				collect_one_slot(kip, i);