Re: [RFC v2 1/3] uapi: introduce kernel uAPI headers import

[Maxime Coquelin <maxime.coquelin@redhat.com>]

On 9/7/24 16:34, David Marchand wrote:
> On Fri, Sep 6, 2024 at 5:23 PM Maxime Coquelin
> <maxime.coquelin@redhat.com> wrote:
> 
> [snip]
> 
>> diff --git a/devtools/check-linux-uapi.sh b/devtools/check-linux-uapi.sh
>> new file mode 100755
>> index 0000000000..76111d78ce
>> --- /dev/null
>> +++ b/devtools/check-linux-uapi.sh
>> @@ -0,0 +1,74 @@
>> +#!/bin/sh
> 
> -e maybe?

It will stop on first diff without reporting anything.
At least on how it is done for now.

>> +# SPDX-License-Identifier: BSD-3-Clause
>> +# Copyright (c) 2024 Red Hat, Inc.
>> +
>> +#
>> +# Import Linux Kernel uAPI header file
> 
> # Check Linux Kernel uAPI headers
> 
>> +#
>> +
>> +base_url="https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/plain/include/uapi/"
>> +base_path="linux-headers/uapi/"
>> +errors=0
>> +
>> +print_usage()
>> +{
>> +       echo "Usage: $(basename $0) [-h]"
>> +}
>> +
>> +check_uapi_header() {
>> +       path=$1
>> +       file=${1//"$base_path"/}
> 
> I suspect it is a bashism.
> 
> https://pubs.opengroup.org/onlinepubs/9699919799/utilities/V3_chap02.html
> ${parameter#[word]}
> Remove Smallest Prefix Pattern. The word shall be expanded to produce
> a pattern. The parameter expansion shall then result in parameter,
> with the smallest portion of the prefix matched by the pattern
> deleted. If present, word shall not begin with an unquoted '#'.
> 
> So a POSIX alternative is:
> file=${1#$base_path/}

Changed to:
file=${1#$base_path}

> 
> 
>> +       version=$(git log --format=%b -1 $path | sed -ne 's/^uAPI Version: \(.*\)$/\1/p')
> 
> I would add a check and raise a warning (error?) if extracting version
> fails (iow -z "$version").

It disappears in new version.

> 
>> +
>> +       url="${base_url}${file}?h=${version}"
>> +       echo -n "Checking $file for version $version... "
>> +       curl -s -f -o $tmpinput $url
>> +       if [ $? -ne 0 ]; then
>> +               echo "Failed to download $url"
>> +               exit 1
>> +       fi
>> +
>> +       diff -q $path $tmpinput >/dev/null
>> +       if [ $? -ne 0 ]; then
>> +               echo "KO"
>> +               diff -u $path $tmpinput
>> +               errors=$((errors+1))
>> +       else
>> +               echo "OK"
>> +       fi
>> +}
>> +
>> +while getopts hv ARG ; do
>> +       case $ARG in
>> +               h )
>> +                       print_usage
>> +                       exit 0
>> +                       ;;
>> +               ? )
>> +                       print_usage
>> +                       exit 1
>> +                       ;;
>> +       esac
>> +done
>> +
>> +shift $(($OPTIND - 1))
>> +if [ $# -ne 0 ]; then
>> +       print_usage
>> +       exit 1
>> +fi
>> +
>> +cd $(dirname $0)/..
>> +
>> +tmpinput=$(mktemp -t dpdk.checkuapi.XXXXXX)
>> +trap "rm -f '$tmpinput'" INT
>> +
>> +while IFS= read -d '' -r filename; do
> 
> Simpler:
> 
> for filename in $(find $base_path -name "*.h" -type f); do
>       check_uapi_header "${filename}" </dev/null
> done

Done!

> 
>> +       check_uapi_header "${filename}" </dev/null
>> +done < <(find $base_path -name "*.h" -type f -print0)
> 
> 
>> +
>> +echo "$errors error(s) found"
>> +
>> +rm -f $tmpinput
>> +trap - INT
>> +
>> +exit $errors
> 
> [snip]
> 
>> diff --git a/doc/guides/contributing/linux_uapi.rst b/doc/guides/contributing/linux_uapi.rst
>> new file mode 100644
>> index 0000000000..a3f684013a
>> --- /dev/null
>> +++ b/doc/guides/contributing/linux_uapi.rst
>> @@ -0,0 +1,77 @@
>> +.. SPDX-License-Identifier: BSD-3-Clause
>> +   Copyright(c) 2024 Red Hat, Inc.
>> +
>> +Linux uAPI header files
>> +=======================
>> +
>> +Rationale
>> +---------
>> +
>> +The system a DPDK library or driver is built on is not necessarily running the
>> +same Kernel version than the system that will run it.
>> +Importing Linux Kernel uAPI headers enable to build features that are not
>> +supported yet by the build system.
>> +
>> +For example, the build system runs upstream Kernel v5.19 and we would like to
>> +build a VDUSE application that will use VDUSE_IOTLB_GET_INFO ioctl() introduced
>> +in Linux Kernel v6.0.
>> +
>> +`Linux Kernel licence exception regarding syscalls
>> +<https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/plain/LICENSES/exceptions/Linux-syscall-note>`_
>> +enable importing unmodified Linux Kernel uAPI header files.
>> +
>> +Importing or updating an uAPI header file
>> +-----------------------------------------
>> +
>> +In order to ensure the imported uAPI headers are both unmodified and from a
>> +released version of the linux Kernel, a helper script is made available and
>> +MUST be used.
>> +Below is an example to import ``linux/vduse.h`` file from Linux ``v6.10``:
>> +
>> +.. code-block:: console
>> +
>> +   ./devtools/import-linux-uapi.sh linux/vduse.h v6.10
>> +
>> +Once imported, the header files should be committed without any other change,
>> +and the commit message MUST specify the imported version using
>> +``uAPI Version:`` tag and title MUST be prefixed with uapi keyword.
>> +For example::
>> +
>> +  uapi: import VDUSE header file
>> +
>> +  This patch imports VDUSE uAPI header file for inclusion
>> +  into the Vhost library.
>> +
>> +  uAPI Version: v6.10
>> +
>> +  Signed-off-by: Alex Smith <alex.smith@example.com>
>> +
>> +Updating an already imported header to a newer released version should only
>> +be done on a need basis.
>> +The commit message should reflect why updating the header is necessary.
> 
> +1.
> 
> 
>> +
>> +Once committed, user can check headers and commit message are valid by using
>> +the Linux uAPI checker tool:
>> +
>> +.. code-block:: console
>> +
>> +   ./devtools/check-linux-uapi.sh
> 
> And thanks for adding this check.
> It will help maintainers.

Welcome.
Do you think we would need a sub-maintainer for this, or you prefere the
main maintainers to take care of this?

I'll be happy to help with the maintenance if you feel the need.

> 
>> +
>> +Header inclusion into library or driver
>> +---------------------------------------
>> +
>> +The library or driver willing to make use of imported uAPI headers needs to
>> +explicitly add uAPI headers path to the ``includes`` var in its ``meson.build``
>> +file:
>> +
>> +.. code-block:: python
>> +
>> +   includes += linux_uapi_inc
> 
> Now that the uapi headers directory is pushed to global_inc, there is
> no need for this part in the doc.

Right, I'm also adapting the commit message to reflect this.

> 
>> +
>> +Then, it can be included with ``uapi/`` prefix in C files.
>> +For example to include VDUSE uAPI:
>> +
>> +.. code-block:: c
>> +
>> +   #include <uapi/linux/vduse.h>
>> +
>> diff --git a/linux-headers/uapi/.gitignore b/linux-headers/uapi/.gitignore
>> new file mode 100644
>> index 0000000000..88829d04e9
>> --- /dev/null
>> +++ b/linux-headers/uapi/.gitignore
>> @@ -0,0 +1,3 @@
>> +**
>> +!**/
>> +!**/*.h
> 
> Nice trick to solve the per patch build issue :-).

:-)

> 

Thanks,
Maxime