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 <mpriv...@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 <jfer...@redhat.com>
John
> +
> + if (rv < 0)
> + goto cleanup;
> +
> + ret = 0;
> + cleanup:
> + VIR_FREE(paths);
> + return ret;
> }
>
>
>
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
