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

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

On 9/17/24 13:36, David Marchand wrote:
> On Wed, Sep 11, 2024 at 9:32 PM Maxime Coquelin
> <maxime.coquelin@redhat.com> wrote:
>>
>> This patch introduces uAPI headers import into the DPDK
>> repository. This import is possible thanks to Linux Kernel
>> licence exception for syscalls:
>>
>> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/LICENSES/exceptions/Linux-syscall-note
>>
>> Header files are have to be explicitly imported.
>>
>> Guidelines are provided in the documentation, and helper
>> scripts are also provided to ensure proper importation of the
>> header (unmodified content from a released Kernel version):
>>   - import-linux-uapi.sh: used to add and update headers and
>>   their dependencies to linux-headers/uapi/
>>   - check-linux-uapi.sh: used to check all headers are valid
>>
>> Signed-off-by: Maxime Coquelin <maxime.coquelin@redhat.com>
> 
> I have been trying this script to import linux/vfio.h and cleanup its
> usage in DPDK.
> 
> There is one issue that was raised.
> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/include/uapi/linux/vfio.h#n1573
> 
> struct vfio_bitmap {
> __u64        pgsize; /* page size for bitmap in bytes */
> __u64        size; /* in bytes */
> __u64 __user *data; /* one bit per page */
> };
> 
> The __user annotation is sanitized by the headers install script in
> the kernel, but the dpdk import script is missing this part.
> Such sanitizations breaks the check script.
> 
> We could invert the logic in the check script: instead of "restoring"
> an imported header, the check would convert a freshly downloaded
> header and compare it against the imported header in dpdk.
> One thing though is that we would need a copy of the "conversion"
> function in the two scripts.
> 
> One idea.. can we have a single script?

I think your suggestion makes sense.
Will implement it in v1 with other nits you suggest below.

Thanks,
Maxime

> # Interactive mode, with questions about what to import if dependencies exist:
> $ devtools/linux-uapi.sh import linux/vfio.h v6.10
> 
> # Non interactive mode, the script uses the version file and imported headers:
> $ devtools/linux-uapi.sh check
> 
> 
> Regardless of this suggestion, I have some nits about the shell scripts below:
> 
>> ---
>>   devtools/check-linux-uapi.sh           |  85 ++++++++++++++++++
>>   devtools/import-linux-uapi.sh          | 119 +++++++++++++++++++++++++
>>   doc/guides/contributing/index.rst      |   1 +
>>   doc/guides/contributing/linux_uapi.rst |  71 +++++++++++++++
>>   linux-headers/uapi/.gitignore          |   4 +
>>   linux-headers/uapi/version             |   1 +
>>   meson.build                            |   8 +-
>>   7 files changed, 287 insertions(+), 2 deletions(-)
>>   create mode 100755 devtools/check-linux-uapi.sh
>>   create mode 100755 devtools/import-linux-uapi.sh
>>   create mode 100644 doc/guides/contributing/linux_uapi.rst
>>   create mode 100644 linux-headers/uapi/.gitignore
>>   create mode 100644 linux-headers/uapi/version
>>
>> diff --git a/devtools/check-linux-uapi.sh b/devtools/check-linux-uapi.sh
>> new file mode 100755
>> index 0000000000..fc42c03169
>> --- /dev/null
>> +++ b/devtools/check-linux-uapi.sh
>> @@ -0,0 +1,85 @@
>> +#!/bin/sh
> 
> It should not be too hard to pass -e, see suggestions.
> 
> 
>> +# SPDX-License-Identifier: BSD-3-Clause
>> +# Copyright (c) 2024 Red Hat, Inc.
>> +
>> +#
>> +# Check Linux Kernel uAPI header files
>> +#
>> +
>> +base_url="https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/plain/include/uapi/"
>> +base_path="linux-headers/uapi/"
>> +errors=0
>> +version=""
>> +
>> +print_usage()
>> +{
>> +       echo "Usage: $(basename $0) [-h]"
>> +}
>> +
>> +check_uapi_header() {
>> +       path=$1
>> +       file=${1#$base_path}
>> +
>> +       cp -f $path $tmpinput1
>> +
>> +       # Restore includes fixups done by import-linux-uapi.sh
>> +       sed -i "s|//#include <linux/compiler.h>|#include <linux/compiler.h>|g" $tmpinput1
>> +       sed -i "s|#include <uapi/|#include <|g" $tmpinput1
>> +
>> +       url="${base_url}${file}?h=${version}"
>> +       echo -n "Checking $file... "
>> +       curl -s -f -o $tmpinput2 $url
>> +       if [ $? -ne 0 ]; then
> 
> I always have trouble remembering the value of $?... maybe simpler:
> if ! curl -s -f -o $tmpinput2 $url; then
> 
> Doing so makes it possible to pass a -e in the shebang.
> 
>> +               echo "Failed to download $url"
>> +               exit 1
> 
> And this is a shell function, prefer return 1, rather than make the
> whole script exit.
> 
>> +       fi
>> +
>> +       diff -q $tmpinput1 $tmpinput2 >/dev/null
>> +       if [ $? -ne 0 ]; then
> 
> Idem:
> if ! diff -q $tmpinput1 $tmpinput2 >/dev/null
> 
>> +               echo "KO"
>> +               diff -u $tmpinput1 $tmpinput2
>> +               errors=$((errors+1))
> 
> Touching the global variable errors should be done by the caller from
> my pov, so here, return 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)/..
>> +
>> +tmpinput1=$(mktemp -t dpdk.checkuapi.XXXXXX)
>> +tmpinput2=$(mktemp -t dpdk.checkuapi.XXXXXX)
>> +trap "rm -f '$tmpinput1 $tmpinput2'" INT
>> +
>> +version=$(< ${base_path}/version)
>> +
>> +echo "Checking imported headers for version ${version}"
>> +
>> +for filename in $(find $base_path -name "*.h" -type f); do
>> +       check_uapi_header "${filename}" </dev/null
> 
> No need for </dev/null afaics.
> 
> And I would increment the global var errors here, so something like:
> check_uapi_header "${filename}" || errors=$((errors + 1))
> 
>> +done
>> +
>> +echo "$errors error(s) found"
>> +
>> +rm -f $tmpinput1 $tmpinput2
>> +trap - INT
>> +
>> +exit $errors
>> diff --git a/devtools/import-linux-uapi.sh b/devtools/import-linux-uapi.sh
>> new file mode 100755
>> index 0000000000..ff15e23821
>> --- /dev/null
>> +++ b/devtools/import-linux-uapi.sh
>> @@ -0,0 +1,119 @@
>> +#!/bin/sh
> 
> Similarly, I would try to make this script runable with -e.
> 
> 
>> +# SPDX-License-Identifier: BSD-3-Clause
>> +# Copyright (c) 2024 Red Hat, Inc.
>> +
>> +#
>> +# Import Linux Kernel uAPI header file
>> +#
>> +
>> +base_url="https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/plain/include/uapi/"
>> +base_path="linux-headers/uapi/"
>> +version=""
>> +file=""
>> +
>> +print_usage()
>> +{
>> +       echo "Usage: $(basename $0) [-h] [-a FILE] [-u VERSION]"
>> +       echo "-a FILE      import Linux header file. E.g. linux/vfio.h"
>> +       echo "-u VERSION   update imported list of Linux headers to a given version. E.g. v6.10"
>> +}
>> +
>> +version_valid() {
> 
> The name is strange, this is comparing versions iiuc.
> version_older_than() maybe ?
> 
> 
>> +    printf '%s\n%s' "$1" "$2" | sort -C -V
>> +}
>> +
>> +update_headers()
>> +{
>> +       local header=${filename//"$base_path"}
> 
> ${filename%$base_path}
> 
>> +       local url
>> +       local path
>> +
>> +       echo "Updating to $version"
>> +       while IFS= read -d '' -r filename; do
> 
> for filename in $(find $base_path -name "*.h" -type f)
> 
>> +               header=${filename//"$base_path"/}
> 
> header=${filename%$base_path}
> 
>> +               url="${base_url}${header}?h=${version}"
>> +               path="${base_path}${header}"
>> +               curl -s -f -o $path $url
> 
> Maybe log an explicit error if curl fails, like in the check script.
> 
> 
>> +       done < <(find $base_path -name "*.h" -type f -print0)
>> +}
>> +
>> +import_header()
>> +{
>> +       local include
>> +       local import
>> +       local header=$1
>> +
>> +       local url="${base_url}${header}?h=${version}"
>> +       local path="${base_path}${header}"
>> +
>> +       curl -s -f --create-dirs -o $path $url
> 
> Log an error if failing.
> 
> 
>> +
>> +       for include in $(sed -ne 's/^#include <\(.*\)>$/\1/p' $path); do
>> +               if [ ! -f "${base_path}${include}" ]; then
>> +                       read -p "Import $include (y/n): " import < /dev/tty && [ "$import" = 'y' ] || continue
> 
> Do we really need to force /dev/tty ?
> 
>> +                       echo "Importing $include for $path"
>> +                       import_header "$include"
>> +               fi
>> +       done
>> +}
>> +
>> +fixup_includes()
>> +{
>> +       local path=$1
>> +
>> +       # Do not include linux/compiler.h as done by make headers
>> +       sed -i "s|^#include <linux/compiler.h>|//#include <linux/compiler.h>|g" $path
>> +
>> +       # Prepend include path with "uapi/" if the header is imported
>> +       for include in $(sed -ne 's/^#include <\(.*\)>$/\1/p' $path); do
>> +               if [ -f "${base_path}${include}" ]; then
>> +                       sed -i "s|${include}|uapi/${include}|g" $path
>> +               fi
>> +       done
>> +}
>> +
>> +while getopts a:u:hv opt ; do
>> +       case ${opt} in
>> +               a )
>> +                       file=$OPTARG
>> +                       ;;
>> +               u )
>> +                       version=$OPTARG
>> +                       ;;
>> +               h )
>> +                       print_usage
>> +                       exit 0
>> +                       ;;
>> +               ? )
>> +                       print_usage
>> +                       exit 1
>> +                       ;;
>> +       esac
>> +done
>> +
>> +cd $(dirname $0)/..
>> +
>> +current_version=$(< ${base_path}/version)
>> +
>> +if [ -n "${version}" ]; then
>> +       version_valid "$version" "$current_version"
>> +       if [ $? -eq 0 ]; then
> 
> if version_older_than "$version" "$current_version"; then
> 
>> +               echo "Headers already up to date ($current_version >= $version)"
>> +               version=$current_version
>> +       else
>> +               update_headers
>> +       fi
>> +else
>> +       echo "Version not specified, using current version ($current_version)"
>> +       version=$current_version
>> +fi
>> +
>> +if [ -n "${file}" ]; then
>> +       import_header $file
>> +fi
>> +
>> +for filename in $(find $base_path -name "*.h" -type f); do
>> +       fixup_includes "${filename}" </dev/null
> 
> No need for </dev/null
> 
>> +done
>> +
>> +echo $version > ${base_path}/version
> 
>