Re: [libvirt PATCH] conf: virtiofs: validate that the target dir is unique even for hotplug

[Martin Kletzander <mkletzan@xxxxxxxxxx>]

On Wed, Jun 07, 2023 at 05:43:37PM +0200, Ján Tomko wrote:
> https://bugzilla.redhat.com/show_bug.cgi?id=2171384
>
> Signed-off-by: Ján Tomko <jtomko@xxxxxxxxxx>
> ---
> src/conf/domain_validate.c | 28 ++++++++++++++++++++--------
> src/conf/domain_validate.h |  2 ++
> src/libvirt_private.syms   |  1 +
> 3 files changed, 23 insertions(+), 8 deletions(-)
>
> diff --git a/src/conf/domain_validate.c b/src/conf/domain_validate.c
> index 80d6a2ffd9..4c76eb7f46 100644
> --- a/src/conf/domain_validate.c
> +++ b/src/conf/domain_validate.c
> @@ -1735,8 +1735,9 @@ virDomainDefIOMMUValidate(const virDomainDef *def)
> }
>
>
> -static int
> -virDomainDefFSValidate(const virDomainDef *def)
> +int
> +virDomainDefFSValidate(const virDomainDef *def,
> +                       const virDomainFSDef *newfs)
> {
>     size_t i;
>     g_autoptr(GHashTable) dsts = virHashNew(NULL);
> @@ -1754,8 +1755,18 @@ virDomainDefFSValidate(const virDomainDef *def)
>             return -1;
>         }
>
> -        if (virHashAddEntry(dsts, fs->dst, (void *) 0x1) < 0)
> +        if (virHashAddEntry(dsts, fs->dst, (void *) fs) < 0)
> +            return -1;
> +    }
> +
> +    if (newfs) {
> +        const virDomainFSDef *fs = g_hash_table_lookup(dsts, newfs->dst);
> +        if (fs && fs != newfs) {
> +            virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
> +                           _("filesystem target '%1$s' specified twice"),
> +                           newfs->dst);
>             return -1;
> +        }
>     }
>
>     return 0;
> @@ -1856,9 +1867,6 @@ virDomainDefValidateInternal(const virDomainDef *def,
>     if (virDomainNumaDefValidate(def->numa) < 0)
>         return -1;
>
> -    if (virDomainDefFSValidate(def) < 0)
> -        return -1;
> -
>     if (virDomainDefValidateIOThreads(def) < 0)
>         return -1;
>
> @@ -2573,7 +2581,8 @@ virDomainShmemDefValidate(const virDomainShmemDef *shmem)
> }
>
> static int
> -virDomainFSDefValidate(const virDomainFSDef *fs)
> +virDomainFSDefValidate(const virDomainDef *def,
> +                       const virDomainFSDef *fs)
> {
>     if (fs->dst == NULL) {
>         const char *source = fs->src->path;
> @@ -2592,6 +2601,9 @@ virDomainFSDefValidate(const virDomainFSDef *fs)
>         return -1;
>     }
>
> +    if (virDomainDefFSValidate(def, fs) < 0)
> +        return -1;
> +

Why not just open-code it here?  The function is not called from
anywhere else, has the same parameters and it's really confusing to see
what's the difference between virDomainFSDefValidate and
virDomainDefFSValidate to be honest.

With that fixed up:

Reviewed-by: Martin Kletzander <mkletzan@xxxxxxxxxx>

Attachment: signature.asc
Description: PGP signature