Re: [Qemu-devel] Is network backend netmap worth keeping?

[Markus Armbruster <armbru@redhat.com>]

Giuseppe Lettieri <giuseppe.lettieri@unipi.it> writes:

> Il 13/09/19 15:04, Markus Armbruster ha scritto:
>>
>> What happens when I build with --enable-netmap=system on host A, then
>> run the resulting binary on some host B that doesn't have netmap
>> installed?
>>
>
> Qemu will fail at startup, complaining that /dev/netmap does not exists.
>
>
>>
>> Yes.  We default to netmap=system, though.  If you break things by
>> passing arcane arguments to configure, you get to keep the pieces :)
>>
>>> If the option is only useful for developers to check that some qemu
>>> change does not break anything, then probably it should be enabled in
>>> some other, less visible way. What do you think?
>>
>> I think an --enable-netmap patterned after --enable-capstone and
>> --enable-slirp has sufficiently low visibility as long as the default is
>> sane.
>>
>> We clearly want configure to pick netmap=system when the system provides
>> netmap.
>>
>> What shall configure pick when the system doesn't provide it?  If you
>> think netmap=git is too dangerous for general audience, consider
>> disabling netmap then.  Experts can still compile-test with
>> --enable-netmap=git.  Our CI certainly should.
>>
>
> OK, sounds reasonable. The attached patch will select system if netmap
> is available, and git only if explicitly requested.
>
> Cheers,
> Giuseppe
>
> -- 
> Dr. Ing. Giuseppe Lettieri
> Dipartimento di Ingegneria della Informazione
> Universita' di Pisa
> Largo Lucio Lazzarino 1, 56122 Pisa - Italy
> Ph. : (+39) 050-2217.649 (direct) .599 (switch)
> Fax : (+39) 050-2217.600
> e-mail: g.lettieri@iet.unipi.it
>
>>From 4e93b5cc3ad68d92bc3562df3745e1d10dc08fc0 Mon Sep 17 00:00:00 2001
> From: Giuseppe Lettieri <g.lettieri@iet.unipi.it>
> Date: Mon, 2 Sep 2019 17:35:33 +0200
> Subject: [PATCH] netmap: support git-submodule build otption
>
> With this patch, netmap support can be enabled with
> the following options to the configure script:
>
>   --enable-netmap[=system]
>
> 	Use the host system netmap installation.
> 	Fail if not found.
>
>   --enable-netmap=git
>
> 	clone the official netmap repository on
> 	github (mostly useful for CI)
>
> system will also be automatically used if no option is
> passed and netmap is available in the host system.
>
> Signed-off-by: Giuseppe Lettieri <giuseppe.lettieri@unipi.it>
> ---
>  .gitmodules |  3 +++
>  configure   | 68 ++++++++++++++++++++++++++++++++++++++++++++---------
>  netmap      |  1 +
>  3 files changed, 61 insertions(+), 11 deletions(-)
>  create mode 160000 netmap
>
> diff --git a/.gitmodules b/.gitmodules
> index c5c474169d..bf75dbc5e3 100644
> --- a/.gitmodules
> +++ b/.gitmodules
> @@ -58,3 +58,6 @@
>  [submodule "roms/opensbi"]
>  	path = roms/opensbi
>  	url = 	https://git.qemu.org/git/opensbi.git
> +[submodule "netmap"]
> +	path = netmap
> +	url = https://github.com/luigirizzo/netmap.git
> diff --git a/configure b/configure
> index 30aad233d1..5cb924985c 100755
> --- a/configure
> +++ b/configure
> @@ -1133,6 +1133,10 @@ for opt do
>    ;;
>    --enable-netmap) netmap="yes"
>    ;;
> +  --enable-netmap=git) netmap="git"
> +  ;;
> +  --enable-netmap=system) netmap="system"
> +  ;;
>    --disable-xen) xen="no"
>    ;;
>    --enable-xen) xen="yes"
> @@ -3314,8 +3318,9 @@ fi
>  # a minor/major version number. Minor new features will be marked with values up
>  # to 15, and if something happens that requires a change to the backend we will
>  # move above 15, submit the backend fixes and modify this two bounds.
> -if test "$netmap" != "no" ; then
> -  cat > $TMPC << EOF
> +case "$netmap" in
> +    "" | yes | system)
> +      cat > $TMPC << EOF
>  #include <inttypes.h>
>  #include <net/if.h>
>  #include <net/netmap.h>
> @@ -3325,15 +3330,56 @@ if test "$netmap" != "no" ; then
>  #endif
>  int main(void) { return 0; }
>  EOF
> -  if compile_prog "" "" ; then
> -    netmap=yes
> -  else
> -    if test "$netmap" = "yes" ; then
> -      feature_not_found "netmap"
> +      if compile_prog "" "" ; then
> +        netmap_system=yes
> +      else
> +        netmap_system=no
> +      fi
> +      ;;
> +esac

Is the indentation change intentional?

> +
> +case "$netmap" in
> +  "" | yes)
> +    if test "$netmap_system" = "yes"; then
> +      netmap=system
> +    elif test -e "${source_path}/.git" && test $git_update = 'yes' ; then
> +      netmap=git
> +    elif test -e "${source_path}/netmap/configure" ; then
> +      netmap=internal
> +    elif test -z "$netmap" ; then
> +      netmap=no
> +    else
> +      feature_not_found "netmap" "Install netmap or git submodule"
>      fi
> -    netmap=no
> -  fi
> -fi
> +    ;;
> +
> +  system)
> +    if test "$netmap_system" = "no"; then
> +      feature_not_found "netmap" "Install netmap"
> +    fi
> +    ;;
> +esac
> +
> +case "$netmap" in
> +  git | internal)
> +    if test "$netmap" = git; then
> +      git_submodules="${git_submodules} netmap"
> +    fi
> +    mkdir -p netmap
> +    QEMU_CFLAGS="$QEMU_CFLAGS -I\$(SRC_PATH)/netmap/sys"
> +    ;;
> +
> +  system)
> +    ;;
> +
> +  no)
> +    ;;
> +  *)
> +    error_exit "Unknown state for netmap: $netmap"
> +    ;;
> +esac
> +
> +##########################################
>  
>  ##########################################
>  # libcap-ng library probe
> @@ -6582,7 +6628,7 @@ if test "$vde" = "yes" ; then
>    echo "CONFIG_VDE=y" >> $config_host_mak
>    echo "VDE_LIBS=$vde_libs" >> $config_host_mak
>  fi
> -if test "$netmap" = "yes" ; then
> +if test "$netmap" != "no" ; then
>    echo "CONFIG_NETMAP=y" >> $config_host_mak
>  fi
>  if test "$l2tpv3" = "yes" ; then
> diff --git a/netmap b/netmap
> new file mode 160000
> index 0000000000..137f537eae
> --- /dev/null
> +++ b/netmap
> @@ -0,0 +1 @@
> +Subproject commit 137f537eae513f02d5d6871d1f91c049e6345803

Looks reasonable to me.  Please submit it as a patch.