This whole series is fine and this patch is also fine. But I had some
unrelated comments about the original code in this function.
On Wed, Nov 12, 2014 at 11:28:24AM -0500, Jeffrey Brown wrote:
> Inserted a necessary brace for an if statement on line 146 of
> parser.c
>
> Signed-off-by: Jeffrey Brown <Jeffrey.Brown@xxxxxxxxxx>
> ---
> drivers/staging/unisys/visorchipset/parser.c | 4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/staging/unisys/visorchipset/parser.c b/drivers/staging/unisys/visorchipset/parser.c
> index 17c1649..6381e59 100644
> --- a/drivers/staging/unisys/visorchipset/parser.c
> +++ b/drivers/staging/unisys/visorchipset/parser.c
> @@ -143,9 +143,9 @@ cleanups:
> visor_memregion_destroy(rgn);
> rgn = NULL;
> }
> - if (rc)
> + if (rc) {
> controlvm_payload_bytes_buffered += ctx->param_bytes;
> - else {
> + } else {
> if (ctx) {
> parser_done(ctx);
> ctx = NULL;
This business with a "cleanups" label at the end of every function is
"One Err" type error handling because the traditional label name is
"err:". It's an anti-pattern which I hate because it is confusing and
error prone.
When you have one err label then you end up with a lot of if statements
and spaghetti code. The error handling here is spaghetti code as I will
explain at length.
The error handling here is also buggy, of course. We call parser_done()
which subtracts ctx->param_bytes but that was never added to begin with.
It should just call kfree(ctx) instead.
"rc" in the linux kernel normally means "Return Code" and zero is
success otherwise it is a negative error code. Here "rc" is a pointer
and NULL is an error. It is the reverse of what people would normally
expect.
Setting "ctx" and "rgn" to NULL at the end doesn't do anything useful.
You should almost always do "error handling" instead of "success
handling" meaning that the code should look like:
x = frob();
if (error)
...
As opposed to:
x = frob();
if (success)
...
In other words, "if (rc) " is the opposite of what you would expect
because it is "success handling".
Label names should be named after what the label does. "Away" is
basically meaningless.
Don't have do nothing labels. They just annoy people who are trying
to figure out what the labels do. This is bad/annoying:
cleanups:
return ret;
It's normally more readable to separate the error path from the success
path. Sometimes it's more code to do that so you might not want to do
that on a hot path. But we don't care about that here so we should do
the most readable thing:
out_rgn:
if (rgn)
visor_memregion_destroy(rgn);
Controlvm_Payload_Bytes_Buffered += ctx->param_bytes;
return ctx;
err_rgn:
if (rgn)
visor_memregion_destroy(rgn);
err_ctx:
kfree(ctx);
return NULL;
In this version there is no "rc" variable so all the "rc = NULL"
assignments can be removed making the code shorter and simpler. There is
only "rgn" if statement and it mirrors an if statement earlier in the
function. All the error labels have meaningful names.
Anyway, none of that affects this particular patch, but could you please
at least fix the bug I mentioned in a later patch.
regards,
dan carpenter
_______________________________________________
devel mailing list
devel@xxxxxxxxxxxxxxxxxxxxxx
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
