Use NULL instead of integer 0 in security/selinux/

Fixup another round of sparse warnings of the type:
warning: Using plain integer as NULL pointer
Acked by Stephen. Rediffed against bk-current.

From: Mika Kukkonen <[email protected]>

===== security/selinux/avc.c 1.13 vs edited =====

@@ -106,7 +106,7 @@
  */
 void avc_dump_av(struct audit_buffer *ab, u16 tclass, u32 av)
 {
-	char **common_pts = 0;
+	char **common_pts = NULL;
 	u32 common_base = 0;
 	int i, i2, perm;
 

@@ -734,7 +734,7 @@
 		}
 	} else {
 		/* apply to one node */
-		node = avc_search_node(ssid, tsid, tclass, 0);
+		node = avc_search_node(ssid, tsid, tclass, NULL);
 		if (node) {
 			avc_update_node(event,node,perms);
 		}

@@ -808,7 +808,7 @@
                  u32 perms, u32 seqno)
 {
 	return avc_control(AVC_CALLBACK_GRANT,
-			   ssid, tsid, tclass, perms, seqno, 0);
+			   ssid, tsid, tclass, perms, seqno, NULL);
 }
 
 /**

@@ -846,7 +846,7 @@
                   u32 perms, u32 seqno)
 {
 	return avc_control(AVC_CALLBACK_REVOKE,
-			   ssid, tsid, tclass, perms, seqno, 0);
+			   ssid, tsid, tclass, perms, seqno, NULL);
 }
 
 /**

@@ -878,7 +878,7 @@
 			avc_node_freelist = tmp;
 			avc_cache.active_nodes--;
 		}
-		avc_cache.slots[i] = 0;
+		avc_cache.slots[i] = NULL;
 	}
 	avc_cache.lru_hint = 0;
 

@@ -890,7 +890,7 @@
 	for (c = avc_callbacks; c; c = c->next) {
 		if (c->events & AVC_CALLBACK_RESET) {
 			rc = c->callback(AVC_CALLBACK_RESET,
-					 0, 0, 0, 0, 0);
+					 0, 0, 0, 0, NULL);
 			if (rc)
 				goto out;
 		}

@@ -918,10 +918,10 @@
 {
 	if (enable)
 		return avc_control(AVC_CALLBACK_AUDITALLOW_ENABLE,
-				   ssid, tsid, tclass, perms, seqno, 0);
+				   ssid, tsid, tclass, perms, seqno, NULL);
 	else
 		return avc_control(AVC_CALLBACK_AUDITALLOW_DISABLE,
-				   ssid, tsid, tclass, perms, seqno, 0);
+				   ssid, tsid, tclass, perms, seqno, NULL);
 }
 
 /**

@@ -938,10 +938,10 @@
 {
 	if (enable)
 		return avc_control(AVC_CALLBACK_AUDITDENY_ENABLE,
-				   ssid, tsid, tclass, perms, seqno, 0);
+				   ssid, tsid, tclass, perms, seqno, NULL);
 	else
 		return avc_control(AVC_CALLBACK_AUDITDENY_DISABLE,
-				   ssid, tsid, tclass, perms, seqno, 0);
+				   ssid, tsid, tclass, perms, seqno, NULL);
 }
 
 /**

@@ -993,7 +993,7 @@
 			ae->used = 1;
 		} else {
 			avc_cache_stats_incr(AVC_ENTRY_DISCARDS);
-			ae = 0;
+			ae = NULL;
 		}
 	}
 

===== security/selinux/ss/conditional.c 1.1 vs edited =====

@@ -217,7 +217,7 @@
 
 int cond_read_bool(struct policydb *p, struct hashtab *h, void *fp)
 {
-	char *key = 0;
+	char *key = NULL;
 	struct cond_bool_datum *booldatum;
 	__u32 *buf, len;
 

@@ -251,7 +251,7 @@
 
 	return 0;
 err:
-	cond_destroy_bool(key, booldatum, 0);
+	cond_destroy_bool(key, booldatum, NULL);
 	return -1;
 }
 

===== security/selinux/ss/ebitmap.c 1.3 vs edited =====

@@ -17,7 +17,7 @@
 
 	n1 = e1->node;
 	n2 = e2->node;
-	prev = 0;
+	prev = NULL;
 	while (n1 || n2) {
 		new = kmalloc(sizeof(*new), GFP_ATOMIC);
 		if (!new) {

@@ -40,7 +40,7 @@
 			n2 = n2->next;
 		}
 
-		new->next = 0;
+		new->next = NULL;
 		if (prev)
 			prev->next = new;
 		else

@@ -80,7 +80,7 @@
 
 	ebitmap_init(dst);
 	n = src->node;
-	prev = 0;
+	prev = NULL;
 	while (n) {
 		new = kmalloc(sizeof(*new), GFP_ATOMIC);
 		if (!new) {

@@ -90,7 +90,7 @@
 		memset(new, 0, sizeof(*new));
 		new->startbit = n->startbit;
 		new->map = n->map;
-		new->next = 0;
+		new->next = NULL;
 		if (prev)
 			prev->next = new;
 		else

@@ -155,7 +155,7 @@
 {
 	struct ebitmap_node *n, *prev, *new;
 
-	prev = 0;
+	prev = NULL;
 	n = e->node;
 	while (n && n->startbit <= bit) {
 		if ((n->startbit + MAPSIZE) > bit) {

@@ -231,7 +231,7 @@
 	}
 
 	e->highbit = 0;
-	e->node = 0;
+	e->node = NULL;
 	return;
 }
 

===== security/selinux/ss/mls.c 1.4 vs edited =====

@@ -654,7 +654,7 @@
 
 int sens_read(struct policydb *p, struct hashtab *h, void *fp)
 {
-	char *key = 0;
+	char *key = NULL;
 	struct level_datum *levdatum;
 	int rc;
 	u32 *buf, len;

@@ -707,7 +707,7 @@
 
 int cat_read(struct policydb *p, struct hashtab *h, void *fp)
 {
-	char *key = 0;
+	char *key = NULL;
 	struct cat_datum *catdatum;
 	int rc;
 	u32 *buf, len;

===== security/selinux/ss/policydb.c 1.12 vs edited =====

@@ -99,7 +99,7 @@
  */
 int roles_init(struct policydb *p)
 {
-	char *key = 0;
+	char *key = NULL;
 	int rc;
 	struct role_datum *role;
 

@@ -402,7 +402,7 @@
 
 	kfree(key);
 	comdatum = datum;
-	hashtab_map(comdatum->permissions.table, perm_destroy, 0);
+	hashtab_map(comdatum->permissions.table, perm_destroy, NULL);
 	hashtab_destroy(comdatum->permissions.table);
 	kfree(datum);
 	return 0;

@@ -416,7 +416,7 @@
 
 	kfree(key);
 	cladatum = datum;
-	hashtab_map(cladatum->permissions.table, perm_destroy, 0);
+	hashtab_map(cladatum->permissions.table, perm_destroy, NULL);
 	hashtab_destroy(cladatum->permissions.table);
 	constraint = cladatum->constraints;
 	while (constraint) {

@@ -498,7 +498,7 @@
 	int i;
 
 	for (i = 0; i < SYM_NUM; i++) {
-		hashtab_map(p->symtab[i].table, destroy_f[i], 0);
+		hashtab_map(p->symtab[i].table, destroy_f[i], NULL);
 		hashtab_destroy(p->symtab[i].table);
 	}
 

@@ -669,7 +669,7 @@
 
 static int perm_read(struct policydb *p, struct hashtab *h, void *fp)
 {
-	char *key = 0;
+	char *key = NULL;
 	struct perm_datum *perdatum;
 	int rc;
 	u32 *buf, len;

@@ -718,7 +718,7 @@
 
 static int common_read(struct policydb *p, struct hashtab *h, void *fp)
 {
-	char *key = 0;
+	char *key = NULL;
 	struct common_datum *comdatum;
 	u32 *buf, len, nel;
 	int i, rc;

@@ -776,7 +776,7 @@
 
 static int class_read(struct policydb *p, struct hashtab *h, void *fp)
 {
-	char *key = 0;
+	char *key = NULL;
 	struct class_datum *cladatum;
 	struct constraint_node *c, *lc;
 	struct constraint_expr *e, *le;

@@ -943,7 +943,7 @@
 
 static int role_read(struct policydb *p, struct hashtab *h, void *fp)
 {
-	char *key = 0;
+	char *key = NULL;
 	struct role_datum *role;
 	int rc;
 	u32 *buf, len;

@@ -1008,7 +1008,7 @@
 
 static int type_read(struct policydb *p, struct hashtab *h, void *fp)
 {
-	char *key = 0;
+	char *key = NULL;
 	struct type_datum *typdatum;
 	int rc;
 	u32 *buf, len;

@@ -1055,7 +1055,7 @@
 
 static int user_read(struct policydb *p, struct hashtab *h, void *fp)
 {
-	char *key = 0;
+	char *key = NULL;
 	struct user_datum *usrdatum;
 	int rc;
 	u32 *buf, len;

===== security/selinux/ss/services.c 1.16 vs edited =====

@@ -308,7 +308,7 @@
 			u32 requested,
 			struct av_decision *avd)
 {
-	struct context *scontext = 0, *tcontext = 0;
+	struct context *scontext = NULL, *tcontext = NULL;
 	int rc = 0;
 
 	if (!ss_initialized) {

@@ -355,7 +355,7 @@
 {
 	char *scontextp;
 
-	*scontext = 0;
+	*scontext = NULL;
 	*scontext_len = 0;
 
 	/* Compute the size of the context. */

@@ -600,8 +600,8 @@
 				u32 specified,
 				u32 *out_sid)
 {
-	struct context *scontext = 0, *tcontext = 0, newcontext;
-	struct role_trans *roletr = 0;
+	struct context *scontext = NULL, *tcontext = NULL, newcontext;
+	struct role_trans *roletr = NULL;
 	struct avtab_key avkey;
 	struct avtab_datum *avdatum;
 	struct avtab_node *node;

What's wrong with using 0 as the NULL pointer? In contexts where a plain 0 is unsafe, NULL is usually unsafe as well.

Cheers,

Visit Openswan at http://www.openswan.org/
Email: Herbert Xu ~{PmV>HI~} <[email protected]>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

It's a general sparse cleanup people are doing across the entire tree. It's the "proper" way to do pointer comparisons post-K&R.

But 0 in such a context isn't an integer, it's a pointer...

If sparse really warns about such things, the warning seems wrong.

-Miles

((lambda (x) (list x x)) (lambda (x) (list x x)))

No it's not.

I'm sorry that you are such a K&R-C bigot that you don't like type checking. But the kernel DOES like type checking, and the kernel is not K&R C. The kernel uses strict ANSI, and in fact, is _more_ strict than ANSI C is in many many ways.

One of the "strict typechecking" rules is that you don't mix integers and pointers by mistake. The fact that C allows dual usage of the integer "0" is an anachronism that should have been fixed long ago.

Final word: K&R C without prototypes etc is still "legal C". That doesn't make it legal kernel code.

Linus

I don't have a copy of the standard handy, but google shows this snippet on the info-minux mailing list:

From ANSI X3.159-1989 3.2.2.3:

An integral constant expression with the value 0, or such an expression cast to the type void *, is called a null pointer constant. If a null pointer constant is assigned to or compared for equality to a pointer, the constant is converted to a pointer of that type.

Your prerogative.

-Miles

"1971 pickup truck; will trade for guns"

I have one. It's in my head. It's called the Linux Kernel C standard. Some of it is documented in CodinggStyle, others is just codified in existing practice.

That's some totally irrelevant standard that only acts as a rough guideline. It dosn't know about inline assembly, and it doesn't know about coding standards, and it allows the most atrocious code. There's even a contest in making such code - it's called the Obfuscated C Contest, and it actually encourages using strict ANSI rules.

Me, I don't accept the kind of entries the OCC accepts.

Linus

drivers/char/drm/ disagrees 8-)

SCNR

Dave

In that case... Linus, please remove those drivers, unless someone (DaveJ?) converts them from OCC to LKC.

Gr{oetje,eeting}s,

Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- [email protected]

In personal conversations with technical people, I call myself a hacker. But when I'm talking to journalists I just say "programmer" or something like that.
-- Linus Torvalds

Hi,

So far we have been quite liberal in style questions, what annoys me here is that people send warning patches directly to you without even notifying the maintainers. If you want people to conform people to a certain CodingStyle please document officially in the kernel, sparse isn't distributed with the kernel and the sparse police is silently changing the kernel all over the place with sometimes questionable benefit. Only the __user warnings had really found the bugs, but the rest I've seen changes perfectly legal code.

bye, Roman

I agree, when you're talking about the `if ((x = f())' cases. We already added the extra parentheses to shut up gcc...

Sparse also found the following for me:
- #if NEVER_DEFINED_DEFINE
- `return f();' in a function returning void (where f() returns void as well)
- `retval k_and_r_func(/* missing void */) { ... }'
- `extern' keywords at function definition
- inline functions used before definition
- const pointer assigned to non-const pointer (strange, because usually gcc tells me as well)
- if (x &= mask)' instead of if (x & mask)'
- floating point constants assigned to integers
- struct x { .field initializer };' (i.e. missing =')

BTW, the hardest part is adding the __user annotations and making sure <asm/uaccess.h> does it right (speaking about macros).

Gr{oetje,eeting}s,

Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- [email protected]

In personal conversations with technical people, I call myself a hacker. But when I'm talking to journalists I just say "programmer" or something like that.
-- Linus Torvalds

But why does sparse complain about

    p->thread.fs = get_fs().seg;

with

    linux-m68k-2.6.7/arch/m68k/kernel/process.c:265:23: warning: expected lvalue for member dereference

? Looks valid to me?

(patch to kill the warning below, _for reference only_)

@@ -242,6 +242,7 @@
 	struct pt_regs * childregs;
 	struct switch_stack * childstack, *stack;
 	unsigned long stack_offset, *retp;
+	mm_segment_t fs;
 
 	stack_offset = THREAD_SIZE - sizeof(struct pt_regs);
 	childregs = (struct pt_regs *) ((unsigned long) (p->stack) + stack_offset);

@@ -262,7 +263,8 @@
 	 * Must save the current SFC/DFC value, NOT the value when
 	 * the parent was last descheduled - RGH  10-08-96
 	 */
-	p->thread.fs = get_fs().seg;
+	fs = get_fs();
+	p->thread.fs = fs.seg;
 
 	if (!FPU_IS_EMU) {
 		/* Copy the current fpu state */

Gr{oetje,eeting}s,

Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- [email protected]

In personal conversations with technical people, I call myself a hacker. But when I'm talking to journalists I just say "programmer" or something like that.
-- Linus Torvalds

It is valid and should be left alone. sparse doesn't handle that area and unless somebody feels heroic it will stay that way.

Note that there are very scary critters in that part of language - among other things it contains such fun as arrays that can't be converted to pointers in normal way, etc. (arrays that have no address, while we are at it).

Sparse bug. Hey, it's not perfect, and this case is actually very easy to just parse, but harder to build a nice internal representation of it.

In particular, sparse internally really _should_ re-write the above as

	fnret = get_fs();
	p->thread.fs = fnret.seg;

since that is what would happen in real life, but since it never came up in early testing, I didn't ever really get around to doing it that way. Oh, well.

So please don't take _all_ sparse warnings too seriously. Some of them literally are still due to sparse limitations. I'm happy to say that they are fairly few these days.

Linus

Followup to:  <[email protected]>
By author:    Geert Uytterhoeven <[email protected]>
In newsgroup: linux.dev.kernel

Considering this one a bug is daft in the extreme.

Why? Because "return f();" is the only kind of tailcall syntax C has, and requiring that "void" functions use a different syntax is just stupid.

Now, if the return types don't match then that's another issue.

-hpa

* H. Peter Anvin:

Huh? If you remove the "return", it's still a valid tailcall syntax (at least from GCC's perspective).

Yes, but as I said in the portion of the message you deleted, it means you have to use a different syntax with void functions as for all other functions.

That is daft in the extreme.

-hpa

Well it's your project so you get to set the coding style.

But it is ironic that you call people who use 0 in a pointer context K&R-C bigots. One of the principal reason why NULL exists at all is in fact the lack of prototypes in K&R...

Visit Openswan at http://www.openswan.org/
Email: Herbert Xu ~{PmV>HI~} <[email protected]>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

There is one place where even prototypes won't help, which is varargs functions like execl. But I don't think the kernel uses functions with execl-like argument lists.

Andreas.

Andreas Schwab, SuSE Labs, [email protected]
SuSE Linux AG, Maxfeldstraße 5, 90409 Nürnberg, Germany
Key fingerprint = 58CA 54C7 6D53 942B 1756  01D3 44D5 214B 8276 4ED5
"And now for something completely different."

Actually printk is variadic. But gcc will provide warnings if it sees a mismatch between the format and the arguments.

Visit Openswan at http://www.openswan.org/
Email: Herbert Xu ~{PmV>HI~} <[email protected]>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

There are quite a few other varargs functions in the kernel, but I was specifically thinking of the use of a null pointer for terminating the argument list like execl does.

Andreas.

Andreas Schwab, SuSE Labs, [email protected]
SuSE Linux AG, Maxfeldstraße 5, 90409 Nürnberg, Germany
Key fingerprint = 58CA 54C7 6D53 942B 1756  01D3 44D5 214B 8276 4ED5
"And now for something completely different."

It's not about "unsafe". It's about being WRONG.

The fact is, people who write "0" are either living in the stone age, or are not sure about the type. "0" is an _integer_. It's not a pointer. It may be legal C, but that doesn't make it right anyway. "0" also happens to be one of the more _common_ integers, so mistakes happen.

Looking at the code, people that used "0" for NULL pointers quite often obviously were NOT aware of the types. The code just happened to pass through the compiler without warnings.

The same is true the other way too. I've seen too many damn people who use NULL in an integer context, and any compiler system that makes NULL be just a plain "0" is frigging _broken_. NULL is _not_ an integer. Never has been, and if the compiler doesn't warn loudly about obviously idiotic code, then the compiler is broken.

In other words:

	char * p = 0;	/* IS WRONG! DAMMIT! */
	int i = NULL;	/* THIS IS WRONG TOO! */

and anybody who writes code like the above either needs to get out of the kernel, or needs to get transported to the 21st century.

Linus

Not to be picky, and I realize that we're not using C++ here, and it may not apply, but every C++ text I've read deprecates NULL and says to use 0. That is, THE WAY that you specify a null pointer in C++ is with a zero. It's no surprise that C programmers might pick up that habit.

It's a bad habit, and it's one (of many) bad things C++ does. The sad part is that C++ does it for all the wrong reasons - there's no reason to not realize that ((void *)0) is an even better special case, and indeed last I heard at least gcc does allow that in C++ too.

The fact is, when somebody else picks up a mistake, that doesn't make it any less of a mistake. And it's not like C++ is the paragon of good taste anyway.

I've seen too damn many people mistake NULL and NUL (admit it, you've seen it too), and I've seen code like

	char c = NULL;

and any system where that goes through without a warning is totally broken. And yes, that includes a _lot_ of C++ braindamages.

Linus

Whee. Five seconds after writing the above and sending it off, what do I see on CNN but:

Another 'Police Academy' in works
New film would be eighth in series

LOS ANGELES, California (Hollywood Reporter) -- "Police Academy" is back. After a decade's absence from the big screen, the cop comedy franchise is gearing up for an eighth installment.

I rest my case.

Linus

THIS is simply a case of the programmer not understanding what NULL means. When I use '0' for a pointer, I know EXACTLY what I mean, and I also know when '0' might be ambiguous, and when I don't know what I'm allowed to do, then I play it REALLY safe and typecast 0 to exactly the pointer type I need.

I suppose it's good form to use the safest syntax in all cases. Good for readability for people who need more redundancy to read the code.

Perhaps the Linux kernel should have a convention where all NULL pointers are typecast explicitly. I can hear the cries of pain already. :)

This I agree with, definately. It's very important to make your code readable, and if it's not obvious from context, make it obvious. Cases like the above are one of the reasons I like languages like Verilog where you can pass parameters by specifying the parameter name.

If your function needs nine arguments it is not readable by definition. :-)

Andreas.

Andreas Schwab, SuSE Labs, [email protected]
SuSE Linux AG, Maxfeldstraße 5, 90409 Nürnberg, Germany
Key fingerprint = 58CA 54C7 6D53 942B 1756  01D3 44D5 214B 8276 4ED5
"And now for something completely different."

"If you have a procedure with ten parameters, you probably missed some."
- Alan Perlis

Andrew

Yeah... unfortunately, in chip design, you can't really avoid that. VHDL has structures which allow you to aggregate different signals, but Verilog doesn't have that feature, so you end up with hundreds of module parameters sometimes. That's when pass-by-name really pays off.

But then again, everything you know about software development is completely goes out the window when doing chip design. (i.e. in C, less code usually produces better results. The opposite is true in Verilog.)

Does this mean constructs like:
``if (pointer)'' and ``if (!pointer)'' are also outlawed.

And do we then need to initialize static pointers to NULL instead of letting them be implicitly 0.

Is doing memset(&(struct with_embeded_pointers), 0, sizeof(struct)) also wrong?

I don't see that 0 is WRONG. I do agree that ``((void *)0)'' is slightly more typesafe than ``0'', but since we don't have a lot of (void *) pointers in the kernel that is still the WRONG pointer type.

I do see that NULL has superior readability and maintainability and so should be encouraged by Documentation/CodingStyle.

The B and K&R roots of a simple single type language are what give C most of it's simplicity flexibility and power. Please don't be so eager to throw those out.

You want to be so typesafe it sounds like you want to recode the kernel in Pascal. You've written sparse, so it should be just a little more work to write a Pascal backend. After that the kernel will be so typesafe the compiler won't let us poor programmers get it wrong.

Eric

I suspect not. Up to Linus. This is all about writing code that doesn't bite.

Since mostly it's us humans doing the writing, this is more a human engineering problem than a pure mathematics problem such as Dijkstra or Wirth were closer to addressing.

Let someone with demonstrated good taste dictate the style choices that lead to short, sweat, but seldom screwy code.

It's all arbitrary as hell. The proof is in the pudding.

                          I won't rest till it's the best ...
                          Programmer, Linux Scalability
                          Paul Jackson <[email protected]> 1.650.933.1373

The place this probably matters most is in various function calls. For example, given the following prototype, (and ignoring the fact that gcc is generally able to properly compile all of these), which is the most clear? These are all "logically" correct, for the most part, but as humans we have certain readability standards.

int some_function(int a, void *b, char *c, unsigned char d, int e);

A) int res = some_function(0,0,0,0,0);
B) int res = some_function(NULL,NULL,NULL,NULL,NULL);
C) int res = some_function(0,NULL,NULL,'\0',0);

C is the most expressive of the intent of the code, and the least likely to contain bugs.

Cheers,
Kyle Moffett

Nope, B is undefined.

Visit Openswan at http://www.openswan.org/
Email: Herbert Xu ~{PmV>HI~} <[email protected]>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

Nope, B is implementation-defined. The conversion from pointers to integers is implementation-defined, and it's meant to be unsurprising to those familiar with the architecture. I.e., if you can zero-initialize a pointer and get a NULL pointer back, it's quite likely that a NULL pointer will convert back to (int)0, even though it's not required by the C Standard AFAICT.

Alexandre Oliva             http://www.ic.unicamp.br/~oliva/
Red Hat Compiler Engineer   aoliva@{redhat.com, gcc.gnu.org}
Free Software Evangelist  oliva@{lsd.ic.unicamp.br, gnu.org}

Of course not.

Why should they be?

What's considered bad form is:
- assignments in boolean context (because of the confusion of "=" and "==")
- thinking the constant "0" is a pointer.

There's no reason why "if (!ptr)" would be wrong. That has zero confusion about 0 vs NULL.

The confusion about "0" is that in traditional C it means two things: it can either be an integer (the common case) or it can sometimes be a pointer. That kind of semantic confusion is bad.

But it has nothing to do with the _value_ zero, or testing pointers for being non-NULL. The value zero is not about semantic confusion, it's just a bit pattern. And testing pointers is not ambiguous: when you test a pointer, it's _un_ambigiously checking that pointer for NULL.

Problems arise when there is room for confusion, and that's when the compiler should (and does) warn. If something is unambiguous, it's not bad.

Linus

Btw, has anybody who is complaining about the 0/NULL fixing actually _looked_ at the code?

Every single time a 0 was replaced by a NULL it was an obvious _improvement_ to the code. Not just "once". EVERY SINGLE TIME. It might be irritating to see the patches, and there might be too many of them since nothing has ever automatically noticed the bugs until now, but the fact is, there is not even any gray areas here - if you look at any of the patches being applied, they are ALL clearly making things more readable in their local context.

I really don't see the point of complaining about the fixes. There's just _no_ way to say that "0" is more readable than "NULL" in any of the cases. I dare you - show _one_ case where a 0/NULL patch was wrong or even remotely debatable. I dare you.

Linus

I definitely agree.

Several years ago I once wrote a singificant piece of code for a projet with #define NULL -1UL, this actually wasn't my choice but a requirement of the project (the headers were pre-defined) but it worked perfectly since we never did '!ptr' we always did 'ptr == NULL' instead (etc..). So at runtime it has never been a problem because we coded with NULL != 0 in mind. Of course I known normally NULL is always equal to 0 but I didn't realize that defining NULL !=0 wasn't exactly the C language (I learnt it later on the hard way in some mailing list, I believe at some point I did patches like the one in this thread but claiming it to be a bugfix, and not just a cleanup ;).

IIRC my argument about these patches being bugfixes, was about an architecture with a valid page mapped at address 0, that wouldn't generate a segfault. This is incidentally why we had to use NULL = -1 instead of NULL = 0. The answer I got at that time form some C guru is that I would need to hack the compiler specifically for such achitecture to accomodate for NULL = -1, so that '!ptr' will be the same as 'ptr == -1UL' (for pointers). In practice I think it has been a lot easier for us to avoid using '!ptr' than to hack gcc...

Well gcc is not terribly hackable that way, something about decades of code bloat...

The thing to keep in mind is that it was the integer to pointer conversion that needed to be hacked. (void *)0 is the null pointer constant by definition. So you it is technically illegal in that context to use (ptr *)0 to refer to the first page of memory. It must be another number that you can convert and manipulate.

For a single program I do agree that it probably would not be worth it but if you were delivering an architecture where people wrote lots of code it would be better fix the compiler to be standards conforming.

Eric

Linus> I really don't see the point of complaining about the Linus> fixes. There's just _no_ way to say that "0" is more Linus> readable than "NULL" in any of the cases. I dare you - Linus> show _one_ case where a 0/NULL patch was wrong or even Linus> remotely debatable. I dare you.

I don't know if any of the 0/NULL kernel patches were of this form, but I've seen sparse complain about this in my code and found it somewhat annoying. I think the following is at least remotely debatable...

Suppose I have

	struct foo {
		int a;
		int b;
	};

then sparse is perfectly happy with someone clearing out a struct foo like this:

	struct foo bar = { 0 };

but then if someone changes struct foo to be

	struct foo {
		void *x;
		int a;
		int b;
	};

sparse will complain about that initialization, and all of the fixes I can think of seem somewhat worse than the original to me:

struct foo bar = { NULL }; /* will I have to change this
again if struct foo changes? */

or

struct foo bar = { .a = 0 }; /* why do I have to name a member? */

or

struct foo bar;
memset(&bar, 0, sizeof bar); /* WRONG if a null pointer is not
the bit pattern 0 */

- Roland

Come on, this is madness. By accident, the first memeber which changed from 'int' to 'void *' now accepts the old initializer. In my book this is a really bad thing because you just changed the semantics of the initialiser '{ 0 }' quietly.

BTW, if nothing else, don't add new members at the top.

--
Eyal Lebedinsky ([email protected])

It complains rather rightly. Think what happens if the original initializer was
struct foo bar = { 1 };

This ambiguity may well be the main reason for the C99 initializer syntax.

Olaf

Only because the definition of the semantics of ``if'' is in terms of comparisons with ``0'', and I am familiar enough with the C programming language that, that is how I read it. It is still the case that because the comparison happens in pointer context the ``0'' referred to is the null pointer constant.

For some of us who are extremely familiar with C your argument is confusing. You make statements that sound like they are about the definition of the C programming language when in fact they are criticism of a given C programming style.

Since I am already making distinctions 0 as the integer value and 0 as the pointer constant when 0 is implicitly introduced. It is really not confusing to me in the case of manifest constants.

I would agree that using the constant "0" in a pointer context when a more explicit NULL is bad form. But "0" is the one legal way in C to write the NULL pointer constant.

For me it has exactly the same level of confusion as the cases that are being fixed has. I have to know the type to know if it is testing against the NULL pointer constant or if it is testing against the integer value zero.

Either that or it can be called greater expressive power though fewer concepts.

I like the fact this allows cases like ``if (!ptr)'' and friends.

see above.

The compiler is compiling the correct code so the code is clearly not ambiguous. But since types are not always obvious to a person staring at the code using the more explicit form of the constant i.e. NULL or '\0' instead of 0 adds useful redundancy.

Hopefully that explains why I objected to the way you can out against using ``0'' as the null pointer constant.

Eric

One reason I dislike the push to use NULL everywhere is that new-comers may feel a false sense of security when using NULL.

This will bite in places where an explicit cast is needed to turn NULL into a null pointer of the correct type, i.e., pointer arguments to variadic functions.

A neat trick to catch such errors is to define NULL to be 0LL. This is perfectly legal under ANSI and will generate a different representation on i386.

Cheers,

Visit Openswan at http://www.openswan.org/
Email: Herbert Xu ~{PmV>HI~} <[email protected]>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

So the real question is why C has no "null" token like Java or Pascal and re-uses the "0" token (which is really no token by itself but a numeric-constant token which happens to have a special value).

If your argument holds that "0" in a pointer context really is a special token like Java's "null" (which is explicitly defined by the standard as a pointer different from any other pointer) then it would be possible to implement a compiler which not only defines NULL to -1L, as someone mentioned here, but actually generates an all-ones bit pattern out of the constant 0 when used in a pointer context, yet generates an all-zeros bit pattern when used in an integer context. It also would have to implement the implicit null-comparison in a boolean context appropriately.

This probably would work with all programs which make a clear distinction between pointers and integer values, but you have to be really pedantic about these "stylistic" issues to always get it right in C. (Worse in C++ where usage of NULL is discouraged, I've always wondered about the reasons.)

The real problem, however, is that this "stylistic" issue may quickly become a _correctness_ issue as soon as the actual bit pattern of a pointer in memory is taken to have any meaning. I.e. it already starts when you initialize a structure with memset().

And this is the reason why really strongly typed languages never allow assignment of a pointer to or from any other data type. (Java is that strict, Pascal has a possible backdoor in the "record case" structure if implemented as overlays like C's unions but otherwise is as strict too, not sure about Ada.)
_The bit pattern of a pointer must have no meaning to the program_.

To answer the question from the first paragraph, it is "because C does _not_ strongly distinguish between pointer and non-pointer values". And for this reason people have invented the NULL constant, and the convention that

    "if (x)" means "if (x != 0)" in numeric context, and
    "if (x)" means "if (x != NULL)" in pointer context.

This resolves all the ambiguities and allows people to use C as a strongly typed language, but can break with program(mer)s taking pointers as equivalent to numbers.

Olaf

PS. I wonder how many bugs have been avoided in the Linux kernel by this kind of style pedantery vs. how many bugs have crept into other systems where people are more sloppy.

[ wondered about this as well, but the answer has been posted before in this
thread ]

Because C++ doesn't do implicit conversions from void * to anything *.

Gr{oetje,eeting}s,

Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- [email protected]

In personal conversations with technical people, I call myself a hacker. But when I'm talking to journalists I just say "programmer" or something like that.
-- Linus Torvalds

So what? NULL must have an integral type in C++. void* is explicitly forbidding in the C++ Standard.

I don't see that NULL is discouraged in C++. It's mostly redundant, like it is in C, and it's not safe for varargs even on machines where NULL pointers are represented can be zero-initialized, because pointers to members and regular pointers don't even have the same size, unlike C, that doesn't have pointers to members and thus can safely use (intptr_t)0 for NULL and it will even work for varargs (given the considerations above).

Alexandre Oliva             http://www.ic.unicamp.br/~oliva/
Red Hat Compiler Engineer   aoliva@{redhat.com, gcc.gnu.org}
Free Software Evangelist  oliva@{lsd.ic.unicamp.br, gnu.org}

This indeed is possible, and has been implemented in the past. Some segmented architectures use different segments for different data types, and a segment tag is part of the appropriate null pointer. Also, obviously, some pointer types are different widths on some architectures.

The C languages was defined in such a way that it works fine on such an architecture. int *p = 0; will always result in the appropriate null pointer for integers.

The one exception is if you have a variadic function, in which case you have to cast the null pointer to the right type, eg. printf("%s\n", (char*)0);

What's not allowed in conformant portable C is clearing pointers to null using memset. That just results in a zero bit pattern.

Eg.,

struct foo {
	int *bar;
} x;

memset(&x, 0, sizeof(x));

However, implicit initializers are valid, and will fill in pointer types with the appropriate null value, eg:

struct foo {
	int *bar;
} x = {};

In addition, defining NULL as (void*)0 is completely broken. A void* null pointer is not valid as a null pointer for some other type - void* is a transitional type, it simply holds the bit values of other pointers so you can cast them back later.

-J

FWIW, g++ defines NULL to the __null GCC-extension keyword, that issues a warning if it's doesn't decay to a pointer type, but behaves like the integral constant of value 0 otherwise, as required by the C++ standard.

I suppose Linux might benefit from such an extension implemented in C as well. Well, looks like sparse already takes care of that, and GCC has entered an new-extension-avoidance mode (or mood? :-) a few years ago.

Alexandre Oliva             http://www.ic.unicamp.br/~oliva/
Red Hat Compiler Engineer   aoliva@{redhat.com, gcc.gnu.org}
Free Software Evangelist  oliva@{lsd.ic.unicamp.br, gnu.org}

Hi,

gcc already warns about this, what value has this extra stuff?

bye, Roman

Err... Because the conditional expression is implicitly compared with 0 [6.8.4.1]/#2. If 0 is not to be used explicitly in pointer contexts, why should it be ok to use it implicitly?

[snip]

[6.5.3.3]/#5 defines the result of the logical negation operator based on the result of comparing the expression with 0.

I don't see why (!ptr) is any more confusing than (ptr != 0), and why (ptr != NULL) would be any clearer. Is `ptr != 0' one of the cases that are not bad?

Alexandre Oliva             http://www.ic.unicamp.br/~oliva/
Red Hat Compiler Engineer   aoliva@{redhat.com, gcc.gnu.org}
Free Software Evangelist  oliva@{lsd.ic.unicamp.br, gnu.org}

Exactly BECAUSE it's not an explicitly WRONG type of 0.

"0" is a number token. It is totally illogical from a conceptual standpoint to use it as a pointer. It makes no sense from any syntactic standpoint, and it's very much an ugly special case because K&R didn't want to add a keyword for NULL.

But the fact is, even early on, exactly _because_ "0" is illogical as a pointer, K&R added a

	#define NULL 0

to make it make _syntactic_ sense to use "NULL" as a pointer, even though the language lacked the specific keyword. So from a _syntactic_ standpoint, NULL is a pointer, even if from an implementation standpoint NULL ended up being this totally illogical integer 0.

In contrast, there is nothing syntactically strange about comparing a non-boolean (even though Pascal and other languages make it illegal). So here again, the "compare against 0" is an _implementation_ issue, not a conceptual syntactic confusion.

What I object to in using "0" as a pointer is that it changes the meaning of the token "0" depending on semantic information that may not even be very local. In contrast

	if (ptr)
		..

has no such confusion.

And you're totally confusing the "this is defined to be equivalent" as an implementation standpoint with "it's the same thing".

For example, the code

	5[ptr]

is _defined_ to be exactly the same as

	ptr[5]

since they both really mean

	*(ptr + 5)

and nothing else. HOWEVER, despite the fact that the C language _defines_ that they are exactly equivalent, I claim that anybody who writes "5[ptr]" is an ass.

For that same reason, your argument is totally irrelevant. Yes,

if (ptr)

is _defined_ to be exactly the same as

	if (ptr != 0)

but I claim that anybody who writes the latter is an ass, because it makes no syntactic sense. The same way "5[ptr]" doesn't make syntactic sense, even though the compiler will silently accept it.

Got it?

Linus

Executive summary:
It is not about the law (ANSI). It is about best practices that improve code quality. And transparency in the programmer's expression should always be welcome.

This thread can easily degrade into a religious argument (an oxymoron, can't argue about religion so just don't do it). I hope the below addresses the issues raised here and gives reason for my opinions.

Very much yes. I will go further and say that only boolean variables should use the above syntax. Using

	if (i)
where 'i' is a non-boolean integer instead of
	if (0 != i)

makes me question what the programmer wanted. Since integers do not have clear names for true/false logic (booleans usually will be called something like 'have_brain" etc.) the simple 'if (i)' may just as well be a miswritten 'if (!i)' - and I caught a few of these bugs in my time.

The whole point is not about "does the blind law of ANSI accepts this" because ANSI is written to be as permissible as possible so as not to invalidate deprecated bad style.

The whole point is about writing code that has a clear meaning, and using different forms for a zero integer and a generic empty pointer is a good way to force the writer to think and do the right thing.

Fact is that some code I can read as I see the snippet and without having to resort to too much header checking etc, while other code is a pain to go through (and don't even start me on consistent indentation).

And just to be clear. ANSI says this is valid

	char 	*p;
	int	i;
	...
	i[p] = 1;

Do you encourage this too? See what I mean? OK, so we all read the reference to the obfuscation competition...

I surely insist on this. Implicit is just not good enough to show that you gave it enough thought.

You probably think that 'calloc' is enough for initializing an object. I fixed enough problems from this. You declare a struct
- write a struct_clear function right away. It is worth it's wordcount in gold.

Yes, it was simple and flexible. It was a damn great improvement over assembly. However, by now it is clear that stronger typing is a good thing. Kernel code, where bugs have a high cost, is a good place to apply stricter rules than usual.

Did we forget the smiley?

--
Eyal Lebedinsky ([email protected])

So how about pushing for writing (i == 1) if i is boolean, to be clearer? It's often nice to be able to tell whether a boolean variable is strict 0/1 or just zero/non-zero, when you're thinking of switching to 3+-state logic. But guess what, i == 1 is far less efficient than i != 0 on many architectures. It's an unfortunate trade-off you have to make in this case.

Alexandre Oliva             http://www.ic.unicamp.br/~oliva/
Red Hat Compiler Engineer   aoliva@{redhat.com, gcc.gnu.org}
Free Software Evangelist  oliva@{lsd.ic.unicamp.br, gnu.org}

(off-topic alert)

Does anyone know of the origins of writing such tests this way, rather than:

	if (i != 0)

I read the first as testing whether "0" has a certain property, which is a silly thing to test, since the properties of "0" are rather constant.

The second form I read as testing a property of "i" - much more interesting. Logically, the same, of course. Just a question of which form is more idiomatic.

Back in the days when it was Ken, Dennis and Brian, not K & R, I don't recall seeing the first form used much. Even now I see _zero_ matches on "if (0 " in kernel or mm - only in arch, drivers, net, scripts, and sound (with a single time.h exception).

If I were Linus, I'd vote the first form off the island. Then again, if I were Linus, you would never have heard of Linux ;).

                          I won't rest till it's the best ...
                          Programmer, Linux Scalability
                          Paul Jackson <[email protected]> 1.650.933.1373

It was because

if (0 = i)

will give an error where you obviously meant '=='. It prevents accidental "assignment in conditional context".

Tim

You say that as if it were a bad thing...

I don't have a current C standard handy, but I believe there's a requirement that otherwise uninitialized static pointers be initialized to NULL even if that isn't "all bits off."

   -bill davidsen ([email protected])
"The secret to procrastination is to put things off until the
 last possible moment - but no longer"  -me

* Linus Torvalds:

It's neither. It's a literal. Context may turn it into something else, though.

Because NULL is a valid pointer value. 0 is not. If you were to make 0 valid, you would use "(void *)0", which is what NULL just happens to be in all known architectures so far, although that could change in an alternate universe.

Cheers,
Dick Johnson
Penguin : Linux version 2.4.26 on an i686 machine (5570.56 BogoMips).
Note 96.31% of all statistics are fiction.

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

Quoting "Richard B. Johnson" <[email protected]>:

No, that is not true.
In C/C++ this is true:
NULL == 0

You can _always_ use 0 instead of NULL. The use of NULL is _only_ for readability reasons. When you assign 0 to a pointer, the compiler knows that you want to assign a NULL-pointer and not the value 0.
Even on architectures where the NULL-pointer is not represented as 0 in memory (another bitmask), it's still valid to assign 0 to a pointer, because the compiler _knows_ that you are handling with a pointer and does The Right Thing (tm).

- --
Regards Michael Buesch [ http://www.tuxsoft.de.vu ]

-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.2.4 (GNU/Linux)

iD8DBQFA7S/mFGK1OIvVOP4RAoJ4AKCVY9QbxaXEjGthqGXpoN1Wqw8bzACgwzW/ AZVhE8MYOGNwKC/k74qZUBw=
=Vy7x
-----END PGP SIGNATURE-----

On Thu, 8 Jul 2004 13:28:38 +0200
Michael Buesch <[email protected]> bubbled:

hmm...

include/linux/stddef.h:

#undef NULL
#if defined(__cplusplus)
#define NULL 0
#else
#define NULL ((void *)0)
#endif

MyExcuse:
fractal radiation jamming the backbone

Martin Zwickel <[email protected]>
Research & Development

TechnoTrend AG <http://www.technotrend.de>

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

Quoting Martin Zwickel <[email protected]>:

Yes, I never understood the reason for this ugly
#if defined(__cplusplus) here.
It works, but is IMHO unneccessary.

- --
Regards Michael Buesch [ http://www.tuxsoft.de.vu ]

-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.2.4 (GNU/Linux)

iD8DBQFA7Ti+FGK1OIvVOP4RAgM/AJ9zsaNf0kKrQTq/a5R89pdjB8+/fgCfbS1p 1m6bM+MX3Dyg3lKcUK9qgRE=
=GxfG
-----END PGP SIGNATURE-----

-- Start of PGP signed section.

(This is is off topic, because kernel is not C++, but C).

Some quotations from Bjarne Stroustrup: The C++ Programming Language (Third Edition),

p. 843: Note that a pointer to function or a pointer to member
cannot be implicity converted to a void *.

p. 844: A constant expression (§C.5) that evaluates to 0 can
be implicitly converted to any pointer or pointer to member type (§5.1.1.).

p. 88: In C, it has been popular to define a macro NULL to
represent the zero pointer. Because of C++'s tighter type checking, the use of plain 0, rather than any suggested NULL macro, leads to fewer problems. If you feel you must define NULL, use

const int NULL = 0;

(typos mine.)

/ Kari Hurtta

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

That's all OK, fine and correct, but #define NULL 0
would work for both, C and C++ as far as I can see. Am I missing some special case?

Quoting Kari Hurtta <[email protected]>:

- --
Regards Michael Buesch [ http://www.tuxsoft.de.vu ]

-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.2.4 (GNU/Linux)

iD8DBQFA7UEvFGK1OIvVOP4RAua2AKCCwyC3TzasTBCbPQLaKaU47UJEbACZAY4P wd6n6AvSuJ+ThZE/Msbs9x0=
=jvQR
-----END PGP SIGNATURE-----

-- Start of PGP signed section.

As far I know it does not work on C when it is used as argument of function and function have not prototype or function's prototype have ...

In that case compiler do not know that pointer is required instead of integer.

However this is just "as far I know", now I have not in hand reference (or I did not found good quotation.)

/ Kari Hurtta

-- End of PGP signed section, PGP failed!

In that case NULL is wrong anyway since not all pointers are equivalent. You need to cast 0 or NULL to the exact pointer type required by that function.

Visit Openswan at http://www.openswan.org/
Email: Herbert Xu ~{PmV>HI~} <[email protected]>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

* Michael Buesch:

It's necessary because in C++, (void *)0 is not implicitly converted to other pointer types. Having to write static_cast<T*>(NULL) is certainly a bit too verbose.

There's also a C++ DR about this topic because people feel that there should be a 0/NULL distinction for overloading. But this is completely irrelevant to C code.

False. "An integer constant expressions with the value 0, or such an expression cast to type void *, is called a null pointer constant. If a null pointer constant is assigned to or compared for equality with a pointer, the constant is converted to a pointer of that type", and "Any two null pointers shall compare equal."

In other words, when you use 0 as a null pointer, you really do get a null pointer. If you are working on an architecture where the bit pattern of the integer 0 and null pointers are not the same, the compiler will perform the appropriate conversion for you, so it is always correct to define NULL as (void *)0.

Personally, I always use 0 and NULL for integers and null pointers respectively, but that's because of long estalished conventions that make the code readabile, rather than anything to do with validity of the code.

Peter

That's NOT what is says. It states that a NULL pointer is converted to the appropriate type before any comparison is made. It does NOT say that 0 is a valid null-pointer.

Cheers,
Dick Johnson
Penguin : Linux version 2.4.26 on an i686 machine (5570.56 BogoMips).
Note 96.31% of all statistics are fiction.

Could you please elaborate the rules of English in which "An integer constant expresion with the value 0 [...] is called a null pointer constant" does not mean that 0 is a null pointer? 0 is certainly an integer constant expression with the value 0, so there must be something extraordinarily subtle in the second half of the sentence.

Michael

He's emphasising the difference between "null pointer constant" and "null pointer", however NULL is defined as "an implementation-defined null pointer constant", so any subtle issues regarding 0 apply equally well to NULL.

Someone pointed out that there can be a difference between 0 and NULL when passing the value to a function lacking a prototype. There's no guarantee that the null pointers of different types have the same representation, so passing NULL into such a function without an explicit cast would also be incorrect.

Peter

Null pointer != null pointer constant. The latter is rather a syntactical construct without a real value. The process of converting a null pointer constant to a null pointer is the point where the decision is made about the final value and type.

Andreas.

Andreas Schwab, SuSE Labs, [email protected]
SuSE Linux AG, Maxfeldstraße 5, 90409 Nürnberg, Germany
Key fingerprint = 58CA 54C7 6D53 942B 1756  01D3 44D5 214B 8276 4ED5
"And now for something completely different."

0 when casted or assigned to a pointer will be converted to the null pointer value by the compiler, that's why doing !ptr is equivalent to ptr == NULL, even if ptr points to address 0xffffffff virtual in reality. Still NULL is set to (void *) 0, or alternatively (void *) -1UL (thought the latter I'm not 100% sure but I think it'll work in such a case).

It's mostly theory though, I've never seen an arch with a compiler with a null pointer not actually meaning virtual address 0UL (that's why we used a C-breaking #define NULL ((void*)-1UL) once we needed it).

exactly, the compiler knows about that.

Yep.

ah you're doing my same mistake! ;) Just read the email I just posted on this thread.