[libvirt] [PATCH v4 19/23] security_dac: Lock metadata when running transaction

[John Ferlan]

On 09/10/2018 05:36 AM, Michal Privoznik wrote:
> Lock all the paths we want to relabel to mutually exclude other
> libvirt daemons.
> 
> The only culprit here hitch here is that directories can't be

reread the above and fix and fix ;-)

> locked. Therefore, when relabeling a directory do not lock it
> (this happens only when setting up some domain private paths
> anyway, e.g. huge pages directory).
> 
> Signed-off-by: Michal Privoznik <mprivozn at redhat.com>
> ---
>  src/security/security_dac.c | 37 ++++++++++++++++++++++++++++++++++---
>  1 file changed, 34 insertions(+), 3 deletions(-)
> 
> diff --git a/src/security/security_dac.c b/src/security/security_dac.c
> index 414e226f0f..e8fd4a9132 100644
> --- a/src/security/security_dac.c
> +++ b/src/security/security_dac.c
> @@ -202,8 +202,28 @@ virSecurityDACTransactionRun(pid_t pid ATTRIBUTE_UNUSED,

The description for this function needs some updating to describe this
extra step and what/how/why it's done this way.  Yeah, I know go back to
the commit message and find it...

>                               void *opaque)
>  {
>      virSecurityDACChownListPtr list = opaque;
> +    const char **paths = NULL;
> +    size_t npaths = 0;
>      size_t i;
> +    int rv;
> +    int ret = -1;
>  
> +    if (VIR_ALLOC_N(paths, list->nItems) < 0)
> +        return -1;
> +
> +    for (i = 0; i < list->nItems; i++) {
> +        const char *p = list->items[i]->path;
> +
> +        if (virFileIsDir(p))
> +            continue;
> +
> +        VIR_APPEND_ELEMENT_COPY_INPLACE(paths, npaths, p);
> +    }
> +
> +    if (virSecurityManagerMetadataLock(list->manager, paths, npaths) < 0)
> +        goto cleanup;
> +
> +    rv = 0;
>      for (i = 0; i < list->nItems; i++) {
>          virSecurityDACChownItemPtr item = list->items[i];
>  
> @@ -217,11 +237,22 @@ virSecurityDACTransactionRun(pid_t pid ATTRIBUTE_UNUSED,
>              (item->restore &&
>               virSecurityDACRestoreFileLabelInternal(list->manager,
>                                                      item->src,
> -                                                    item->path) < 0))
> -            return -1;
> +                                                    item->path) < 0)) {
> +            rv = -1;
> +            break;

If you'd used the non || construct, I think this would look cleaner:

    if (!item->restore)
        rv = virSecurityDACSetOwnership
    else
        rv = virSecurityDACRestoreFileLabelInternal

    if (rv < 0)
        break;

So much easier to read.

> +        }
>      }
>  
> -    return 0;
> +    if (virSecurityManagerMetadataUnlock(list->manager, paths, npaths) < 0)
> +        goto cleanup;

See my second note in patch 16 - Perhaps it's better to not repeat the
same sequence since paths/npaths doesn't change.

In any case, for this code...  With a couple of adjustments,

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

John

> +
> +    if (rv < 0)
> +        goto cleanup;
> +
> +    ret = 0;
> + cleanup:
> +    VIR_FREE(paths);
> +    return ret;
>  }
>  
>  
>