On Wed, Sep 11, 2024 at 9:32 PM Maxime Coquelin <maxime.coque...@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.coque...@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? # 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 -- David Marchand
