Re: [PATCH] qemu: Add sysusers config file for qemu & kvm user/groups
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-le...@lists.libvirt.org
Re: [PATCH] qemu: Add sysusers config file for qemu & kvm user/groups
On Mon, Feb 05, 2024 at 03:10:41PM +0100, Peter Krempa wrote: > On Fri, Feb 02, 2024 at 18:59:44 -, t...@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 > > Signed-off-by: Timothée Ravier > > --- > > 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 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-le...@lists.libvirt.org
Re: [PATCH] qemu: Add sysusers config file for qemu & kvm user/groups
> 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 Date: Fri Feb 2 18:59:44 2024 +
>
> 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 ", 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
> 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-le...@lists.libvirt.org
Re: [PATCH] qemu: Add sysusers config file for qemu & kvm user/groups
On Fri, Feb 02, 2024 at 18:59:44 -, t...@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
> Signed-off-by: Timothée Ravier
> ---
> 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: t...@siosm.fr
Date: Fri Feb 2 18:59:44 2024 +
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
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-le...@lists.libvirt.org
