functions returning 0 on success [was: [PATCH] Fix a memory leakin the i386 setup code]

[Michael Tokarev]

Catalin Marinas wrote:
> From: Catalin Marinas <catalin.marinas@xxxxxxxxx>
[]
> -		request_resource(&iomem_resource, res);
> +		if (request_resource(&iomem_resource, res)) {
> +			kfree(res);
...

Just a small nitpick, or, rather, a question.
Quite alot of functions return 0 on success, or !=0 on failure.
There are other functions, which, say, return NULL on failure.
Or when 0 is valid result, something <0 is returned.. etc.

Without looking at the function description or code, it's
impossible to say wich of the above it is.  But.

When reading that kind of code as the quoted patch adds,
I for one always think it's somehow incorrect or backwards.
When used like that, request_resource() seems like a boolean,
and the whole thing becomes:

  if (do_something_successeful())
    fail();

Ofcourse, later you understand that do_something() returns 0
on failure, and the code is correct.  But the first impression
(again, for me anyway) is that it's wrong.

In such cases when a routine returns 0 on error, I usually write
it this way:

   if (request_resource() != 0)
     fail()

This way it becomes obvious what does it do, and compiler
generates EXACTLY the same instructions.

Yes it's redundrand, that "!= 0" tail.  But it makes the
whole stuff readable without a need to re-think what does it
return, and, which is especially important, logically correct
when reading.

Alternatively, it can be named something like

  request_resource_failed()

instead of just request_resource(). Compare:

 if (request_resource(..))
    fail();

(current).  And

 if (request_resource_failed())
   fail();

The latter seems more logical... ;)

But that "!= 0" tail achieves almost the same effect.
Yet again, from my point of view anyway... ;)

Thanks.

/mjt
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/