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

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

On 12/3/2024 5:36 AM, Vladimir Oltean wrote:
> On Mon, Dec 02, 2024 at 04:26:26PM -0800, Jacob Keller wrote:
>> diff --git a/include/linux/packing.h b/include/linux/packing.h
>> index 5d36dcd06f60420325473dae3a0e9ac37d03da4b..f9bfb20060300e33a455b46d3266ea5083a62102 100644
>> --- a/include/linux/packing.h
>> +++ b/include/linux/packing.h
>> @@ -7,6 +7,7 @@
>>  
>>  #include <linux/types.h>
>>  #include <linux/bitops.h>
>> +#include <linux/packing_types.h>
> 
> I'm unsure of the benefit of splitting the headers in this way, if
> packing_types.h is not going to contain purely auto-generated code and
> is tracked fully by git.
> 

I can put this back into one file. I had kept it split from when we
needed things separated to work with modpost.

I can move it back to a single file.

>> diff --git a/Documentation/core-api/packing.rst b/Documentation/core-api/packing.rst
>> index 821691f23c541cee27995bb1d77e23ff04f82433..5f729a9d4e87b438b67ec6b46626403c8f1655c3 100644
>> --- a/Documentation/core-api/packing.rst
>> +++ b/Documentation/core-api/packing.rst
>> @@ -235,3 +235,61 @@ programmer against incorrect API use.  The errors are not expected to occur
>>  during runtime, therefore it is reasonable for xxx_packing() to return void
>>  and simply swallow those errors. Optionally it can dump stack or print the
>>  error description.
>> +
>> +The pack_fields() and unpack_fields() macros automatically select the
>> +appropriate function at compile time based on the type of the fields array
>> +passed in.
> 
> This paragraph is out of context (select the appropriate function among
> which options? what fields array?).
> 

I'll use your suggested documentation.

> Also, I think this patch could use some de-cluttering by making the
> documentation update separate. We need to document 2 new APIs anyway,
> not just pack_fields() but also pack().
> 

I can split the documentation out.

>> diff --git a/scripts/Makefile b/scripts/Makefile
>> index 6bcda4b9d054021b185488841cd36c6e0fb86d0c..546e8175e1c4c8209e67a7f92f7d1e795a030988 100644
>> --- a/scripts/Makefile
>> +++ b/scripts/Makefile
>> @@ -47,7 +47,7 @@ HOSTCFLAGS_sorttable.o += -DMCOUNT_SORT_ENABLED
>>  endif
>>  
>>  # The following programs are only built on demand
>> -hostprogs += unifdef
>> +hostprogs += unifdef gen_packed_field_checks
> 
> I will let those who have been more vocal in their complaints about
> the compile-time checks comment on whether this approach of running
> gen_packed_field_checks on demand rather than during any build is
> acceptable.
> 

We had a lot of issues with getting the generated file to work properly,
and I even *still* had some minor issues where the files just didn't get
generated properly. If others agree to go back to generating on demand I
can try to resolve all of those.. but I'd prefer to not touch the top
level Kbuild :(

I think the overhead of committing these lines when its no longer 20K+
is acceptable for the simplicity of just having it made on-demand. I
also prefer keeping the script as an actual script you can run vs trying
to embed the script in the header.