From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mails.dpdk.org (mails.dpdk.org [217.70.189.124]) by inbox.dpdk.org (Postfix) with ESMTP id 2578A459BD; Tue, 17 Sep 2024 13:37:17 +0200 (CEST) Received: from mails.dpdk.org (localhost [127.0.0.1]) by mails.dpdk.org (Postfix) with ESMTP id B4DD8402CB; Tue, 17 Sep 2024 13:37:16 +0200 (CEST) Received: from us-smtp-delivery-124.mimecast.com (us-smtp-delivery-124.mimecast.com [170.10.133.124]) by mails.dpdk.org (Postfix) with ESMTP id 5A7EB40261 for ; Tue, 17 Sep 2024 13:37:15 +0200 (CEST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1726573034; h=from:from:reply-to:subject:subject:date:date:message-id:message-id: to:to:cc:cc:mime-version:mime-version:content-type:content-type: content-transfer-encoding:content-transfer-encoding: in-reply-to:in-reply-to:references:references; bh=tIEwg/Lt73JqVdbahyulJhN6txwmVE8xEzgyjxeCq6Q=; b=U1tHf/iwHtsFBGW75T4aUM4ZZEsrJaG7wj5o2CPyO0V7N+HVtjul+0qw4CTrQf6CAhoz5F Zv40mXeLDtNBKSnaZCdrZqra9PWFW1DkCUW9nGnerMubZ/eG6iEYuQjcVdPkNLfAjjl2o4 3Ib4A5Cekbe7lHXPhIdvIX+Y2paljMk= Received: from mail-lj1-f200.google.com (mail-lj1-f200.google.com [209.85.208.200]) by relay.mimecast.com with ESMTP with STARTTLS (version=TLSv1.3, cipher=TLS_AES_256_GCM_SHA384) id us-mta-528-MftdNDbrPdSYOt4Fb9mFNA-1; Tue, 17 Sep 2024 07:37:13 -0400 X-MC-Unique: MftdNDbrPdSYOt4Fb9mFNA-1 Received: by mail-lj1-f200.google.com with SMTP id 38308e7fff4ca-2f761cfa667so36964361fa.3 for ; Tue, 17 Sep 2024 04:37:13 -0700 (PDT) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1726573032; x=1727177832; h=content-transfer-encoding:cc:to:subject:message-id:date:from :in-reply-to:references:mime-version:x-gm-message-state:from:to:cc :subject:date:message-id:reply-to; bh=tIEwg/Lt73JqVdbahyulJhN6txwmVE8xEzgyjxeCq6Q=; b=nArauxODYfNN+275VlGkO+wAohYP+77C6mL+8QLWe7SlLiDYn8LMLuYavwWksJuzjE a6Bg97+7lR0iGlJ4Vry8LywrO4dQFXjyQCRPDI2NTo386Xz21aulAAHhTQh/Ogx0AE8a JzXkBCPLVhKwEPYc1U4BhoMO2ldksI+DEEGIZA/E75rp9nRtMTpArc+7fUgwwoX+cUxz u9kaIc3tnqHEM4SATXTbPxA3va07CROqF+FpUbHGfrpf8YHRBth/4Gba2KXw5kIdaU/i yRIztXTsTP8hG+idwlkF9o30eo9JhYGf4iU5TX2Stz1U/2jjVkD2ll+AEixgDWEAnJ48 aLlQ== X-Gm-Message-State: AOJu0YygdVHJyzbhSEX4355Gsmmxr/3XVcDYFZqDCZDMV2CH2oD5l4be GHjrfpvmSFJNsJP90EkmBAfn9VlnhmsRwxZr9gBPB1XMdTlwvl14QrAEXmUKREgn14YC6eH4Bac ii0CtXdLyq0SmYjz1mXd2XVY3dab1699wO4dv0FX1NChHTamhiW6k94wt4D7oZL4Evpk6zHYmhz YoFqvgZaq0IhWTy8g= X-Received: by 2002:a05:651c:1992:b0:2f7:6194:b716 with SMTP id 38308e7fff4ca-2f787da0c7bmr105182621fa.5.1726573031915; Tue, 17 Sep 2024 04:37:11 -0700 (PDT) X-Google-Smtp-Source: AGHT+IGKvYLNiS3ql+zPAtRcl2lzGqLcqD9q2eKzyD27FiYcuO6+uCRzEcjAwwHpWJEC2pm3ptiuuCi81HDCg9HtsPw= X-Received: by 2002:a05:651c:1992:b0:2f7:6194:b716 with SMTP id 38308e7fff4ca-2f787da0c7bmr105182251fa.5.1726573031245; Tue, 17 Sep 2024 04:37:11 -0700 (PDT) MIME-Version: 1.0 References: <20240911193224.1966122-1-maxime.coquelin@redhat.com> <20240911193224.1966122-2-maxime.coquelin@redhat.com> In-Reply-To: <20240911193224.1966122-2-maxime.coquelin@redhat.com> From: David Marchand Date: Tue, 17 Sep 2024 13:36:59 +0200 Message-ID: Subject: Re: [RFC v3 1/3] uapi: introduce kernel uAPI headers import To: Maxime Coquelin Cc: dev@dpdk.org, techboard@dpdk.org, thomas@monjalon.net, mb@smartsharesystems.com, stephen@networkplumber.org X-Mimecast-Spam-Score: 0 X-Mimecast-Originator: redhat.com Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable X-BeenThere: dev@dpdk.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: DPDK patches and discussions List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: dev-bounces@dpdk.org On Wed, Sep 11, 2024 at 9:32=E2=80=AFPM Maxime Coquelin 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/L= ICENSES/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 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/inc= lude/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 exi= st: $ devtools/linux-uapi.sh import linux/vfio.h v6.10 # Non interactive mode, the script uses the version file and imported heade= rs: $ devtools/linux-uapi.sh check Regardless of this suggestion, I have some nits about the shell scripts bel= ow: > --- > 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=3D"https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/lin= ux.git/plain/include/uapi/" > +base_path=3D"linux-headers/uapi/" > +errors=3D0 > +version=3D"" > + > +print_usage() > +{ > + echo "Usage: $(basename $0) [-h]" > +} > + > +check_uapi_header() { > + path=3D$1 > + file=3D${1#$base_path} > + > + cp -f $path $tmpinput1 > + > + # Restore includes fixups done by import-linux-uapi.sh > + sed -i "s|//#include |#include |g" $tmpinput1 > + sed -i "s|#include + > + url=3D"${base_url}${file}?h=3D${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=3D$((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=3D$(mktemp -t dpdk.checkuapi.XXXXXX) > +tmpinput2=3D$(mktemp -t dpdk.checkuapi.XXXXXX) > +trap "rm -f '$tmpinput1 $tmpinput2'" INT > + > +version=3D$(< ${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}" +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.s= h > 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=3D"https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/lin= ux.git/plain/include/uapi/" > +base_path=3D"linux-headers/uapi/" > +version=3D"" > +file=3D"" > + > +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 giv= en 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=3D${filename//"$base_path"} ${filename%$base_path} > + local url > + local path > + > + echo "Updating to $version" > + while IFS=3D read -d '' -r filename; do for filename in $(find $base_path -name "*.h" -type f) > + header=3D${filename//"$base_path"/} header=3D${filename%$base_path} > + url=3D"${base_url}${header}?h=3D${version}" > + path=3D"${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=3D$1 > + > + local url=3D"${base_url}${header}?h=3D${version}" > + local path=3D"${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/t= ty && [ "$import" =3D 'y' ] || continue Do we really need to force /dev/tty ? > + echo "Importing $include for $path" > + import_header "$include" > + fi > + done > +} > + > +fixup_includes() > +{ > + local path=3D$1 > + > + # Do not include linux/compiler.h as done by make headers > + sed -i "s|^#include |//#include |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=3D$OPTARG > + ;; > + u ) > + version=3D$OPTARG > + ;; > + h ) > + print_usage > + exit 0 > + ;; > + ? ) > + print_usage > + exit 1 > + ;; > + esac > +done > + > +cd $(dirname $0)/.. > + > +current_version=3D$(< ${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 >=3D $= version)" > + version=3D$current_version > + else > + update_headers > + fi > +else > + echo "Version not specified, using current version ($current_vers= ion)" > + version=3D$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}" +done > + > +echo $version > ${base_path}/version --=20 David Marchand