On Tue, Jan 25, 2022 at 03:38:33PM +0100, Olaf Hering wrote:

Tue, 25 Jan 2022 14:17:23 +0100 Olaf Hering <olaf@aepfle.de>:

...

Commit 4c69d64efa3731d074d198f871fd42e74c4a39f6 revealed the bug, /etc/os-release must exist during build.

Now that a missing ^ID= became fatal, was it actually a good idea to be explicit? I think poking around for clues in ^ID_LIKE= will be more robust, appending 'sles' would be not required.

You're right, /etc/os-release being absent should not result in a build failure. And instead of matching on ID specifically, we could probably use something like grep -E '^ID(_LIKE)*=' /etc/os-release instead and check only a subset of the strings we currently do. RHEL and CentOS for example are both ID_LIKE=fedora. We'd have to be careful about the order in which the Debian and Ubuntu branches appear though. -- Andrea Bolognani / Red Hat / Virtualization

On Tue, Jan 25, 2022 at 06:30:43PM +0100, Olaf Hering wrote:

Tue, 25 Jan 2022 09:16:33 -0800 Andrea Bolognani <abologna@redhat.com>:

...

/etc/os-release being absent should not result in a build failure.

Yes. This should be the first thing to fix.

Commit 29cd1877acd91883df32bf71ec07fe908e96db32 does not say why ID= was used. This grep should be relaxed to ID_LIKE=.

We have to consider *both*, because only looking at ID_LIKE would not work on some distros: $ grep -E '^ID(_LIKE)*=' /etc/os-release ID=fedora $

Furthermore, if both qemu_user and qemu_group are known, no grep is required.

Do you mean that if the user has passed -Dqemu_user=... -Dqemu_group=... to meson we could skip the detection logic? That'd be nice, but I'm afraid it might make the code less readable for very little gain. Are you going to take a stab at these patches? -- Andrea Bolognani / Red Hat / Virtualization

Tue, 25 Jan 2022 10:11:01 -0800 Andrea Bolognani <abologna@redhat.com>:

Do you mean that if the user has passed -Dqemu_user=... -Dqemu_group=... to meson we could skip the detection logic? That'd be nice, but I'm afraid it might make the code less readable for very little gain.

The whole thing will be just a whitespace change because it goes into its own block: qemu_user = get_option('qemu_user') qemu_group = get_option('qemu_group') if qemu_user == '' or qemu_group == '' if host_machine.system() in [ 'freebsd', 'darwin' ] default_qemu_user = 'root' default_qemu_group = 'wheel' else os_release = run_command('grep', '^ID=', '/etc/os-release', check: true).stdout() if os_release.contains('arch') default_qemu_user = 'nobody' default_qemu_group = 'nobody' elif (os_release.contains('centos') or os_release.contains('fedora') or os_release.contains('gentoo') or os_release.contains('rhel') or os_release.contains('sles') or os_release.contains('suse')) default_qemu_user = 'qemu' default_qemu_group = 'qemu' elif os_release.contains('debian') default_qemu_user = 'libvirt-qemu' default_qemu_group = 'libvirt-qemu' elif os_release.contains('ubuntu') default_qemu_user = 'libvirt-qemu' default_qemu_group = 'kvm' else default_qemu_user = 'root' default_qemu_group = 'root' endif # If the expected user and group don't exist, or we haven't hit any # of the cases above bacuse we're running on an unknown OS, the only # sensible fallback is root:root if (run_command('getent', 'passwd', default_qemu_user, check: false).returncode() != 0 and run_command('getent', 'group', default_qemu_group, check: false).returncode() != 0) default_qemu_user = 'root' default_qemu_group = 'root' endif endif qemu_group = default_qemu_group qemu_user = default_qemu_user endif conf.set_quoted('QEMU_USER', qemu_user) conf.set_quoted('QEMU_GROUP', qemu_group) This looks like an opportunity to get rid of default_qemu_user/group. The getent part is bogus because it forces the user/group to be present on the system which is building libvirt. This build system is usually not the system which runs libvirt. Please fix bug this as well. Olaf

Wed, 26 Jan 2022 07:14:34 -0800 Andrea Bolognani <abologna@redhat.com>:

Patches here: https://listman.redhat.com/archives/libvir-list/2022-January/msg01189.html

Looks good at a fist glance, thanks! Olaf