On Tue, Aug 18, 2015 at 10:54:52PM -0400, Cody P Schafer wrote: > On Tue, Aug 18, 2015 at 7:08 PM, David Gibson > <da...@gibson.dropbear.id.au> wrote: > > On Mon, Aug 17, 2015 at 08:33:29PM -0400, Cody P Schafer wrote: > >> Signed-off-by: Cody P Schafer <d...@codyps.com> > > > > Concept looks fine. A number of minor nits. > > > >> --- > >> +/** > >> + * memstarts - determine if @data starts with @prefix > >> + * @data: does this begin with @prefix? > >> + * @data_len: bytes in @data > >> + * @prefix: does @data begin with these bytes? > >> + * @prefix_len: bytes in @prefix > >> + * > >> + * Returns true if @data starts with @prefix, otherwise return false. > >> + * > >> + * Example: > >> + * if (memstarts(somebytes, bytes_len, otherbytes, otherbytes_len)) { > >> + * printf("somebytes starts with otherbytes!\n"); > >> + * } > >> + */ > >> +static inline bool memstarts(void const *data, size_t data_len, > >> + void const *prefix, size_t prefix_len) > >> +{ > >> + if (prefix_len > data_len) > >> + return false; > >> + return !memcmp(data, prefix, prefix_len); > > > > I'd prefer to see the order changed, and use memeq() here. > > I don't think memeq() fits here, as it includes the length comparison, > which is required when checking if 2 blocks of memory are equal, but > serves no purpose here. > > I'd need to either: > - pass prefix_len twice (ugly)
I think this is correct, and I don't see it as that ugly. > - remove the memstarts length check and use MIN(prefix_len, > data_len), which makes it harder to think about the function, and it > seems silly to generate a value we know will cause the function to > return false rather than just returning false directly. > > Neither seems really great. > > > > >> +} > >> + > >> +/** > >> + * memeq - Are two byte arrays equal? > >> + * @a: first array > >> + * @al: bytes in first array > >> + * @b: second array > >> + * @bl: bytes in second array > >> + * > >> + * Example: > >> + * if (memeq(somebytes, bytes_len, otherbytes, otherbytes_len)) { > >> + * printf("memory blocks are the same!\n"); > >> + * } > >> + */ > >> +#define memeq(a, al, b, bl) (al == bl && !memcmp(a, b, bl)) > > > > Needs brackeds around the macro arguments, in case they're complex > > expressions. > > Are you looking for just the following? : > > +#define memeq(a, al, b, bl) ((al) == (bl) && !memcmp(a, b, bl)) > > Or should the function args be paren'd as well? The function args should have parens as well - consider what happens if one of the params is a comma expression (of course it's not trivial to get an un-parenthised comma expression into a macro argument, but you should still handle it. -- David Gibson | I'll have my music baroque, and my code david AT gibson.dropbear.id.au | minimalist, thank you. NOT _the_ _other_ | _way_ _around_! http://www.ozlabs.org/~dgibson
pgpHqXfE588yL.pgp
Description: PGP signature
_______________________________________________ ccan mailing list ccan@lists.ozlabs.org https://lists.ozlabs.org/listinfo/ccan