Re: [PATCH v2 1/2] migration: add build option to support qemu build with qatzip

[Thomas Huth <thuth@redhat.com>]

On 17/04/2023 10.39, you.chen wrote:
> In this patch, we added qatzip build option in the meson_options.txt and meson-buildoptons.sh to support qemu build with qatzip.
> If you installed qatzip and would like to use it for live migration, you could use "--enable-qatzip" during configure, it will check qatzip availablility from the pkg-config list (Please make sure you correctly set PKG_CONFIG_PATH to include qatzip.pc).
> If you simply use "configure" or use "configure --disable-qatzip", qatzip will not be enabled.

When neither specifying --disable-qatzip nor --enable-qatzip, the default 
should be auto-detection, not disablement ... or is this too instable to be 
enabled by default?

> Co-developed-by: dennis.wu <dennis.wu@intel.com>
> 
> Signed-off-by: you.chen <you.chen@intel.com>
> ---
>   meson.build                   | 11 ++++++++++-
>   meson_options.txt             |  2 ++
>   scripts/meson-buildoptions.sh |  3 +++
>   3 files changed, 15 insertions(+), 1 deletion(-)
> 
> diff --git a/meson.build b/meson.build
> index 29f8644d6d..aa7445e29e 100644
> --- a/meson.build
> +++ b/meson.build
> @@ -553,6 +553,13 @@ if have_system or have_tools
>   endif
>   zlib = dependency('zlib', required: true, kwargs: static_kwargs)
>   
> +qatzip = not_found
> +if not get_option('qatzip').auto()

This looks weird - why ignoring the library when the option should be 
auto-detected? I think you likely want something like this instead:

  if not get_option('qatzip').auto() or have_system

... so that the feature gets enabled automatically when system emulator 
binaries are built and the library is available.

> +  qatzip = dependency('qatzip', required: get_option('qatzip'),
> +                         method: 'pkg-config',
> +                         static: enable_static)
> +endif
> +
>   libaio = not_found
>   if not get_option('linux_aio').auto() or have_block
>     libaio = cc.find_library('aio', has_headers: ['libaio.h'],
> @@ -1863,6 +1870,7 @@ config_host_data.set('CONFIG_LIBISCSI', libiscsi.found())
>   config_host_data.set('CONFIG_LIBNFS', libnfs.found())
>   config_host_data.set('CONFIG_LIBSSH', libssh.found())
>   config_host_data.set('CONFIG_LINUX_AIO', libaio.found())
> +config_host_data.set('CONFIG_QATZIP', qatzip.found())
>   config_host_data.set('CONFIG_LINUX_IO_URING', linux_io_uring.found())
>   config_host_data.set('CONFIG_LIBPMEM', libpmem.found())
>   config_host_data.set('CONFIG_NUMA', numa.found())
> @@ -3339,7 +3347,7 @@ libmigration = static_library('migration', sources: migration_files + genh,
>                                 name_suffix: 'fa',
>                                 build_by_default: false)
>   migration = declare_dependency(link_with: libmigration,
> -                               dependencies: [zlib, qom, io])
> +                               dependencies: [qatzip, zlib, qom, io])

Not sure whether this is the right place to add this ... can't you add the 
dependency in the migration/meson.build file instead?

>   softmmu_ss.add(migration)
>   
>   block_ss = block_ss.apply(config_host, strict: false)
> @@ -3986,6 +3994,7 @@ summary_info += {'vde support':       vde}
>   summary_info += {'netmap support':    have_netmap}
>   summary_info += {'l2tpv3 support':    have_l2tpv3}
>   summary_info += {'Linux AIO support': libaio}
> +summary_info += {'qatzip compress support': qatzip}

Could you please keep this closer to the other compression options in the 
summary instead? (lzo, bzip2, zstd, ...)

>   summary_info += {'Linux io_uring support': linux_io_uring}
>   summary_info += {'ATTR/XATTR support': libattr}
>   summary_info += {'RDMA support':      rdma}
> diff --git a/meson_options.txt b/meson_options.txt
> index fc9447d267..ef6d639876 100644
> --- a/meson_options.txt
> +++ b/meson_options.txt
> @@ -191,6 +191,8 @@ option('smartcard', type : 'feature', value : 'auto',
>          description: 'CA smartcard emulation support')
>   option('snappy', type : 'feature', value : 'auto',
>          description: 'snappy compression support')
> +option('qatzip', type : 'feature', value : 'auto',
> +       description: 'qatzip compress support')
>   option('spice', type : 'feature', value : 'auto',
>          description: 'Spice server support')
>   option('spice_protocol', type : 'feature', value : 'auto',
> diff --git a/scripts/meson-buildoptions.sh b/scripts/meson-buildoptions.sh
> index 009fab1515..84d110197d 100644
> --- a/scripts/meson-buildoptions.sh
> +++ b/scripts/meson-buildoptions.sh
> @@ -152,6 +152,7 @@ meson_options_help() {
>     printf "%s\n" '  slirp-smbd      use smbd (at path --smbd=*) in slirp networking'
>     printf "%s\n" '  smartcard       CA smartcard emulation support'
>     printf "%s\n" '  snappy          snappy compression support'
> +  printf "%s\n" '  qatzip          qatzip compression support'
>     printf "%s\n" '  sndio           sndio sound support'
>     printf "%s\n" '  sparse          sparse checker'
>     printf "%s\n" '  spice           Spice server support'
> @@ -332,6 +333,8 @@ _meson_option_parse() {
>       --disable-libvduse) printf "%s" -Dlibvduse=disabled ;;
>       --enable-linux-aio) printf "%s" -Dlinux_aio=enabled ;;
>       --disable-linux-aio) printf "%s" -Dlinux_aio=disabled ;;
> +    --enable-qatzip) printf "%s" -Dqatzip=enabled ;;
> +    --disable-qatzip) printf "%s" -Dqatzip=disabled ;;

The placement here looks rather random ... Could you please run 
scripts/meson-buildoptions.py to create this file instead?

>       --enable-linux-io-uring) printf "%s" -Dlinux_io_uring=enabled ;;
>       --disable-linux-io-uring) printf "%s" -Dlinux_io_uring=disabled ;;
>       --enable-live-block-migration) printf "%s" -Dlive_block_migration=enabled ;;

  Thomas