[libvirt PATCH 0/5] meson: Fixes and improvements to handling of qemu_user and qemu_group
Mostly reported/suggested by Olaf. Test pipeline: https://gitlab.com/abologna/libvirt/-/pipelines/456827585 Ignore the macOS failure, which is caused by a transient network error on GitLab's part. I have verified manually that the default values are still the ones we expect. Andrea Bolognani (5): meson: Don't require /etc/os-release to be present meson: Check for os-release's ID_LIKE in addition to ID meson: Don't check whether qemu_user and qemu_group exist meson: Require that both qemu_user and qemu_group are provided meson: Optimize handling of qemu_user and qemu_group meson.build | 68 ++++++++++++++++++++++------------------------------- 1 file changed, 28 insertions(+), 40 deletions(-) -- 2.34.1
It might be part of some non-mandatory package on certain distros, and our logic deals just fine with its contents not being available. Fixes: 4c69d64efa3731d074d198f871fd42e74c4a39f6 Reported-by: Olaf Hering <olaf@aepfle.de> Signed-off-by: Andrea Bolognani <abologna@redhat.com> --- meson.build | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/meson.build b/meson.build index 0f0a71a2da..ab5f7433a6 100644 --- a/meson.build +++ b/meson.build @@ -1659,7 +1659,7 @@ if not get_option('driver_qemu').disabled() default_qemu_user = 'root' default_qemu_group = 'wheel' else - os_release = run_command('grep', '^ID=', '/etc/os-release', check: true).stdout() + os_release = run_command('grep', '^ID=', '/etc/os-release', check: false).stdout() if os_release.contains('arch') default_qemu_user = 'nobody' default_qemu_group = 'nobody' -- 2.34.1
On Wed, Jan 26, 2022 at 04:13:12PM +0100, Andrea Bolognani wrote:
It might be part of some non-mandatory package on certain distros, and our logic deals just fine with its contents not being available.
Fixes: 4c69d64efa3731d074d198f871fd42e74c4a39f6 Reported-by: Olaf Hering <olaf@aepfle.de> Signed-off-by: Andrea Bolognani <abologna@redhat.com> --- meson.build | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
Reviewed-by: Pavel Hrdina <phrdina@redhat.com>
This makes is possible to reduce the number of cases we have to consider, because 'sles' declares itself to be like 'suse' and both 'rhel' and 'centos' declare themselves to be like 'fedora'. We have to move the check for Ubuntu before the one for Debian, however, because 'ubuntu' declares itself to be like 'debian' and it would end up with the wrong defaults otherwise. Suggested-by: Olaf Hering <olaf@aepfle.de> Signed-off-by: Andrea Bolognani <abologna@redhat.com> --- meson.build | 13 +++++-------- 1 file changed, 5 insertions(+), 8 deletions(-) diff --git a/meson.build b/meson.build index ab5f7433a6..388e2cfa5e 100644 --- a/meson.build +++ b/meson.build @@ -1659,24 +1659,21 @@ if not get_option('driver_qemu').disabled() default_qemu_user = 'root' default_qemu_group = 'wheel' else - os_release = run_command('grep', '^ID=', '/etc/os-release', check: false).stdout() + os_release = run_command('grep', '-E', '^ID(_LIKE)*=', '/etc/os-release', check: false).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 + elif (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' + elif os_release.contains('debian') + default_qemu_user = 'libvirt-qemu' + default_qemu_group = 'libvirt-qemu' else default_qemu_user = 'root' default_qemu_group = 'root' -- 2.34.1
On Wed, Jan 26, 2022 at 04:13:13PM +0100, Andrea Bolognani wrote:
This makes is possible to reduce the number of cases we have to
s/is/it/
consider, because 'sles' declares itself to be like 'suse' and both 'rhel' and 'centos' declare themselves to be like 'fedora'.
We have to move the check for Ubuntu before the one for Debian, however, because 'ubuntu' declares itself to be like 'debian' and it would end up with the wrong defaults otherwise.
Suggested-by: Olaf Hering <olaf@aepfle.de> Signed-off-by: Andrea Bolognani <abologna@redhat.com> --- meson.build | 13 +++++-------- 1 file changed, 5 insertions(+), 8 deletions(-)
diff --git a/meson.build b/meson.build index ab5f7433a6..388e2cfa5e 100644 --- a/meson.build +++ b/meson.build @@ -1659,24 +1659,21 @@ if not get_option('driver_qemu').disabled() default_qemu_user = 'root' default_qemu_group = 'wheel' else - os_release = run_command('grep', '^ID=', '/etc/os-release', check: false).stdout() + os_release = run_command('grep', '-E', '^ID(_LIKE)*=', '/etc/os-release', check: false).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 + elif (os_release.contains('fedora') or os_release.contains('gentoo') or - os_release.contains('rhel') or - os_release.contains('sles') or os_release.contains('suse'))
I would add comments here, for example: # Check for Fedora like OSes which includes CentOS, RHEL and similar. And the same for SUSE and SLES.
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' + elif os_release.contains('debian') + default_qemu_user = 'libvirt-qemu' + default_qemu_group = 'libvirt-qemu'
This should be commented as well as we know that not everyone checks the commit message so it's better to have it tied to the code itself.
else default_qemu_user = 'root' default_qemu_group = 'root'
In general I like this idea and at the same time I don't like it at all :D. It will catch more OSes based on fedora/ubuntu/suse but at the same time it might cause issues in the future if something changes. Let's see what others think about it. Pavel
On Thu, Jan 27, 2022 at 11:05:49AM +0100, Pavel Hrdina wrote:
On Wed, Jan 26, 2022 at 04:13:13PM +0100, Andrea Bolognani wrote:
+ elif (os_release.contains('fedora') or os_release.contains('gentoo') or os_release.contains('suse'))
I would add comments here, for example:
# Check for Fedora like OSes which includes CentOS, RHEL and similar.
And the same for SUSE and SLES.
Yeah, good idea.
In general I like this idea and at the same time I don't like it at all :D. It will catch more OSes based on fedora/ubuntu/suse but at the same time it might cause issues in the future if something changes.
I think it's an improvement because it avoids the need to keep track of every single distribution, which is exactly what the ID_LIKE field is supposed to help with. As an example, Rocky Linux is a RHEL rebuild which has ID_LIKE=fedora, and so after this change it gets good defaults instead of the root:root fallback. If we later find out that some derivative uses different defaults than the distro it's based on we can always special-case it, like we're doing with Ubuntu today. I've posted a v2 which adds comments. -- Andrea Bolognani / Red Hat / Virtualization
QEMU might not be installed on the build system, in which case the user and group will not be present. We should avoid falling back to root:root in that case, and assume the user and group are going to be present in the target system instead. Suggested-by: Olaf Hering <olaf@aepfle.de> Signed-off-by: Andrea Bolognani <abologna@redhat.com> --- meson.build | 8 -------- 1 file changed, 8 deletions(-) diff --git a/meson.build b/meson.build index 388e2cfa5e..f08a25f53c 100644 --- a/meson.build +++ b/meson.build @@ -1678,14 +1678,6 @@ if not get_option('driver_qemu').disabled() 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_user = get_option('qemu_user') if qemu_user == '' -- 2.34.1
On Wed, Jan 26, 2022 at 04:13:14PM +0100, Andrea Bolognani wrote:
QEMU might not be installed on the build system, in which case the user and group will not be present. We should avoid falling back to root:root in that case, and assume the user and group are going to be present in the target system instead.
Suggested-by: Olaf Hering <olaf@aepfle.de> Signed-off-by: Andrea Bolognani <abologna@redhat.com> --- meson.build | 8 -------- 1 file changed, 8 deletions(-)
Reviewed-by: Pavel Hrdina <phrdina@redhat.com>
Or that neither is. The current implementation, where if only one of the two is provided the other one will be based on OS-specific defaults is more likely to cause confusion than it is to be helpful. Signed-off-by: Andrea Bolognani <abologna@redhat.com> --- meson.build | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/meson.build b/meson.build index f08a25f53c..36ebe9aa2a 100644 --- a/meson.build +++ b/meson.build @@ -1680,10 +1680,13 @@ if not get_option('driver_qemu').disabled() endif endif qemu_user = get_option('qemu_user') + qemu_group = get_option('qemu_group') + if (qemu_user == '' and qemu_group != '') or (qemu_user != '' and qemu_group == '') + error('Please specify both qemu_user and qemu_group or neither of them') + endif if qemu_user == '' qemu_user = default_qemu_user endif - qemu_group = get_option('qemu_group') if qemu_group == '' qemu_group = default_qemu_group endif -- 2.34.1
On Wed, Jan 26, 2022 at 04:13:15PM +0100, Andrea Bolognani wrote:
Or that neither is. The current implementation, where if only one of the two is provided the other one will be based on OS-specific defaults is more likely to cause confusion than it is to be helpful.
Signed-off-by: Andrea Bolognani <abologna@redhat.com> --- meson.build | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-)
Reviewed-by: Pavel Hrdina <phrdina@redhat.com>
If the user has already provided us with the configuration they want, there's no point in trying to come up with a reasonable OS-specific default. Suggested-by: Olaf Hering <olaf@aepfle.de> Signed-off-by: Andrea Bolognani <abologna@redhat.com> --- meson.build | 54 +++++++++++++++++++++++++---------------------------- 1 file changed, 25 insertions(+), 29 deletions(-) diff --git a/meson.build b/meson.build index 36ebe9aa2a..0e35d64fd1 100644 --- a/meson.build +++ b/meson.build @@ -1655,40 +1655,36 @@ if not get_option('driver_qemu').disabled() endif conf.set_quoted('QEMU_DATADIR', qemu_datadir) - if host_machine.system() in [ 'freebsd', 'darwin' ] - default_qemu_user = 'root' - default_qemu_group = 'wheel' - else - os_release = run_command('grep', '-E', '^ID(_LIKE)*=', '/etc/os-release', check: false).stdout() - if os_release.contains('arch') - default_qemu_user = 'nobody' - default_qemu_group = 'nobody' - elif (os_release.contains('fedora') or - os_release.contains('gentoo') or - os_release.contains('suse')) - default_qemu_user = 'qemu' - default_qemu_group = 'qemu' - elif os_release.contains('ubuntu') - default_qemu_user = 'libvirt-qemu' - default_qemu_group = 'kvm' - elif os_release.contains('debian') - default_qemu_user = 'libvirt-qemu' - default_qemu_group = 'libvirt-qemu' - else - default_qemu_user = 'root' - default_qemu_group = 'root' - endif - endif qemu_user = get_option('qemu_user') qemu_group = get_option('qemu_group') if (qemu_user == '' and qemu_group != '') or (qemu_user != '' and qemu_group == '') error('Please specify both qemu_user and qemu_group or neither of them') endif - if qemu_user == '' - qemu_user = default_qemu_user - endif - if qemu_group == '' - qemu_group = default_qemu_group + if qemu_user == '' and qemu_group == '' + if host_machine.system() in [ 'freebsd', 'darwin' ] + qemu_user = 'root' + qemu_group = 'wheel' + else + os_release = run_command('grep', '-E', '^ID(_LIKE)*=', '/etc/os-release', check: false).stdout() + if os_release.contains('arch') + qemu_user = 'nobody' + qemu_group = 'nobody' + elif (os_release.contains('fedora') or + os_release.contains('gentoo') or + os_release.contains('suse')) + qemu_user = 'qemu' + qemu_group = 'qemu' + elif os_release.contains('ubuntu') + qemu_user = 'libvirt-qemu' + qemu_group = 'kvm' + elif os_release.contains('debian') + qemu_user = 'libvirt-qemu' + qemu_group = 'libvirt-qemu' + else + qemu_user = 'root' + qemu_group = 'root' + endif + endif endif conf.set_quoted('QEMU_USER', qemu_user) conf.set_quoted('QEMU_GROUP', qemu_group) -- 2.34.1
On Wed, Jan 26, 2022 at 04:13:16PM +0100, Andrea Bolognani wrote:
If the user has already provided us with the configuration they want, there's no point in trying to come up with a reasonable OS-specific default.
Suggested-by: Olaf Hering <olaf@aepfle.de> Signed-off-by: Andrea Bolognani <abologna@redhat.com> --- meson.build | 54 +++++++++++++++++++++++++---------------------------- 1 file changed, 25 insertions(+), 29 deletions(-)
Reviewed-by: Pavel Hrdina <phrdina@redhat.com>
participants (2)
-
Andrea Bolognani -
Pavel Hrdina