[libvirt] [PATCH v4 17/23] security_dac: Move transaction handling up one level

[John Ferlan]

On 09/10/2018 05:36 AM, Michal Privoznik wrote:
> So far the whole transaction handling is done
> virSecurityDACSetOwnershipInternal(). This needs to change for
> the sake of security label remembering and locking. Otherwise we
> would be locking a path when only appending it to transaction
> list and not when actually relabelling it.

relabeling according to my spell checker...

> 
> Signed-off-by: Michal Privoznik <mprivozn at redhat.com>
> ---
>  src/security/security_dac.c | 65 ++++++++++++++++++++++++++++++---------------
>  1 file changed, 44 insertions(+), 21 deletions(-)
> 
> diff --git a/src/security/security_dac.c b/src/security/security_dac.c
> index 926c9a33c1..52e28b5fda 100644
> --- a/src/security/security_dac.c
> +++ b/src/security/security_dac.c

[...]

> @@ -201,11 +208,16 @@ virSecurityDACTransactionRun(pid_t pid ATTRIBUTE_UNUSED,
>          virSecurityDACChownItemPtr item = list->items[i];
>  
>          /* TODO Implement rollback */
> -        if (virSecurityDACSetOwnershipInternal(list->priv,
> -                                               item->src,
> -                                               item->path,
> -                                               item->uid,
> -                                               item->gid) < 0)
> +        if ((!item->restore &&
> +             virSecurityDACSetOwnership(list->manager,
> +                                        item->src,
> +                                        item->path,
> +                                        item->uid,
> +                                        item->gid) < 0) ||

yuck - one of more least favorite constructs.

> +            (item->restore &&
> +             virSecurityDACRestoreFileLabelInternal(list->manager,
> +                                                    item->src,
> +                                                    item->path) < 0))
>              return -1;
>      }
>  
> @@ -455,7 +467,6 @@ virSecurityDACPreFork(virSecurityManagerPtr mgr)
>  static int
>  virSecurityDACTransactionStart(virSecurityManagerPtr mgr)
>  {
> -    virSecurityDACDataPtr priv = virSecurityManagerGetPrivateData(mgr);
>      virSecurityDACChownListPtr list;
>  
>      list = virThreadLocalGet(&chownList);
> @@ -468,7 +479,7 @@ virSecurityDACTransactionStart(virSecurityManagerPtr mgr)
>      if (VIR_ALLOC(list) < 0)
>          return -1;
>  
> -    list->priv = priv;
> +    list->manager = mgr;

Should this be a virObjectRef(mgr) with the corresponding virObjectUnref
at the appropriate moment in time? I don't think there's a chance @mgr
could be Unref'd otherwise, but every time I see this type construct for
an object I want to be 'safe'.

>  
>      if (virThreadLocalSet(&chownList, list) < 0) {
>          virReportSystemError(errno, "%s",
> @@ -564,11 +575,6 @@ virSecurityDACSetOwnershipInternal(const virSecurityDACData *priv,
>      /* Be aware that this function might run in a separate process.
>       * Therefore, any driver state changes would be thrown away. */
>  

^^ The above comment is repeated below looks strange "naked" here
without the virSecurityDACTransactionAppend... Theoretically though not
really true since the TransactionRun will call *SetOwnership or
*RestoreFileLabelInternal...

IDC, but figured I'd just note it anyway

The Ref/Unref is up to you - fix a couple of nits...

Reviewed-by: John Ferlan <jferlan at redhat.com>

John