[PATCH 0/3] meson: Drop userfaultfd_sysctl option
Recently introduced as part of [1]. We can skip the meson part for this one, and deal with everything in the spec file only. Test pipeline: https://gitlab.com/abologna/libvirt/-/pipelines/1176890275 [1] https://lists.libvirt.org/archives/list/devel@lists.libvirt.org/thread/DKS4A... Andrea Bolognani (3): rpm: Always enable sysctl_config rpm: Always enable userfaultfd_sysctl meson: Drop userfaultfd_sysctl option libvirt.spec.in | 13 +++++-------- meson.build | 7 ------- meson_options.txt | 1 - src/qemu/meson.build | 2 +- 4 files changed, 6 insertions(+), 17 deletions(-) -- 2.43.0
We currently rely on it being enabled by default. Signed-off-by: Andrea Bolognani <abologna@redhat.com> --- libvirt.spec.in | 1 + 1 file changed, 1 insertion(+) diff --git a/libvirt.spec.in b/libvirt.spec.in index 49ce717e1b..af2ba20c02 100644 --- a/libvirt.spec.in +++ b/libvirt.spec.in @@ -1368,6 +1368,7 @@ export SOURCE_DATE_EPOCH=$(stat --printf='%Y' %{_specdir}/libvirt.spec) -Dqemu_moddir=%{qemu_moddir} \ -Dqemu_datadir=%{qemu_datadir} \ -Dtls_priority=%{tls_priority} \ + -Dsysctl_config=enabled \ %{?arg_userfaultfd_sysctl} \ %{?enable_werror} \ -Dexpensive_tests=enabled \ -- 2.43.0
On Wed, Feb 14, 2024 at 20:53:22 +0100, Andrea Bolognani wrote:
We currently rely on it being enabled by default.
Signed-off-by: Andrea Bolognani <abologna@redhat.com> --- libvirt.spec.in | 1 + 1 file changed, 1 insertion(+)
diff --git a/libvirt.spec.in b/libvirt.spec.in index 49ce717e1b..af2ba20c02 100644 --- a/libvirt.spec.in +++ b/libvirt.spec.in @@ -1368,6 +1368,7 @@ export SOURCE_DATE_EPOCH=$(stat --printf='%Y' %{_specdir}/libvirt.spec) -Dqemu_moddir=%{qemu_moddir} \ -Dqemu_datadir=%{qemu_datadir} \ -Dtls_priority=%{tls_priority} \ + -Dsysctl_config=enabled \ %{?arg_userfaultfd_sysctl} \ %{?enable_werror} \ -Dexpensive_tests=enabled \
Reviewed-by: Jiri Denemark <jdenemar@redhat.com>
Instead of enabling the feature conditionally, always enable it and then remove the installed file when building on a platform that doesn't need it. We already perform the complementary check to decide whether the file should be included in the package. Signed-off-by: Andrea Bolognani <abologna@redhat.com> --- libvirt.spec.in | 12 +++++------- 1 file changed, 5 insertions(+), 7 deletions(-) diff --git a/libvirt.spec.in b/libvirt.spec.in index af2ba20c02..70d2d37502 100644 --- a/libvirt.spec.in +++ b/libvirt.spec.in @@ -1283,12 +1283,6 @@ exit 1 %define arg_remote_mode -Dremote_default_mode=legacy %endif -%if %{with_userfaultfd_sysctl} - %define arg_userfaultfd_sysctl -Duserfaultfd_sysctl=enabled -%else - %define arg_userfaultfd_sysctl -Duserfaultfd_sysctl=disabled -%endif - %define when %(date +"%%F-%%T") %define where %(hostname) %define who %{?packager}%{!?packager:Unknown} @@ -1369,7 +1363,7 @@ export SOURCE_DATE_EPOCH=$(stat --printf='%Y' %{_specdir}/libvirt.spec) -Dqemu_datadir=%{qemu_datadir} \ -Dtls_priority=%{tls_priority} \ -Dsysctl_config=enabled \ - %{?arg_userfaultfd_sysctl} \ + -Duserfaultfd_sysctl=enabled \ %{?enable_werror} \ -Dexpensive_tests=enabled \ -Dinit_script=systemd \ @@ -1518,6 +1512,10 @@ rm -f $RPM_BUILD_ROOT%{_datadir}/augeas/lenses/libvirtd_libxl.aug rm -f $RPM_BUILD_ROOT%{_datadir}/augeas/lenses/tests/test_libvirtd_libxl.aug %endif + %if ! %{with_userfaultfd_sysctl} +rm -f $RPM_BUILD_ROOT%{_prefix}/lib/sysctl.d/60-qemu-postcopy-migration.conf + %endif + # Copied into libvirt-docs subpackage eventually mv $RPM_BUILD_ROOT%{_datadir}/doc/libvirt libvirt-docs -- 2.43.0
It was introduced to make installation of the corresponding file conditional based on the build platform, but we've already changed the spec file so that all the decisions on the matter happen there, which makes having yet another meson option for the purpose unnecessary. Signed-off-by: Andrea Bolognani <abologna@redhat.com> --- libvirt.spec.in | 2 -- meson.build | 7 ------- meson_options.txt | 1 - src/qemu/meson.build | 2 +- 4 files changed, 1 insertion(+), 11 deletions(-) diff --git a/libvirt.spec.in b/libvirt.spec.in index 70d2d37502..b3208eb21e 100644 --- a/libvirt.spec.in +++ b/libvirt.spec.in @@ -1363,7 +1363,6 @@ export SOURCE_DATE_EPOCH=$(stat --printf='%Y' %{_specdir}/libvirt.spec) -Dqemu_datadir=%{qemu_datadir} \ -Dtls_priority=%{tls_priority} \ -Dsysctl_config=enabled \ - -Duserfaultfd_sysctl=enabled \ %{?enable_werror} \ -Dexpensive_tests=enabled \ -Dinit_script=systemd \ @@ -1447,7 +1446,6 @@ export SOURCE_DATE_EPOCH=$(stat --printf='%Y' %{_specdir}/libvirt.spec) -Dstorage_vstorage=disabled \ -Dstorage_zfs=disabled \ -Dsysctl_config=disabled \ - -Duserfaultfd_sysctl=disabled \ -Dtests=disabled \ -Dudev=disabled \ -Dwireshark_dissector=disabled \ diff --git a/meson.build b/meson.build index dbd9cad6df..63983d2a10 100644 --- a/meson.build +++ b/meson.build @@ -2019,12 +2019,6 @@ elif get_option('sysctl_config').enabled() error('sysctl configuration is supported only on linux') endif -if not get_option('userfaultfd_sysctl').disabled() and conf.has('WITH_SYSCTL') - conf.set('WITH_USERFAULTFD_SYSCTL', 1) -elif get_option('userfaultfd_sysctl').enabled() - error('userfaultfd_sysctl option requires sysctl_config to be enabled') -endif - conf.set_quoted('TLS_PRIORITY', get_option('tls_priority')) @@ -2325,7 +2319,6 @@ misc_summary = { 'virt-host-validate': conf.has('WITH_HOST_VALIDATE'), 'TLS priority': conf.get_unquoted('TLS_PRIORITY'), 'sysctl config': conf.has('WITH_SYSCTL'), - 'userfaultfd sysctl': conf.has('WITH_USERFAULTFD_SYSCTL'), } summary(misc_summary, section: 'Miscellaneous', bool_yn: true, list_sep: ' ') diff --git a/meson_options.txt b/meson_options.txt index ed91d97abf..182e28b3d1 100644 --- a/meson_options.txt +++ b/meson_options.txt @@ -108,5 +108,4 @@ option('nbdkit', type: 'feature', value: 'auto', description: 'Build nbdkit stor option('nbdkit_config_default', type: 'feature', value: 'auto', description: 'Whether to use nbdkit storage backend for network disks by default (configurable)') option('pm_utils', type: 'feature', value: 'auto', description: 'use pm-utils for power management') option('sysctl_config', type: 'feature', value: 'auto', description: 'Whether to install sysctl configs') -option('userfaultfd_sysctl', type: 'feature', value: 'auto', description: 'Whether to install sysctl config for enabling unprivileged userfaultfd') option('tls_priority', type: 'string', value: 'NORMAL', description: 'set the default TLS session priority string') diff --git a/src/qemu/meson.build b/src/qemu/meson.build index 907893d431..7a0e908a66 100644 --- a/src/qemu/meson.build +++ b/src/qemu/meson.build @@ -209,7 +209,7 @@ if conf.has('WITH_QEMU') 'in_file': files('virtqemud.init.in'), } - if conf.has('WITH_USERFAULTFD_SYSCTL') + if conf.has('WITH_SYSCTL') install_data( 'postcopy-migration.sysctl', install_dir: prefix / 'lib' / 'sysctl.d', -- 2.43.0
On Wed, Feb 14, 2024 at 20:53:21 +0100, Andrea Bolognani wrote:
Recently introduced as part of [1].
We can skip the meson part for this one, and deal with everything in the spec file only.
Test pipeline: https://gitlab.com/abologna/libvirt/-/pipelines/1176890275
[1] https://lists.libvirt.org/archives/list/devel@lists.libvirt.org/thread/DKS4A...
Except for 1/3 (which I acked) I think this series is a little bit too spec-centric. We can do a lot of things just in the spec file, but having meson options still makes sense for people not building rpms. Whether we use that option in our spec file or handle it differently is another question, but I would vote against removing the meson option. Jirka
On Thu, Feb 15, 2024 at 09:21:15AM +0100, Jiri Denemark wrote:
On Wed, Feb 14, 2024 at 20:53:21 +0100, Andrea Bolognani wrote:
Recently introduced as part of [1].
We can skip the meson part for this one, and deal with everything in the spec file only.
Test pipeline: https://gitlab.com/abologna/libvirt/-/pipelines/1176890275
[1] https://lists.libvirt.org/archives/list/devel@lists.libvirt.org/thread/DKS4A...
Except for 1/3 (which I acked) I think this series is a little bit too spec-centric. We can do a lot of things just in the spec file, but having meson options still makes sense for people not building rpms. Whether we use that option in our spec file or handle it differently is another question, but I would vote against removing the meson option.
It's not always clear-cut. A meson option is definitely necessary when it affects how the code is built, or when we want something to be disabled by default while giving user a convenient way to enable it. Having some non-trivial logic deciding whether or not it should be enabled is also a hint that something should be a meson option. In this case, it's enabled by default and the way to undo its effects after the file is simply to delete a single file. Doesn't quite justify introducing yet another meson option just for it IMO. Anyway, I just put this out there to see how people reacted to it. If the option remains, so be it. I just think we could do without :) -- Andrea Bolognani / Red Hat / Virtualization
participants (2)
-
Andrea Bolognani -
Jiri Denemark