On Tue, 2022-05-03 at 18:44 -0700, Kees Cook wrote: > > For example, using the most complicated helper, mem_to_flex_dup(): > > /* Flexible array struct with members identified. */ > struct something { > int mode; > DECLARE_FLEX_ARRAY_ELEMENTS_COUNT(int, how_many); > unsigned long flags; > DECLARE_FLEX_ARRAY_ELEMENTS(u32, value); In many cases, the order of the elements doesn't really matter, so maybe it'd be nicer to be able to write it as something like DECLARE_FLEX_STRUCT(something, int mode; unsigned long flags; , int, how_many, u32, value); perhaps? OK, that doesn't seem so nice either. Maybe struct something { int mode; unsigned long flags; FLEX_ARRAY( int, how_many, u32, value ); }; or so? The long and duplicated DECLARE_FLEX_ARRAY_ELEMENTS_COUNT and DECLARE_FLEX_ARRAY_ELEMENTS seems a bit tedious to me, at least in cases where the struct layout is not the most important thing (or it's already at the end anyway). > struct something *instance = NULL; > int rc; > > rc = mem_to_flex_dup(&instance, byte_array, count, GFP_KERNEL); > if (rc) > return rc; This seems rather awkward, having to set it to NULL, then checking rc (and possibly needing a separate variable for it), etc. But I can understand how you arrived at this: - need to pass instance or &instance or such for typeof() or offsetof() or such - instance = mem_to_flex_dup(instance, ...) looks too much like it would actually dup 'instance', rather than 'byte_array' - if you pass &instance anyway, checking for NULL is simple and adds a bit of safety but still, honestly, I don't like it. As APIs go, it feels a bit cumbersome and awkward to use, and you really need everyone to use this, and not say "uh what, I'll memcpy() instead". Maybe there should also be a realloc() version of it? > +/** __fas_bytes - Calculate potential size of flexible array structure I think you forgot "\n *" in many cases here after "/**". johannes