Re: [libvirt PATCH v2 00/21] cleanup meson checks for runtime binaries

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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@xxxxxxxxxx> 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
> >
> >  libvirt.spec.in                               |  31 ---
> >  meson.build                                   | 214 ++++++------------
> >  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 +-
> >  .../bhyvexml2argv-console.ldargs              |   2 +-
> >  .../bhyvexml2argv-cputopology.args            |   2 +-
> >  .../bhyvexml2argv-cputopology.ldargs          |   2 +-
> >  .../bhyvexml2argv-custom-loader.args          |   2 +-
> >  .../bhyvexml2argv-custom-loader.ldargs        |   2 +-
> >  .../bhyvexml2argv-disk-cdrom-grub.args        |   2 +-
> >  .../bhyvexml2argv-disk-cdrom-grub.ldargs      |   2 +-
> >  .../bhyvexml2argv-disk-cdrom.args             |   2 +-
> >  .../bhyvexml2argv-disk-cdrom.ldargs           |   2 +-
> >  .../bhyvexml2argv-disk-virtio.args            |   2 +-
> >  .../bhyvexml2argv-disk-virtio.ldargs          |   2 +-
> >  .../bhyvexml2argv-firmware-efi.args           |   2 +-
> >  .../bhyvexml2argv-fs-9p-readonly.args         |   2 +-
> >  .../bhyvexml2argv-fs-9p-readonly.ldargs       |   2 +-
> >  .../bhyvexml2argv-fs-9p.args                  |   2 +-
> >  .../bhyvexml2argv-fs-9p.ldargs                |   2 +-
> >  .../bhyvexml2argv-grub-bootorder.args         |   2 +-
> >  .../bhyvexml2argv-grub-bootorder.ldargs       |   2 +-
> >  .../bhyvexml2argv-grub-bootorder2.args        |   2 +-
> >  .../bhyvexml2argv-grub-bootorder2.ldargs      |   2 +-
> >  .../bhyvexml2argv-grub-defaults.args          |   2 +-
> >  .../bhyvexml2argv-grub-defaults.ldargs        |   2 +-
> >  .../bhyvexml2argv-input-xhci-tablet.args      |   2 +-
> >  .../bhyvexml2argv-input-xhci-tablet.ldargs    |   2 +-
> >  .../bhyvexml2argv-isa-controller.args         |   2 +-
> >  .../bhyvexml2argv-isa-controller.ldargs       |   2 +-
> >  .../bhyvexml2argv-localtime.args              |   2 +-
> >  .../bhyvexml2argv-localtime.ldargs            |   2 +-
> >  .../bhyvexml2argv-macaddr.args                |   2 +-
> >  .../bhyvexml2argv-macaddr.ldargs              |   2 +-
> >  .../bhyvexml2argvdata/bhyvexml2argv-msrs.args |   2 +-
> >  .../bhyvexml2argv-msrs.ldargs                 |   2 +-
> >  .../bhyvexml2argv-net-e1000.args              |   2 +-
> >  .../bhyvexml2argv-net-e1000.ldargs            |   2 +-
> >  .../bhyvexml2argv-serial-grub-nocons.args     |   2 +-
> >  .../bhyvexml2argv-serial-grub-nocons.ldargs   |   2 +-
> >  .../bhyvexml2argv-serial-grub.args            |   2 +-
> >  .../bhyvexml2argv-serial-grub.ldargs          |   2 +-
> >  .../bhyvexml2argv-serial.args                 |   2 +-
> >  .../bhyvexml2argv-serial.ldargs               |   2 +-
> >  .../bhyvexml2argv-sound.args                  |   2 +-
> >  .../bhyvexml2argv-sound.ldargs                |   2 +-
> >  .../bhyvexml2argvdata/bhyvexml2argv-uefi.args |   2 +-
> >  .../bhyvexml2argv-vnc-autoport.args           |   2 +-
> >  .../bhyvexml2argv-vnc-password.args           |   2 +-
> >  .../bhyvexml2argv-vnc-resolution.args         |   2 +-
> >  .../bhyvexml2argv-vnc-vgaconf-io.args         |   2 +-
> >  .../bhyvexml2argv-vnc-vgaconf-off.args        |   2 +-
> >  .../bhyvexml2argv-vnc-vgaconf-on.args         |   2 +-
> >  .../bhyvexml2argvdata/bhyvexml2argv-vnc.args  |   2 +-
> >  .../bhyvexml2argv-wired.args                  |   2 +-
> >  .../bhyvexml2argv-wired.ldargs                |   2 +-
> >  tests/bhyvexml2argvtest.c                     |   4 +-
> >  tests/domaincapsmock.c                        |  17 ++
> >  tests/meson.build                             |   1 +
> >  tests/networkxml2conftest.c                   |   6 +-
> >  tests/networkxml2firewalltest.c               |  16 +-
> >  tests/nwfilterebiptablestest.c                |  15 +-
> >  tests/nwfilterxml2firewalltest.c              |  14 +-
> >  tests/qemuxml2argvmock.c                      |   5 +-
> >  tests/testutilsqemu.c                         |  15 --
> >  tests/virfirewallmock.c                       |  35 +++
> >  tests/virfirewalltest.c                       |  15 +-
> >  132 files changed, 403 insertions(+), 484 deletions(-)
> >  create mode 100644 tests/virfirewallmock.c
> >
> > --
> > 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.

> 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.

> To make it clear, I still NACK the series, because I think this is
> fundamentally a bad idea.

It was already pointed out that the series have other patches fixing
issues not related to the binary path detection, NACKing the whole
series is ignoring the fact.

To summarize it I don't see any strong reason why this series shouldn't
be accepted.

Pavel

Attachment: signature.asc
Description: PGP signature


[Index of Archives]     [Virt Tools]     [Libvirt Users]     [Lib OS Info]     [Fedora Users]     [Fedora Desktop]     [Fedora SELinux]     [Big List of Linux Books]     [Yosemite News]     [KDE Users]     [Fedora Tools]

  Powered by Linux