[Pkg-libvirt-maintainers] Bug#844339: [PATCH v3 2/4] libvirt-daemon-system.{config, templates, postinst, NEWS}: warn if allocated uid/gid is taken

[Guido Günther]

Hi Mauricio,
thanks! See my two inline comments below, we're nearing completion!

On Fri, Nov 18, 2016 at 03:07:08PM -0200, Mauricio Faria de Oliveira wrote:
> If the uid/gid that is allocated for libvirt-qemu (64055)
> is already in use by another user/group, stop/ask user if
> it is OK to continue (e.g., no plans with guest migration
> over NFS) or abort to go fix the problem.
> 
> This warning is only displayed on new installations.  The
> default value is 'yes'/continue/do not abort, thus not to
> disrupt automated installations.

I'm failing to spot how you make sure you only run on new
installations. Shouldn't a:

    if ! getent passwd libvirt-qemu || ! getent group libvirt-qemu; then
        ...
    fi

gurad the whole libvirt-daemon-system.config to detect that we've
already done our job (correctly on not) in a previous installation?

> 
> On existing installations, no warning is displayed - just
> a NEWS file is provided.
> 
> Signed-off-by: Mauricio Faria de Oliveira <mauricfo at linux.vnet.ibm.com>
> ---
>  debian/libvirt-daemon-system.NEWS      | 22 ++++++++++++++++++++++
>  debian/libvirt-daemon-system.config    | 26 ++++++++++++++++++++++++++
>  debian/libvirt-daemon-system.postinst  |  3 +++
>  debian/libvirt-daemon-system.templates | 18 ++++++++++++++++++
>  4 files changed, 69 insertions(+)
>  create mode 100755 debian/libvirt-daemon-system.config
>  create mode 100644 debian/libvirt-daemon-system.templates
> 
> diff --git a/debian/libvirt-daemon-system.NEWS b/debian/libvirt-daemon-system.NEWS
> index 977abdb..94367e5 100644
> --- a/debian/libvirt-daemon-system.NEWS
> +++ b/debian/libvirt-daemon-system.NEWS
> @@ -1,3 +1,25 @@
> +libvirt (2.4.0-2uidgid3) UNRELEASED; urgency=medium
> +
> +  libvirt-daemon-system now uses the allocated uid and gid 64055
> +  for the libvirt-qemu user and group on new installations, when
> +  the uid/gid is available (otherwise a debconf warning is shown).
> +
> +  On existing installations, which have different uid/gid values
> +  assigned, the recommended procedure is to reassign the uid/gid
> +  (might require considerations for ownership/permission changes).
> +  No debconf warning is shown in this case; only this NEWS entry.
> +
> +  This change is in order to prevent I/O errors during migration
> +  of guests with disk image files shared over NFS, caused by the
> +  different uid/gid ownership between the source and destination
> +  host systems, which leads to access/permission errors with NFS.
> +
> +  If guest migration over NFS is not a requirement in the system,
> +  there should not be any impact to the guests for not using the
> +  allocated uid/gid.
> +
> + -- Mauricio Faria de Oliveira <mauricfo at linux.vnet.ibm.com>  Thu, 18 Nov 2016 13:56:38 -0200
> +
>  libvirt (1.2.9~rc1-1) experimental; urgency=medium
>  
>    libvirtd now uses PolicyKit instead of unix socket domain permissions for r/w
> diff --git a/debian/libvirt-daemon-system.config b/debian/libvirt-daemon-system.config
> new file mode 100755
> index 0000000..caf0ac2
> --- /dev/null
> +++ b/debian/libvirt-daemon-system.config
> @@ -0,0 +1,26 @@
> +#!/bin/sh -e
> +
> +# Source debconf library.
> +. /usr/share/debconf/confmodule
> +
> +# Allocated UID and GID for libvirt-qemu
> +LIBVIRT_QEMU_UID=64055
> +LIBVIRT_QEMU_GID=64055
> +
> +# Check if allocated UID/GID are assigned to a different user/group.
> +UID_TO_NAME="$(getent passwd $LIBVIRT_QEMU_UID | cut -d: -f1)"
> +GID_TO_NAME="$(getent group  $LIBVIRT_QEMU_GID | cut -d: -f1)"
> +
> +if ( [ -n "$UID_TO_NAME" ] && [ "$UID_TO_NAME" != 'libvirt-qemu' ] ) \
> +|| ( [ -n "$GID_TO_NAME" ] && [ "$GID_TO_NAME" != 'libvirt-qemu' ] )
> \

I think this can be shortened to:

if [ "$UID_TO_NAME" != 'libvirt-qemu' ] || [ "$GID_TO_NAME" != 'libvirt-qemu' ]; then

> +then
> +	# Ask if the user would like to continue or abort installation.
> +	db_input high libvirt-daemon-system/id_warning || true
> +	db_go
> +	db_get libvirt-daemon-system/id_warning
> +	if [ "$RET" = "false" ]; then
> +		exit 1
> +	fi
> +fi
> +
> +exit 0
> diff --git a/debian/libvirt-daemon-system.postinst b/debian/libvirt-daemon-system.postinst
> index 99e9fec..f36b806 100644
> --- a/debian/libvirt-daemon-system.postinst
> +++ b/debian/libvirt-daemon-system.postinst
> @@ -17,6 +17,9 @@ set -e
>  # for details, see http://www.debian.org/doc/debian-policy/ or
>  # the debian-policy package
>  
> +# Source debconf library.
> +. /usr/share/debconf/confmodule
> +
>  add_users_groups()
>  {
>      if ! getent group libvirt >/dev/null; then
> diff --git a/debian/libvirt-daemon-system.templates b/debian/libvirt-daemon-system.templates
> new file mode 100644
> index 0000000..7e1594b
> --- /dev/null
> +++ b/debian/libvirt-daemon-system.templates
> @@ -0,0 +1,18 @@
> +Template: libvirt-daemon-system/id_warning
> +Type: boolean
> +Default: true
> +Description: Continue with incorrect libvirt-qemu user/group ID(s)?
> +  The user/group ID (uid/gid) allocated for libvirt-qemu (64055)
> +  seems to be taken by another user/group, thus it is not possible
> +  to create the user/group with this numeric ID.
> + .
> +  The migration of guests with disk image files shared over NFS
> +  requires a static libvirt-qemu user and group ID (uid and gid)
> +  between the source and destination host systems.
> + .
> +  If guest migration over NFS is not required, you can continue
> +  the installation.
> + .
> +  In order to resolve this problem, do not continue the installation,
> +  release the 64055 uid/gid (which might involve permission changes),
> +  then install this package again.
> -- 
> 2.10.2
>