Re: [PATCH net-next v3 3/9] lib: packing: add pack_fields() and unpack_fields()

[Jacob Keller <jacob.e.keller@intel.com>]

On 11/8/2024 10:31 AM, Vladimir Oltean wrote:
> On Fri, Nov 08, 2024 at 01:24:07PM +0200, Vladimir Oltean wrote:
>> On Thu, Nov 07, 2024 at 11:50:34AM -0800, Jacob Keller wrote:
>>> +#define DECLARE_PACKED_FIELDS_S(name, buffer_sz) \
>>> +	const size_t __ ## name ## _buffer_sz __pf_section_s = buffer_sz; \
>>> +	const struct packed_field_s name[] __pf_section_s
>>
>> This will need sorting out - how to make this declaration macro
>> compatible with the "static" keyword. The obvious way (to group the
>> field array and the buffer size into a structure) doesn't work. It loses
>> the ARRAY_SIZE() of the fields, which is important to the pack_fields()
>> and unpack_fields() wrappers.
>>
>> Maybe a different tool is needed for checking that no packed fields
>> exceed the buffer size? Forcing the buffer size be a symbol just because
>> that's what modpost works with seems unnatural.
>>
>> If we knew the position of the largest field array element in C, and if
>> we enforced that all pack_fields() use a strong type (size included) for
>> the packed buffer, we'd have all information inside the pack_fields()
>> macro, because we only need to compare the largest field against the
>> buffer size. We could just have that part as a BUILD_BUG_ON() wrapped
>> inside the pack_fields() macro itself. And have the compile-time checks
>> spill over between C and modpost...
>>
>> Not to mention, pack_fields() would have one argument less: pbuflen.
> 
> I was thinking something like this (attached). I still don't like
> modpost more than the assertions in C code, because it imposes more
> constraints upon the library user which didn't exist before. Though
> without the extra restrictions added in this patch (just ascending order
> for packed fields + strong type for packed buffer), I don't think the
> modpost variant is going to work, or is going to become extremely complex.

To me, the restrictions seem acceptable. You can always wrap an
arbitrary void buffer into a sized type via a structure.

I do agree that modpost isn't perfect with respect to the C code, but I
dislike the sheer size of the macros generated and the complexity added
to the build system.

My preferred solution is unfortunately not available in C: having the
compiler evaluate a constant function. There is a C++ extension that
does that, but it doesn't seem like GCC has enabled such support in the
C even as an extension.

I'd prefer to keep allowing both ascending and descending order, because
I think those are both valid orderings depending on how you're thinking
about the fields (little vs big endian).