[syslinux] [PATCH 02/23] com32/chain: comments, minor adjustments

[Shao Miller]

On 11/5/2012 19:32, Michal Soltys wrote:
> - add some comments to clarify c{nul,add,max} modes
> - resilient against -Wconversion (uint8_t casts)
> - minor handover comment/flow changes
> - clean up some old comment-outs
>
> diff --git a/com32/chain/chain.c b/com32/chain/chain.c
> index f0ccd97..060043c 100644
> --- a/com32/chain/chain.c
> +++ b/com32/chain/chain.c
> @@ -453,7 +457,8 @@ static int setup_handover(const struct part_iter *iter,
>   	disk_dos_part_dump(ha);
>   	disk_gpt_part_dump((struct disk_gpt_part_entry *)(plen + 1));
>   #endif
> -    } else if (iter->type == typedos) {
> +    /* the only possible case left is dos scheme */
> +    } else {

An alternative to this would be a final 'else' that bails with an error. 
  While unlikely, it _could_ be useful for chain.c32 to understand APM, 
some day.  That'll be even more useful once chain.c32 stuff is further 
librarized for use with future multiple FS support.  If APM support were 
introduced, this recent hunk would change back the way it was.

>   	/* MBR handover protocol */
>   	synth_size = sizeof(struct disk_dos_part_entry);
>   	ha = malloc(synth_size);
> @@ -472,9 +477,6 @@ static int setup_handover(const struct part_iter *iter,
>   	dprintf("MBR handover:\n");
>   	disk_dos_part_dump(ha);
>   #endif
> -    } else {
> -	/* shouldn't ever happen */
> -	goto bail;
>       }
>

I didn't see this until just now. :)  It seems like it might've been 
fine the way it was, given my opinion above.

> diff --git a/com32/chain/utility.c b/com32/chain/utility.c
> index bc5b81a..d40c0dd 100644
> --- a/com32/chain/utility.c
> +++ b/com32/chain/utility.c
> -    (*dst)[0] = h;
> -    (*dst)[1] = s | ((c & 0x300) >> 2);
> -    (*dst)[2] = c;
> +    (*dst)[0] = (uint8_t)h;
> +    (*dst)[1] = (uint8_t)(s | ((c & 0x300) >> 2));
> +    (*dst)[2] = (uint8_t)c;
>   }
>
>   uint32_t get_file_lba(const char *filename)

Was this producing warnings?  Those casts seem redundant, since the 
assignment expression has type 'uint8_t'.  It's a touch more cluttered, 
with those.

> diff --git a/com32/chain/utility.h b/com32/chain/utility.h
> index 8b49122..a0519b7 100644
> --- a/com32/chain/utility.h
> +++ b/com32/chain/utility.h
>
> +/* see utility.c for details */
>   #define l2c_cnul 0
>   #define l2c_cadd 1
>   #define l2c_cmax 2
>

I missed the original opportunity to suggest an 'enum', here.  It can 
help with 'git grep', for example, if the 'mode' argument had an 
enumeration type that hinted where to look for related code.

The ordering here also seems consistent with the utility of 'enum'.

> -void error(const char *msg);
> -int guid_is0(const struct guid *guid);
>   void wait_key(void);
> -void lba2chs(disk_chs *dst, const struct disk_info *di, uint64_t lba, uint32_t mode);
> +void lba2chs(disk_chs *dst, const struct disk_info *di, uint64_t lba, int mode);
>   uint32_t get_file_lba(const char *filename);
>   int drvoff_detect(int type, unsigned int *off);
>   int bpb_detect(const uint8_t *bpb, const char *tag);
>
> +static inline
> +void error(const char *msg)
> +{
> +    fputs(msg, stderr);
> +}
> +
> +static inline
> +int guid_is0(const struct guid *guid)
> +{
> +    return !*(const uint64_t *)guid && !*((const uint64_t *)guid + 1);
> +}
> +
> +
>   #endif
>
>   /* vim: set ts=8 sts=4 sw=4 noet: */
>

I know this is a move from earlier code ('static inline' seems like a 
good idea), but for 'guid_is0', why not test the individual members 
instead of re-interpreting the object representation as a different 
type?  There are only 4 members and there are 2 comparands, here.  That 
doesn't take much more code, and removes assumptions about object 
representation.

- Shao Miller