[syslinux] [PATCH 06/23] com32/chain: partiter - simplifications and updates

[Shao Miller]

On 11/5/2012 19:32, Michal Soltys wrote:
> The code uses more abstractions than it's really worth or necessary, so
> these patches simplify the code a bit. Partially suggested by Shao as
> well.
>
> Additionally, there're some added comments and more consistent naming
> used.
>
> Signed-off-by: Michal Soltys <soltys at ziu.info>
> ---
>   com32/chain/chain.c    |    8 +-
>   com32/chain/mangle.c   |    4 +-
>   com32/chain/partiter.c |  350 +++++++++++++-----------------------------------
>   com32/chain/partiter.h |   39 +++---
>   4 files changed, 122 insertions(+), 279 deletions(-)
>
> diff --git a/com32/chain/partiter.c b/com32/chain/partiter.c
> index 037236f..dffe83f 100644
> --- a/com32/chain/partiter.c
> +++ b/com32/chain/partiter.c

>   {
> -    int i, cnt = sizeof(types)/sizeof(types[0]);
> -    for (i = 0; i < cnt; i++) {
> -	if (type == types + i)
> -	    return 0;
> -    }
> -    return -1;
> +    /* syslinux's free is null resilient */
> +    free(iter->data);
>   }

As a matter of fact, it wouldn't conform to Standard C, if it wasn't. 
'free' must accept null pointers.  A little trivia: Interestingly, 
Standard C states that after doing so, the pointer value becomes 
indeterminate; could be a trap representation, so it is not safe to 
evaluate afterwards.  free(ptr); printf("Freed: %p\n", ptr); /* Bad */

> -#ifdef DEBUG
> -    if (inv_type(type)) {
> -	error("Unknown iterator requested.\n");
> -	goto bail;
> -    }
> -#endif
> -
> -    if (!(iter = malloc(sizeof(struct part_iter)))) {
> +    if (!(iter = malloc(sizeof(struct part_iter))))

The idiom 'obj_ptr = malloc(sizeof *obj_ptr)' works, for what it's worth.

>   	error("Couldn't allocate memory for the iterator.\n");
> -	goto bail;
> -    }
> -
> -    memset(iter, 0, sizeof(struct part_iter));

Same with 'memset(obj_ptr, 0, sizeof *obj_ptr)'

> diff --git a/com32/chain/partiter.h b/com32/chain/partiter.h
> index a394b6e..f441e46 100644
> --- a/com32/chain/partiter.h
> +++ b/com32/chain/partiter.h
> @@ -40,12 +40,14 @@
>   #include <stdint.h>
>   #include <syslinux/disk.h>
>
> -#define PI_ERRLOAD 3
> -#define PI_INSANE 2
> -#define PI_DONE 1
> -#define PI_OK 0
> +/* status */
>
> -/* behaviour flags */
> +#define PI_ERRLOAD  3
> +#define PI_INSANE   2
> +#define PI_DONE	    1
> +#define PI_OK	    0
> +
> +/* flags */
>

Another opportunity for an 'enum' for these status values, perhaps? 
These look to be mutually exclusive.  I'm mentioning it just because the 
code being changed here, anyway.

- Shao Miller