11:13 p.m.
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
11:13 p.m.
New subject: [libvirt PATCH 1/5] meson: Don't require /etc/os-release to be present
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(a)aepfle.de>
Signed-off-by: Andrea Bolognani <abologna(a)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
6:06 p.m.
New subject: [libvirt PATCH 1/5] meson: Don't require /etc/os-release to be present
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(a)aepfle.de>
Signed-off-by: Andrea Bolognani <abologna(a)redhat.com>
---
meson.build | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
Reviewed-by: Pavel Hrdina <phrdina(a)redhat.com>
11:13 p.m.
New subject: [libvirt PATCH 2/5] meson: Check for os-release's ID_LIKE in addition to ID
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(a)aepfle.de>
Signed-off-by: Andrea Bolognani <abologna(a)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
6:05 p.m.
New subject: [libvirt PATCH 2/5] meson: Check for os-release's ID_LIKE in addition to ID
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(a)aepfle.de>
Signed-off-by: Andrea Bolognani <abologna(a)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
7:07 p.m.
New subject: [libvirt PATCH 2/5] meson: Check for os-release's ID_LIKE in addition to ID
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
11:13 p.m.
New subject: [libvirt PATCH 3/5] meson: Don't check whether qemu_user and qemu_group exist
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(a)aepfle.de>
Signed-off-by: Andrea Bolognani <abologna(a)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
6:07 p.m.
New subject: [libvirt PATCH 3/5] meson: Don't check whether qemu_user and qemu_group exist
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(a)aepfle.de>
Signed-off-by: Andrea Bolognani <abologna(a)redhat.com>
---
meson.build | 8 --------
1 file changed, 8 deletions(-)
Reviewed-by: Pavel Hrdina <phrdina(a)redhat.com>
11:13 p.m.
New subject: [libvirt PATCH 4/5] meson: Require that both qemu_user and qemu_group are provided
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(a)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
6:09 p.m.
New subject: [libvirt PATCH 4/5] meson: Require that both qemu_user and qemu_group are provided
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(a)redhat.com>
---
meson.build | 5 ++++-
1 file changed, 4 insertions(+), 1 deletion(-)
Reviewed-by: Pavel Hrdina <phrdina(a)redhat.com>
11:13 p.m.
New subject: [libvirt PATCH 5/5] meson: Optimize handling of qemu_user and qemu_group
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(a)aepfle.de>
Signed-off-by: Andrea Bolognani <abologna(a)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
6:11 p.m.
New subject: [libvirt PATCH 5/5] meson: Optimize handling of qemu_user and qemu_group
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(a)aepfle.de>
Signed-off-by: Andrea Bolognani <abologna(a)redhat.com>
---
meson.build | 54 +++++++++++++++++++++++++----------------------------
1 file changed, 25 insertions(+), 29 deletions(-)
Reviewed-by: Pavel Hrdina <phrdina(a)redhat.com>
1008
days inactive
1009
days old
11 comments
2 participants
participants (2)
-
Andrea Bolognani -
Pavel Hrdina