I have added Markus to the CC list because I want to describe how
"extra" if statements make the code more readable towards the end of
this function.
This patch needs more work.
The subject shouldn't be "11/11" because it's not part of a series.
Just say: "[PATCH] staging: unisys: small bug in parser.c".
On Thu, Nov 13, 2014 at 09:56:39AM -0500, Jeffrey Brown wrote:
> Fixed small bug in parser.c by removing "cleanups:" in parser_init
> _guts struct. Replaced it with proper error handling code
> and removed the instances of rc = NULL in the code. rc = NULL is
> redudant
>
> Signed-off-by: Jeffrey Brown <Jeffrey.Brown@xxxxxxxxxx>
> ---
> drivers/staging/unisys/visorchipset/parser.c | 46 ++++++-------------------
> 1 files changed, 11 insertions(+), 35 deletions(-)
>
> diff --git a/drivers/staging/unisys/visorchipset/parser.c b/drivers/staging/unisys/visorchipset/parser.c
> index 5f6a7b2..beb36a2 100644
> --- a/drivers/staging/unisys/visorchipset/parser.c
> +++ b/drivers/staging/unisys/visorchipset/parser.c
Get rid of the "rc" declaration.
> @@ -64,8 +64,6 @@ parser_init_guts(u64 addr, u32 bytes, BOOL is_local,
> MAX_CONTROLVM_PAYLOAD_BYTES);
> if (try_again)
> *try_again = TRUE;
> - rc = NULL;
> - goto cleanups;
We haven't allocated "ctx" yet so it's just "return NULL;"
> }
> ctx = kzalloc(allocbytes, GFP_KERNEL|__GFP_NORETRY);
> if (ctx == NULL) {
> @@ -73,8 +71,6 @@ parser_init_guts(u64 addr, u32 bytes, BOOL is_local,
> __func__, __FILE__, __LINE__, allocbytes);
> if (try_again)
> *try_again = TRUE;
> - rc = NULL;
> - goto cleanups;
return NULL;
> }
>
> ctx->allocbytes = allocbytes;
> @@ -89,42 +85,27 @@ parser_init_guts(u64 addr, u32 bytes, BOOL is_local,
> ERRDRV("%s - bad local address (0x%-16.16Lx for %lu)",
> __func__,
> (unsigned long long)addr, (ulong)bytes);
> - rc = NULL;
> - goto cleanups;
Now we have allocated ctx but not rgn so "goto err_ctx;"
> }
> p = __va((ulong)(addr));
> memcpy(ctx->data, p, bytes);
> } else {
> rgn = visor_memregion_create(addr, bytes);
> - if (!rgn) {
> - rc = NULL;
> - goto cleanups;
We tried to allocate rgn but failed so it's still "goto err_ctx;"
> - }
> - if (visor_memregion_read(rgn, 0, ctx->data, bytes) < 0) {
> - rc = NULL;
> - goto cleanups;
Every error after this point is "goto err_rgn;"
> - }
> }
> if (!has_standard_payload_header) {
> ctx->byte_stream = TRUE;
> rc = ctx;
> - goto cleanups;
But this is not an error so it goes to the success path. "goto out_rgn;"
> }
> phdr = (struct spar_controlvm_parameters_header *)(ctx->data);
> if (phdr->total_length != bytes) {
> ERRDRV("%s - bad total length %lu (should be %lu)",
> __func__,
> (ulong)(phdr->total_length), (ulong)(bytes));
> - rc = NULL;
> - goto cleanups;
goto err_rgn;
> }
> if (phdr->total_length < phdr->header_length) {
> ERRDRV("%s - total length < header length (%lu < %lu)",
> __func__,
> (ulong)(phdr->total_length),
> (ulong)(phdr->header_length));
> - rc = NULL;
> - goto cleanups;
goto err_rgn;
> }
> if (phdr->header_length <
> sizeof(struct spar_controlvm_parameters_header)) {
> @@ -133,25 +114,20 @@ parser_init_guts(u64 addr, u32 bytes, BOOL is_local,
> (ulong)(phdr->header_length),
> (ulong)(sizeof(
> struct spar_controlvm_parameters_header)));
> - rc = NULL;
> - goto cleanups;
goto err_rgn;
> }
>
Ok, now we write the code for the end of the success path.
The rgn allocation is conditinal so we *must* have an if statement here.
We could use the if statement hidden inside the call to
visor_memregion_destroy() but that means when someone reads the code
they think, "Why is there not an if statement here. Oh. It is hidden
in another place in the code." Using the hidden if statement is a
layering violation. Readers shouldn't have to know the deep details of
how visor_memregion_destroy() is implemented.
out_rgn:
if (rgn)
visor_memregion_destroy(rgn);
return ctx;
Now we write the error path:
err_rgn:
if (rgn)
visor_memregion_destroy(rgn);
err_ctx:
kfree(ctx);
return NULL;
}
Some people might argue that it is a waste of memory to have:
if (rgn)
visor_memregion_destroy(rgn);
on both the success path and the error path but I have three responses:
1) This is not a fast path.
2) This makes the code more readable.
3) The original code was far more wasteful.
regards,
dan carpenter
_______________________________________________
devel mailing list
devel@xxxxxxxxxxxxxxxxxxxxxx
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
