Install a systemd sysusers config file for the qemu & kvm user/groups.

We can not use the sysusers_create_compat macro in the RPM specfile to
create those users as we want to keep the specfile standalone and not
relying on additionnal files.

Update the specfile to make the commands closer to what is generated by
the current macro.

See: https://src.fedoraproject.org/rpms/libvirt/pull-request/22
See: https://gitlab.com/libvirt/libvirt/-/merge_requests/319
See: https://docs.fedoraproject.org/en-US/packaging-guidelines/UsersAndGroups/

Based on previous work by: Peter Krempa <pkrempa@redhat.com>
Signed-off-by: Timothée Ravier <tim@siosm.fr>
---
 libvirt.spec.in                     | 21 +++++++++++++--------
 src/qemu/libvirt-qemu.sysusers.conf |  4 ++++
 src/qemu/meson.build                |  7 +++++++
 3 files changed, 24 insertions(+), 8 deletions(-)
 create mode 100644 src/qemu/libvirt-qemu.sysusers.conf

diff --git a/libvirt.spec.in b/libvirt.spec.in
index 8413e3c19a..a411ac6515 100644
--- a/libvirt.spec.in
+++ b/libvirt.spec.in
@@ -1473,6 +1473,7 @@ chmod 600 $RPM_BUILD_ROOT%{_sysconfdir}/libvirt/nwfilter/*.xml
     %if ! %{with_qemu}
 rm -f $RPM_BUILD_ROOT%{_datadir}/augeas/lenses/libvirtd_qemu.aug
 rm -f $RPM_BUILD_ROOT%{_datadir}/augeas/lenses/tests/test_libvirtd_qemu.aug
+rm -f $RPM_BUILD_ROOT%{_sysusersdir}/libvirt-qemu.conf
     %endif
 %find_lang %{name}
 
@@ -1834,16 +1835,19 @@ exit 0
 %pre daemon-driver-qemu
 %libvirt_sysconfig_pre virtqemud
 %libvirt_systemd_unix_pre virtqemud
+
 # We want soft static allocation of well-known ids, as disk images
-# are commonly shared across NFS mounts by id rather than name; see
-# https://fedoraproject.org/wiki/Packaging:UsersAndGroups
-getent group kvm >/dev/null || groupadd -f -g 36 -r kvm
-getent group qemu >/dev/null || groupadd -f -g 107 -r qemu
-if ! getent passwd qemu >/dev/null; then
-  if ! getent passwd 107 >/dev/null; then
-    useradd -r -u 107 -g qemu -G kvm -d / -s /sbin/nologin -c "qemu user" qemu
+# are commonly shared across NFS mounts by id rather than name.
+# See https://docs.fedoraproject.org/en-US/packaging-guidelines/UsersAndGroups/
+# We can not use the sysusers_create_compat macro here as we want to keep the
+# specfile standalone and not relying on additionnal files.
+getent group 'kvm' >/dev/null || groupadd -f -g '36' -r 'kvm' || :
+getent group 'qemu' >/dev/null || groupadd -f -g '107' -r 'qemu' || :
+if ! getent passwd 'qemu' >/dev/null; then
+  if ! getent passwd '107' >/dev/null; then
+    useradd -r -u '107' -g 'qemu' -G 'kvm' -d '/' -s '/sbin/nologin' -c 'qemu user' 'qemu' || :
   else
-    useradd -r -g qemu -G kvm -d / -s /sbin/nologin -c "qemu user" qemu
+    useradd -r -g 'qemu' -G 'kvm' -d '/' -s '/sbin/nologin' -c 'qemu user' 'qemu' || :
   fi
 fi
 exit 0
@@ -2246,6 +2250,7 @@ exit 0
 %{_bindir}/virt-qemu-run
 %{_mandir}/man1/virt-qemu-run.1*
 %{_mandir}/man8/virtqemud.8*
+%{_sysusersdir}/libvirt-qemu.conf
     %endif
 
     %if %{with_lxc}
diff --git a/src/qemu/libvirt-qemu.sysusers.conf b/src/qemu/libvirt-qemu.sysusers.conf
new file mode 100644
index 0000000000..3189191e73
--- /dev/null
+++ b/src/qemu/libvirt-qemu.sysusers.conf
@@ -0,0 +1,4 @@
+g kvm 36
+g qemu 107
+u qemu 107:qemu "qemu user" - -
+m qemu kvm
diff --git a/src/qemu/meson.build b/src/qemu/meson.build
index 4c3e1dee78..7a0e908a66 100644
--- a/src/qemu/meson.build
+++ b/src/qemu/meson.build
@@ -160,6 +160,13 @@ if conf.has('WITH_QEMU')
     configuration: qemu_user_group_hack_conf,
   )
 
+  # Install the sysuser config for the qemu driver
+  install_data(
+    'libvirt-qemu.sysusers.conf',
+    install_dir: prefix / 'lib' / 'sysusers.d',
+    rename: [ 'libvirt-qemu.conf' ],
+  )
+
   virt_conf_files += qemu_conf
   virt_aug_files += files('libvirtd_qemu.aug')
   virt_test_aug_files += {
-- 
2.43.0
_______________________________________________
Devel mailing list -- devel@lists.libvirt.org
To unsubscribe send an email to devel-leave@lists.libvirt.org

On Fri, Feb 02, 2024 at 18:59:44 -0000, tim@siosm.fr wrote:
> Install a systemd sysusers config file for the qemu & kvm user/groups.
> 
> We can not use the sysusers_create_compat macro in the RPM specfile to
> create those users as we want to keep the specfile standalone and not
> relying on additionnal files.
> 
> Update the specfile to make the commands closer to what is generated by
> the current macro.
> 
> See: https://src.fedoraproject.org/rpms/libvirt/pull-request/22
> See: https://gitlab.com/libvirt/libvirt/-/merge_requests/319
> See: https://docs.fedoraproject.org/en-US/packaging-guidelines/UsersAndGroups/

IMO we should also mention:
https://bugzilla.redhat.com/show_bug.cgi?id=2095429

> 
> Based on previous work by: Peter Krempa <pkrempa@redhat.com>
> Signed-off-by: Timothée Ravier <tim@siosm.fr>
> ---
>  libvirt.spec.in                     | 21 +++++++++++++--------
>  src/qemu/libvirt-qemu.sysusers.conf |  4 ++++
>  src/qemu/meson.build                |  7 +++++++
>  3 files changed, 24 insertions(+), 8 deletions(-)
>  create mode 100644 src/qemu/libvirt-qemu.sysusers.conf

I've tested that 'rpmbuild -tb libvirt*.tar.xz' works properly after
this patch.

Note that in the way you've posted this patch the authorship looks like:

commit 746e7b69bf57631ceb2be93e8a0b9db4b4b50e5f
Author: tim@siosm.fr <tim@siosm.fr>
Date:   Fri Feb 2 18:59:44 2024 +0000

    qemu: Add sysusers config file for qemu & kvm user/groups

Is this what you've wanted? Or should I update it using the spelling
from the sign off?

> diff --git a/libvirt.spec.in b/libvirt.spec.in
> index 8413e3c19a..a411ac6515 100644
> --- a/libvirt.spec.in
> +++ b/libvirt.spec.in

[...]

> @@ -1834,16 +1835,19 @@ exit 0
>  %pre daemon-driver-qemu
>  %libvirt_sysconfig_pre virtqemud
>  %libvirt_systemd_unix_pre virtqemud
> +
>  # We want soft static allocation of well-known ids, as disk images
> -# are commonly shared across NFS mounts by id rather than name; see
> -# https://fedoraproject.org/wiki/Packaging:UsersAndGroups
> -getent group kvm >/dev/null || groupadd -f -g 36 -r kvm
> -getent group qemu >/dev/null || groupadd -f -g 107 -r qemu
> -if ! getent passwd qemu >/dev/null; then
> -  if ! getent passwd 107 >/dev/null; then
> -    useradd -r -u 107 -g qemu -G kvm -d / -s /sbin/nologin -c "qemu user" qemu
> +# are commonly shared across NFS mounts by id rather than name.
> +# See https://docs.fedoraproject.org/en-US/packaging-guidelines/UsersAndGroups/
> +# We can not use the sysusers_create_compat macro here as we want to keep the
> +# specfile standalone and not relying on additionnal files.
> +getent group 'kvm' >/dev/null || groupadd -f -g '36' -r 'kvm' || :
> +getent group 'qemu' >/dev/null || groupadd -f -g '107' -r 'qemu' || :
> +if ! getent passwd 'qemu' >/dev/null; then
> +  if ! getent passwd '107' >/dev/null; then
> +    useradd -r -u '107' -g 'qemu' -G 'kvm' -d '/' -s '/sbin/nologin' -c 'qemu user' 'qemu' || :
>    else
> -    useradd -r -g qemu -G kvm -d / -s /sbin/nologin -c "qemu user" qemu
> +    useradd -r -g 'qemu' -G 'kvm' -d '/' -s '/sbin/nologin' -c 'qemu user' 'qemu' || :
>    fi
>  fi
>  exit 0

The quoting changes are okay and result in identical commands, but I'm
not that much sold on the discarding of errors (' || : ') which we
didn't do before. Why would we want to ignore errors here?

Other than that:

Reviewed-by: Peter Krempa <pkrempa@redhat.com>

You don't need to re-send but I want to know the reason for dropping the
errors first. I can update the patch before pushing.
_______________________________________________
Devel mailing list -- devel@lists.libvirt.org
To unsubscribe send an email to devel-leave@lists.libvirt.org

On Mon, Feb 05, 2024 at 03:10:41PM +0100, Peter Krempa wrote:
> On Fri, Feb 02, 2024 at 18:59:44 -0000, tim@siosm.fr wrote:
> > Install a systemd sysusers config file for the qemu & kvm user/groups.
> >
> > We can not use the sysusers_create_compat macro in the RPM specfile to
> > create those users as we want to keep the specfile standalone and not
> > relying on additionnal files.
> >
> > Update the specfile to make the commands closer to what is generated by
> > the current macro.
> >
> > See: https://src.fedoraproject.org/rpms/libvirt/pull-request/22
> > See: https://gitlab.com/libvirt/libvirt/-/merge_requests/319
> > See: https://docs.fedoraproject.org/en-US/packaging-guidelines/UsersAndGroups/
> >
> > Based on previous work by: Peter Krempa <pkrempa@redhat.com>
> > Signed-off-by: Timothée Ravier <tim@siosm.fr>
> > ---
> >  libvirt.spec.in                     | 21 +++++++++++++--------
> >  src/qemu/libvirt-qemu.sysusers.conf |  4 ++++
> >  src/qemu/meson.build                |  7 +++++++
> >  3 files changed, 24 insertions(+), 8 deletions(-)
> >  create mode 100644 src/qemu/libvirt-qemu.sysusers.conf
>
> Reviewed-by: Peter Krempa <pkrempa@redhat.com>

Unfortunately I failed to notice this before it had already made it
into a release...

> > +++ b/src/qemu/libvirt-qemu.sysusers.conf
> > @@ -0,0 +1,4 @@
> > +g kvm 36
> > +g qemu 107
> > +u qemu 107:qemu "qemu user" - -
> > +m qemu kvm

These values are fine for Fedora/RHEL, but not elsewhere. For
example, Debian would need something like

  g libvirt-qemu 64055
  u libvirt-qemu 64055:libvirt-qemu

instead.

If you look at meson.build, you will see that we detect a number of
operating systems/distributions in order to integrate as smoothly as
possible with them. This can potentially break that, or at the very
least make things quite confusing by virtue of more than one "QEMU
user" existing on the system.

Additionally, it completely fails to account for the qemu_user and
qemu_group meson options, which have been around forever and can take
arbitrary values.

At the very least, installing this file needs to be gated behind a
meson option that is off by default. A more complete solution that
integrates properly with the existing facilities will require further
work.

-- 
Andrea Bolognani / Red Hat / Virtualization
_______________________________________________
Devel mailing list -- devel@lists.libvirt.org
To unsubscribe send an email to devel-leave@lists.libvirt.org

Good catch. I've tried to look at fixing this but this requires meson knowledge that I don't have so I haven't been able to focus on it yet. Pointers appreciated.
_______________________________________________
Devel mailing list -- devel@lists.libvirt.org
To unsubscribe send an email to devel-leave@lists.libvirt.org

> IMO we should also mention:
> https://bugzilla.redhat.com/show_bug.cgi?id=2095429

Sure, feel free to add it to the list of references I added.

> Note that in the way you've posted this patch the authorship looks like:
> 
> commit 746e7b69bf57631ceb2be93e8a0b9db4b4b50e5f
> Author: tim(a)siosm.fr <tim(a)siosm.fr&gt;
> Date:   Fri Feb 2 18:59:44 2024 +0000
> 
>     qemu: Add sysusers config file for qemu & kvm user/groups
> 
> Is this what you've wanted? Or should I update it using the spelling
> from the sign off?

It should be "Timothée Ravier <tim@siosm.fr>", not sure why this got altered when I tried sending here.

> The quoting changes are okay and result in identical commands, but I'm
> not that much sold on the discarding of errors (' || : ') which we
> didn't do before. Why would we want to ignore errors here?

This is taken from the output of the macro that I re-ran on the sysusers config file and trimmed to match what we have already.

It's generally not a good idea to fail in a scriptlet as there isn't any option for the user to recover at that point. It's also unlikely to fail but if it does, we should try setting up as much as possible and move on. The user / group creation failures will likely be surfaced at runtime.

> Other than that:
> 
> Reviewed-by: Peter Krempa <pkrempa(a)redhat.com&gt;
> 
> You don't need to re-send but I want to know the reason for dropping the
> errors first. I can update the patch before pushing.

Thanks. Feel free to edit and apply as you wish.
_______________________________________________
Devel mailing list -- devel@lists.libvirt.org
To unsubscribe send an email to devel-leave@lists.libvirt.org