On Sat, Sep 07, 2024 at 17:15:25 +0300, Nikolai Barybin via Devel wrote:
> Signed-off-by: Nikolai Barybin <nikolai.barybin@xxxxxxxxxxxxx>
> ---
> src/security/security_dac.c | 26 +++++++++++++++++++++-----
> 1 file changed, 21 insertions(+), 5 deletions(-)
>
> diff --git a/src/security/security_dac.c b/src/security/security_dac.c
> index 59fc5b840f..f0dde46e25 100644
> --- a/src/security/security_dac.c
> +++ b/src/security/security_dac.c
> @@ -951,6 +951,10 @@ virSecurityDACSetImageLabel(virSecurityManager *mgr,
> if (virSecurityDACSetImageLabelInternal(mgr, def, n, parent, isChainTop) < 0)
> return -1;
>
> + if (n->dataFileStore &&
You "arbitrarily" chose to set 'isChainTop' as true instead of passing
the real value.
While I do understand why (the data file can't be shared anyways so it
can't be used by anything else, thus we need to consider it with teh
same permissions as the top image), it's not clear for random readers
why.
Add a comment and explain it.
> + virSecurityDACSetImageLabelInternal(mgr, def, n->dataFileStore, n, true) < 0)
> + return -1;
> +
> if (!(flags & VIR_SECURITY_DOMAIN_IMAGE_LABEL_BACKING_CHAIN))
> break;
>
> @@ -1042,7 +1046,12 @@ virSecurityDACRestoreImageLabel(virSecurityManager *mgr,
> virStorageSource *src,
> virSecurityDomainImageLabelFlags flags G_GNUC_UNUSED)
> {
> - return virSecurityDACRestoreImageLabelInt(mgr, def, src, false);
> + int rc = virSecurityDACRestoreImageLabelInt(mgr, def, src, false);
> +
> + if (rc == 0 && src->dataFileStore)
> + rc = virSecurityDACRestoreImageLabelInt(mgr, def, src->dataFileStore, false);
> +
> + return rc;
Please use the more common way to do this:
if (virSecurityDACRestoreImageLabelInt(mgr, def, src, false) < 0)
return -1;
if (src->dataFileStore &&
virSecurityDACRestoreImageLabelInt(mgr, def, src->dataFileStore, false) < 0)
return -1;
return 0;
I've also considered whether the data file seclabel shouldn't be
restored even if the restoration of the normal part fails.
Given that I don't think this will be used much I guess we should be
okay even like this.
> @@ -1906,10 +1915,17 @@ virSecurityDACRestoreAllLabel(virSecurityManager *mgr,
> def->name, migrated);
>
> for (i = 0; i < def->ndisks; i++) {
> - if (virSecurityDACRestoreImageLabelInt(mgr,
> - def,
> - def->disks[i]->src,
> - migrated) < 0)
> + int ret = virSecurityDACRestoreImageLabelInt(mgr,
> + def,
> + def->disks[i]->src,
> + migrated);
> +
> + if (ret == 0 && def->disks[i]->src->dataFileStore)
> + ret = virSecurityDACRestoreImageLabelInt(mgr,
> + def,
> + def->disks[i]->src->dataFileStore,
> + migrated);
> + if (ret < 0)
> rc = -1;
Use logic as above ... e.g. don't use 'ret'
> }
>
> --
> 2.43.5
>
