Re: [libvirt PATCH 00/17] drop meson checks for runtime binaries
On Mon, Apr 19, 2021 at 11:10 AM Pavel Hrdina wrote: > > On Mon, Apr 19, 2021 at 01:59:52PM +0100, Daniel P. Berrangé wrote: > > On Mon, Apr 19, 2021 at 02:50:13PM +0200, Michal Privoznik wrote: > > > On 4/19/21 2:24 PM, Neal Gompa wrote: > > > > On Fri, Apr 16, 2021 at 3:13 PM Pavel Hrdina wrote: > > > > > > > > > > > > > > > > > > I don't think this is a good "cleanup" to do. Having these checks is > > > > useful since without them, we'd blindly build modules that possibly > > > > wouldn't work because we haven't verified that those dependencies > > > > exist. People do install from source into runtime (I don't, but people > > > > do), and it's useful for making sure all the necessary dependencies > > > > are captured for runtime use at build-time for package builds (I've > > > > caught mistakes because of these). > > > > > > To be fair though, some cleanups Pavel did are worth merging (e.g. couple > > > of > > > first patches that fix comments or remove unused functions) regardless of > > > ... > > > > > > > > > > > So I NACK the whole series. > > > > > > > > > > > > > > this NACK. What I am worried about is that usually, when a distro builds > > > libvirt package a path to a runtime binary will be recorded (e.g. DNSMASQ > > > will be expanded to /usr/sbin/dnsmasq and compiled in). This way, we will > > > try to find "dnsmasq" in PATH, which may work for qemu:///system, but may > > > lead to unexpected results for qemu:///session because for instance I > > > override PATH for my regular user so that a directory with my helper > > > scripts > > > comes first. Let's hope that I won't pick wrong name for my scripts. > > > > I think that's actually the desirable situation for libvirtd running > > as non-root. If the user overrides a system binary with an alternate > > impl, it is right that we honour that. > > Agreed. > > > I think we ought to consider two parts to this series - honouring $PATH, > > and probing at meson time. We can have the former, without changing > > the latter, so we still get feature auto-detection at build time to > > avoid uncessarily building stuff that's not appropriate for the OS > > in question. > > I was about to propose that as well. We can have check for some binaries > in meson to figure out if something should be enabled or not for > features that have value set to "auto" so by default meson would do the > "right" thing. > > But if a user/developer explicitly enables some feature, for example > iscsi storage we should not error out if the binary is missing because > the code can be compiled without the binary. This would also allow > package maintainers to reduce a list of build dependencies that are > required. > > I'll post a v2 with this approach. 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. -- 真実はいつも一つ!/ Always, there's only one truth!
Re: [libvirt PATCH 00/17] drop meson checks for runtime binaries
On Mon, Apr 19, 2021 at 01:59:52PM +0100, Daniel P. Berrangé wrote: > On Mon, Apr 19, 2021 at 02:50:13PM +0200, Michal Privoznik wrote: > > On 4/19/21 2:24 PM, Neal Gompa wrote: > > > On Fri, Apr 16, 2021 at 3:13 PM Pavel Hrdina wrote: > > > > > > > > > > > > > > I don't think this is a good "cleanup" to do. Having these checks is > > > useful since without them, we'd blindly build modules that possibly > > > wouldn't work because we haven't verified that those dependencies > > > exist. People do install from source into runtime (I don't, but people > > > do), and it's useful for making sure all the necessary dependencies > > > are captured for runtime use at build-time for package builds (I've > > > caught mistakes because of these). > > > > To be fair though, some cleanups Pavel did are worth merging (e.g. couple of > > first patches that fix comments or remove unused functions) regardless of > > ... > > > > > > > > So I NACK the whole series. > > > > > > > > > > this NACK. What I am worried about is that usually, when a distro builds > > libvirt package a path to a runtime binary will be recorded (e.g. DNSMASQ > > will be expanded to /usr/sbin/dnsmasq and compiled in). This way, we will > > try to find "dnsmasq" in PATH, which may work for qemu:///system, but may > > lead to unexpected results for qemu:///session because for instance I > > override PATH for my regular user so that a directory with my helper scripts > > comes first. Let's hope that I won't pick wrong name for my scripts. > > I think that's actually the desirable situation for libvirtd running > as non-root. If the user overrides a system binary with an alternate > impl, it is right that we honour that. Agreed. > I think we ought to consider two parts to this series - honouring $PATH, > and probing at meson time. We can have the former, without changing > the latter, so we still get feature auto-detection at build time to > avoid uncessarily building stuff that's not appropriate for the OS > in question. I was about to propose that as well. We can have check for some binaries in meson to figure out if something should be enabled or not for features that have value set to "auto" so by default meson would do the "right" thing. But if a user/developer explicitly enables some feature, for example iscsi storage we should not error out if the binary is missing because the code can be compiled without the binary. This would also allow package maintainers to reduce a list of build dependencies that are required. I'll post a v2 with this approach. Thanks Pavel signature.asc Description: PGP signature
Re: [libvirt PATCH 00/17] drop meson checks for runtime binaries
On Mon, Apr 19, 2021 at 02:50:13PM +0200, Michal Privoznik wrote: > On 4/19/21 2:24 PM, Neal Gompa wrote: > > On Fri, Apr 16, 2021 at 3:13 PM Pavel Hrdina wrote: > > > > > > > > > > I don't think this is a good "cleanup" to do. Having these checks is > > useful since without them, we'd blindly build modules that possibly > > wouldn't work because we haven't verified that those dependencies > > exist. People do install from source into runtime (I don't, but people > > do), and it's useful for making sure all the necessary dependencies > > are captured for runtime use at build-time for package builds (I've > > caught mistakes because of these). > > To be fair though, some cleanups Pavel did are worth merging (e.g. couple of > first patches that fix comments or remove unused functions) regardless of > ... > > > > > So I NACK the whole series. > > > > > > this NACK. What I am worried about is that usually, when a distro builds > libvirt package a path to a runtime binary will be recorded (e.g. DNSMASQ > will be expanded to /usr/sbin/dnsmasq and compiled in). This way, we will > try to find "dnsmasq" in PATH, which may work for qemu:///system, but may > lead to unexpected results for qemu:///session because for instance I > override PATH for my regular user so that a directory with my helper scripts > comes first. Let's hope that I won't pick wrong name for my scripts. I think that's actually the desirable situation for libvirtd running as non-root. If the user overrides a system binary with an alternate impl, it is right that we honour that. I think we ought to consider two parts to this series - honouring $PATH, and probing at meson time. We can have the former, without changing the latter, so we still get feature auto-detection at build time to avoid uncessarily building stuff that's not appropriate for the OS in question. 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 :|
Re: [libvirt PATCH 00/17] drop meson checks for runtime binaries
On 4/19/21 2:24 PM, Neal Gompa wrote: On Fri, Apr 16, 2021 at 3:13 PM Pavel Hrdina wrote: I don't think this is a good "cleanup" to do. Having these checks is useful since without them, we'd blindly build modules that possibly wouldn't work because we haven't verified that those dependencies exist. People do install from source into runtime (I don't, but people do), and it's useful for making sure all the necessary dependencies are captured for runtime use at build-time for package builds (I've caught mistakes because of these). To be fair though, some cleanups Pavel did are worth merging (e.g. couple of first patches that fix comments or remove unused functions) regardless of ... So I NACK the whole series. this NACK. What I am worried about is that usually, when a distro builds libvirt package a path to a runtime binary will be recorded (e.g. DNSMASQ will be expanded to /usr/sbin/dnsmasq and compiled in). This way, we will try to find "dnsmasq" in PATH, which may work for qemu:///system, but may lead to unexpected results for qemu:///session because for instance I override PATH for my regular user so that a directory with my helper scripts comes first. Let's hope that I won't pick wrong name for my scripts. Michal
Re: [libvirt PATCH 00/17] drop meson checks for runtime binaries
On Fri, Apr 16, 2021 at 3:13 PM Pavel Hrdina wrote: > > Recent attempt to add a lot of meson options to specify different > runtime paths motivated me enough to cleanup this from meson. > > Pavel Hrdina (17): > 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: optional_programs should be used only for building libvirt > > meson.build | 180 +- > src/bhyve/bhyve_command.c | 4 + > src/libvirt_private.syms | 6 +- > src/locking/lock_driver_lockd.c | 12 +- > src/network/bridge_driver.c | 8 +- > src/node_device/node_device_driver.c | 2 + > src/qemu/qemu_conf.c | 23 ++- > src/qemu/qemu_domain.c| 3 +- > src/storage/storage_backend_fs.c | 24 +-- > src/storage/storage_backend_logical.c | 13 ++ > src/storage/storage_backend_sheepdog.c| 2 + > src/storage/storage_backend_zfs.c | 3 + > src/storage/storage_util.c| 2 + > src/storage/storage_util.h| 6 + > src/util/virdnsmasq.c | 56 +- > src/util/virdnsmasq.h | 8 +- > src/util/virfile.c| 16 +- > src/util/virfile.h| 6 +- > src/util/virfirewall.c| 4 +- > src/util/virfirewall.h| 4 + > src/util/viriscsi.h | 2 + > src/util/virkmod.h| 3 + > src/util/virnetdev.c | 46 - > src/util/virnetdev.h | 4 - > src/util/virnetdevbandwidth.c | 50 + > src/util/virnetdevbandwidth.h | 6 + > src/util/virnetdevip.c| 2 + > src/util/virnetdevmidonet.c | 2 + > src/util/virnetdevopenvswitch.c | 2 + > src/util/virnuma.c| 1 + > src/util/virsysinfo.c | 1 + > src/util/virutil.c| 2 + > .../bhyvexml2argv-acpiapic.args | 2 +- > .../bhyvexml2argv-acpiapic.ldargs | 2 +- > ...ml2argv-addr-isa-controller-on-slot-1.args | 2 +- > ...2argv-addr-isa-controller-on-slot-1.ldargs | 2 +- > ...l2argv-addr-isa-controller-on-slot-31.args | 2 +- > ...argv-addr-isa-controller-on-slot-31.ldargs | 2 +- > ...xml2argv-addr-more-than-32-sata-disks.args | 2 +- > ...l2argv-addr-more-than-32-sata-disks.ldargs | 2 +- > ...hyvexml2argv-addr-multiple-sata-disks.args | 2 +- > ...vexml2argv-addr-multiple-sata-disks.ldargs | 2 +- > ...vexml2argv-addr-multiple-virtio-disks.args | 2 +- > ...xml2argv-addr-multiple-virtio-disks.ldargs | 2 +- > ...rgv-addr-no32devs-multiple-sata-disks.args | 2 +- > ...v-addr-no32devs-multiple-sata-disks.ldargs | 2 +- > ...l2argv-addr-no32devs-single-sata-disk.args | 2 +- > ...argv-addr-no32devs-single-sata-disk.ldargs | 2 +- > ...rgv-addr-non-isa-controller-on-slot-1.args | 2 +- > .../bhyvexml2argv-addr-single-sata-disk.args | 2 +- > ...bhyvexml2argv-addr-single-sata-disk.ldargs | 2 +- > ...bhyvexml2argv-addr-single-virtio-disk.args | 2 +- > ...yvexml2argv-addr-single-virtio-disk.ldargs | 2 +- > .../bhyvexml2argvdata/bhyvexml2argv-base.args | 2 +- > .../bhyvexml2argv-base.ldargs | 2 +- > .../bhyvexml2argv-bhyveload-bootorder.args| 2 +- > .../bhyvexml2argv-bhyveload-bootorder.ldargs | 2 +- > .../bhyvexml2argv-bhyveload-bootorder1.args | 2 +- > .../bhyvexml2argv-bhyveload-bootorder1.ldargs | 2 +- > .../bhyvexml2argv-bhyveload-bootorder3.args | 2 +- > .../bhyvexml2argv-bhyveload-bootorder3.ldargs | 2 +- > .../bhyvexml2argv-bhyveload-explicitargs.args | 2 +- > ...hyvexml2argv-bhyveload-explicitargs.ldargs | 2 +- > .../bhyvexml2argv-commandline.args| 2 +- >
[libvirt PATCH 00/17] drop meson checks for runtime binaries
Recent attempt to add a lot of meson options to specify different runtime paths motivated me enough to cleanup this from meson. Pavel Hrdina (17): 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: optional_programs should be used only for building libvirt meson.build | 180 +- src/bhyve/bhyve_command.c | 4 + src/libvirt_private.syms | 6 +- src/locking/lock_driver_lockd.c | 12 +- src/network/bridge_driver.c | 8 +- src/node_device/node_device_driver.c | 2 + src/qemu/qemu_conf.c | 23 ++- src/qemu/qemu_domain.c| 3 +- src/storage/storage_backend_fs.c | 24 +-- src/storage/storage_backend_logical.c | 13 ++ src/storage/storage_backend_sheepdog.c| 2 + src/storage/storage_backend_zfs.c | 3 + src/storage/storage_util.c| 2 + src/storage/storage_util.h| 6 + src/util/virdnsmasq.c | 56 +- src/util/virdnsmasq.h | 8 +- src/util/virfile.c| 16 +- src/util/virfile.h| 6 +- src/util/virfirewall.c| 4 +- src/util/virfirewall.h| 4 + src/util/viriscsi.h | 2 + src/util/virkmod.h| 3 + src/util/virnetdev.c | 46 - src/util/virnetdev.h | 4 - src/util/virnetdevbandwidth.c | 50 + src/util/virnetdevbandwidth.h | 6 + src/util/virnetdevip.c| 2 + src/util/virnetdevmidonet.c | 2 + src/util/virnetdevopenvswitch.c | 2 + src/util/virnuma.c| 1 + src/util/virsysinfo.c | 1 + src/util/virutil.c| 2 + .../bhyvexml2argv-acpiapic.args | 2 +- .../bhyvexml2argv-acpiapic.ldargs | 2 +- ...ml2argv-addr-isa-controller-on-slot-1.args | 2 +- ...2argv-addr-isa-controller-on-slot-1.ldargs | 2 +- ...l2argv-addr-isa-controller-on-slot-31.args | 2 +- ...argv-addr-isa-controller-on-slot-31.ldargs | 2 +- ...xml2argv-addr-more-than-32-sata-disks.args | 2 +- ...l2argv-addr-more-than-32-sata-disks.ldargs | 2 +- ...hyvexml2argv-addr-multiple-sata-disks.args | 2 +- ...vexml2argv-addr-multiple-sata-disks.ldargs | 2 +- ...vexml2argv-addr-multiple-virtio-disks.args | 2 +- ...xml2argv-addr-multiple-virtio-disks.ldargs | 2 +- ...rgv-addr-no32devs-multiple-sata-disks.args | 2 +- ...v-addr-no32devs-multiple-sata-disks.ldargs | 2 +- ...l2argv-addr-no32devs-single-sata-disk.args | 2 +- ...argv-addr-no32devs-single-sata-disk.ldargs | 2 +- ...rgv-addr-non-isa-controller-on-slot-1.args | 2 +- .../bhyvexml2argv-addr-single-sata-disk.args | 2 +- ...bhyvexml2argv-addr-single-sata-disk.ldargs | 2 +- ...bhyvexml2argv-addr-single-virtio-disk.args | 2 +- ...yvexml2argv-addr-single-virtio-disk.ldargs | 2 +- .../bhyvexml2argvdata/bhyvexml2argv-base.args | 2 +- .../bhyvexml2argv-base.ldargs | 2 +- .../bhyvexml2argv-bhyveload-bootorder.args| 2 +- .../bhyvexml2argv-bhyveload-bootorder.ldargs | 2 +- .../bhyvexml2argv-bhyveload-bootorder1.args | 2 +- .../bhyvexml2argv-bhyveload-bootorder1.ldargs | 2 +- .../bhyvexml2argv-bhyveload-bootorder3.args | 2 +- .../bhyvexml2argv-bhyveload-bootorder3.ldargs | 2 +- .../bhyvexml2argv-bhyveload-explicitargs.args | 2 +- ...hyvexml2argv-bhyveload-explicitargs.ldargs | 2 +- .../bhyvexml2argv-commandline.args| 2 +- .../bhyvexml2argv-commandline.ldargs | 2 +- ...gv-console-master-slave-not-specified.args | 2 +- ...-console-master-slave-not-specified.ldargs | 2 +- .../bhyvexml2argv-console.args| 2 +-