On Tue, Apr 20, 2021 at 12:50:51PM +0200, Pavel Hrdina wrote:
On Tue, Apr 20, 2021 at 05:30:09AM -0400, Neal Gompa wrote:
On Mon, Apr 19, 2021 at 1:14 PM Pavel Hrdina <phrdina@redhat.com> wrote:
Recent attempt to add a lot of meson options to specify different runtime paths motivated me enough to cleanup this from meson.
Changes in v2: - split and rework patch 16/17 to address review comments - added a new patch to cleanup libvirt.spec.in file
Pavel Hrdina (21): bridge_driver: fix comment about dnsmasqCaps virdnsmasq: drop unused dnsmasqCapsNewFromFile function virdnsmasq: drop unused dnsmasqCapsRefresh function virdnsmasq: remove binaryPath argument from dnsmasqCapsNewFromBuffer virdnsmasq: remove binaryPath argument from dnsmasqCapsNewFromBinary virfirewall: use virFindFileInPath instead of virFileIsExecutable tests: introduce virfirewallmock tests: use virfirewallmock instead of hasNetfilterTools virnetdev: move virNetDevSetRootQDisc to virnetdevbandwidth tests: testutilsqemu: move virFindFileInPath into domaincapsmock meson: don't check collie as program for sheepdog bhyvexml2argvtest: use virCommandToStringFull to strip command path storage: use virFindFileInPath to validate presence of mkfs virfile: introduce virFindFileInPathFull() qemu_conf: use virFindFileInPathFull for runtime binaries meson: drop check for runtime binary dependencies meson: move iscsiadm check into storage_iscsi condition meson: stop setting runtime binaries defines during compilation meson: use runtime binaries to only resolve features with "auto" value meson: optional_programs should be used only for building libvirt libvirt.spec: drop no longer required build dependencies
-- 2.30.2
In case my reply on the v1 thread is missed, I'll say it again here, since it still applies here.
So, I think you *shouldn't* do it that way. The problem with doing this is that we can wind up with mismatched capabilities and non-functional libvirt build based on the assumption that things will just "be there" with no check that they will actually be there.
In other words, runtime executable dependencies should be treated *exactly* as they are now, because we have no other avenue to guarantee that things work for a given installation.
I disagree, let me explain why using several possible scenarios:
1) libvirt is compiled by package maintainers to be shipped in some distribution
In this case it is responsibility of the package maintainer to ensure that the build dependencies and runtime dependencies are correctly defined. Users usually don't have an option to remove any dependency without removing the dependent package as well so the assumption that things will just "be there" is correct.
2) libvirt is compiled directly by user from git/tarball
In this situation there is nothing ensuring the correct dependencies are installed for compilation or runtime so everything can breake with or without this series. For example without this series users can easily install the necessary binaries, compile libvirt and uninstall the binaries again which would result in non-functional libvirt features using these binaries.
Another point for this series is that by default (keeping all libvirt defined meson features set to "auto") will behave exactly the same as before this series. The difference comes only once user explicitly enables some libvirt feature without required binaries.
The following list of binaries was always optional and if missing in the system only the name was defined so libvirt would always figure out the full path during runtime:
dmidecode dnsmasq ebtables ip ip6tables iptables mdevctl mm-ctl modprobe ovs-vsctl radvd rmmod scrub tc udevadm
qemu-bridge-helper qemu-pr-helper slirp-helper dbus-daemon
showmount
Before this series if these are available during compilation libvirt would set the full path to be used during runtime but if they are not available it would silently use only the names. After the series it will always use only the names. So the possibility to end up with non-functional libvirt is the same as libvirt assumes that things will just "be there" if they are not during compilation.
For remaining binaries affected by this series we change the behavior to not error out if they are missing but as I've already pointed out we keep the default behavior to not enable the libvirt features requiring these binaries.
bhyve, bhyveclt, bhyveload (driver_bhyve)
parted (storage_disk) mount, umount, mkfs (storage_fs) lvm aliases (storage_lvm) dog (storage_sheepdog) zfs, zpool (storage_zfs)
numad (numad)
Here we will use the same behavior as the previous list of optional binaries.
I would note that over the years users have fairly frequent hit problems where they didn't have a binary present during build, but installed it later, and then complained to us that libvirt didn't use it. This is why alot of the binaries you list in the previous set we changed to be optional at configure time, relying on $PATH at runtime.
The idea that binaries would randomly get replaced because of differences in system and session could also lead to other unexpected issues, but I think those are considerably less likely. I don't think Daniel's idea of this being supported is a good one though. It's remarkably easy to accidentally break things that way, and since GNOME Boxes is a very prominent user of the session connection, I'd rather not see that get broken by accident.
Looking at the list of binaries that will be affected by this series I don't see any that would be used by GNOME Boxes.
Even if it does though, I don't think this is a reason not to do the change. If an unprivileged user is replacing system binaries then it is their responsiblity if their replacements break stuff. I think this is not likely to be a significant problem in the real world though. Regards, Daniel -- |: https://berrange.com -o- https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o- https://fstop138.berrange.com :| |: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|