[libvirt] [PATCH v3 25/28] security_dac: Fix info messages when chown()-ing

[John Ferlan]

On 08/27/2018 04:08 AM, Michal Privoznik wrote:
> Firstly, the message that says we're setting uid:gid shouldn't be
> called from virSecurityDACSetOwnershipInternal() because
> virSecurityDACRestoreFileLabelInternal() is calling it too.
> Secondly, there are places between us reporting label restore and
> us actually doing it where we can quit. Don't say we're doing
> something until we are actually about to do it.
> 
> Signed-off-by: Michal Privoznik <mprivozn at redhat.com>
> ---
>  src/security/security_dac.c | 12 ++++++------
>  1 file changed, 6 insertions(+), 6 deletions(-)
> 
> diff --git a/src/security/security_dac.c b/src/security/security_dac.c
> index 3d0c8d20cb..1be4ead21c 100644
> --- a/src/security/security_dac.c
> +++ b/src/security/security_dac.c
> @@ -563,9 +563,6 @@ virSecurityDACSetOwnershipInternal(const virSecurityDACData *priv,
>      else if (rc > 0)
>          return 0;
>  
> -    VIR_INFO("Setting DAC user and group on '%s' to '%ld:%ld'",
> -             NULLSTR(src ? src->path : path), (long)uid, (long)gid);
> -
>      if (priv && src && priv->chownCallback) {
>          rc = priv->chownCallback(src, uid, gid);
>          /* here path is used only for error messages */
> @@ -649,6 +646,9 @@ virSecurityDACSetOwnership(virSecurityManagerPtr mgr,
>              return -1;
>      }
>  
> +    VIR_INFO("Setting DAC user and group on '%s' to '%ld:%ld'",
> +             NULLSTR(src ? src->path : path), (long)uid, (long)gid);
> +

Moving this here means the message could be printed before
virSecurityDACTransactionAppend is called (success or failure).

Besides if it fails, then we print this message, but it really didn't
happen.

So maybe this changes to "Preparing to set DAC user and group on ..."

and the cut lines move and are copied to just before the chown and the
chownCallback. That way you know that the chown was done and didn't have
a intermediate failure.  Or just before return 0 after the (rc < 0)
checks move the "Setting..." message and change to "Set the..."

>      return virSecurityDACSetOwnershipInternal(priv, src, path, uid, gid);
>  }
>  
> @@ -663,9 +663,6 @@ virSecurityDACRestoreFileLabelInternal(virSecurityManagerPtr mgr,
>      uid_t uid = 0;  /* By default return to root:root */
>      gid_t gid = 0;
>  
> -    VIR_INFO("Restoring DAC user and group on '%s'",
> -             NULLSTR(src ? src->path : path));
> -
>      if (!path && src && src->path &&
>          virStorageSourceIsLocalStorage(src))
>          path = src->path;
> @@ -678,6 +675,9 @@ virSecurityDACRestoreFileLabelInternal(virSecurityManagerPtr mgr,
>              return 0;
>      }
>  
> +    VIR_INFO("Restoring DAC user and group on '%s' to %ld:%ld",
> +             NULLSTR(src ? src->path : path), (long)uid, (long)gid);
> +

This one is fine, but one could change the removed lines to "Preparing
to restore the DAC user and group on %s"... then if *Internal prints,
then you know it happened; otherwise, something prevented it.


John

>      return virSecurityDACSetOwnershipInternal(priv, src, path, uid, gid);
>  }
>  
>