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

2016-11-29 Thread Mauricio Faria de Oliveira 

Hi Guido,

On 11/18/2016 05:43 PM, Mauricio Faria de Oliveira wrote:

If you're OK w/ my suggestions, would you apply the guard snippet,
or would prefer a v4 of this particular patch submitted?


Did you have a chance to check this?  Wondering how you'd prefer to
continue with this one.

Thanks!

--
Mauricio Faria de Oliveira
IBM Linux Technology Center

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

2016-11-18 Thread Mauricio Faria de Oliveira 

Guido,

On 11/18/2016 04:24 PM, Guido Günther wrote:

I'm failing to spot how you make sure you only run on new
installations. [snip]


Oops. You're right. Not sure where my head was at. It passed
tests due to debconf again; sorry.

I thought of not doing a whole-file guard, but an early exit,
to keep it short.  Is this snippet OK with you?

# Only perform this check/warning on new installations
# (precisely: when not creating neither user nor group).
if getent passwd libvirt-qemu >/dev/null \
&& getent group  libvirt-qemu >/dev/null; then
exit 0
fi


> +# 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


Hm, but if the uid/gid is not taken, then getent output is null,
which is != 'libvirt-qemu', so the warning shows up incorrectly.

If you're OK w/ my suggestions, would you apply the guard snippet,
or would prefer a v4 of this particular patch submitted?

Thanks,

--
Mauricio Faria de Oliveira
IBM Linux Technology Center

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

2016-11-18 Thread 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 
> ---
>  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   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 000..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 000..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

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

2016-11-18 Thread Mauricio Faria de Oliveira 
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.

On existing installations, no warning is displayed - just
a NEWS file is provided.

Signed-off-by: Mauricio Faria de Oliveira 
---
 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   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 000..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' ] ) \
+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 000..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