[PATCH v1 4/7] qemu: add multi-secret support in qemuBlockStorageSourceAttachData

[Peter Krempa]

On Mon, Mar 06, 2023 at 06:53:09 -0600, Or Ozeri wrote:
> This commit changes the qemuBlockStorageSourceAttachData struct
> to support multiple secrets (instead of a single one before this commit).
> This will useful for storage encryption requiring more than a single secret.
> 
> Signed-off-by: Or Ozeri <oro at il.ibm.com>
> ---
>  src/qemu/qemu_block.c    | 35 ++++++++++++++++++++++++++---------
>  src/qemu/qemu_block.h    |  5 +++--
>  src/qemu/qemu_blockjob.c |  6 ++++++
>  src/qemu/qemu_command.c  | 21 +++++++++++++++++----
>  4 files changed, 52 insertions(+), 15 deletions(-)

[...]

> @@ -1605,8 +1615,15 @@ qemuBlockStorageSourceDetachPrepare(virStorageSource *src)
>          if (srcpriv->secinfo)
>              data->authsecretAlias = g_strdup(srcpriv->secinfo->alias);
>  
> -        if (srcpriv->encinfo)
> -            data->encryptsecretAlias = g_strdup(srcpriv->encinfo->alias);
> +        if (srcpriv->encinfo) {
> +            if (!data->encryptsecretAlias) {

If this were non-NULL ...

> +                data->encryptsecretCount = 1;
> +                data->encryptsecretProps = g_new0(virJSONValue *, 1);
> +                data->encryptsecretAlias = g_new0(char *, 1);
> +            }
> +
> +            data->encryptsecretAlias[0] = g_strdup(srcpriv->encinfo->alias);

... then it's very likely that this was already filled, and you'd
overwrite it.

Given that qemuBlockStorageSourceDetachPrepare freshly allocates the
'data' object the above is impossible and the if condition you've added
is tautological and thus pointless.

I'll remove it before pushing.

> +        }
>  
>          if (srcpriv->httpcookie)
>              data->httpcookiesecretAlias = g_strdup(srcpriv->httpcookie->alias);

[...]

> diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
> index 4839d45a34..f5dcb46e42 100644
> --- a/src/qemu/qemu_command.c
> +++ b/src/qemu/qemu_command.c

[...]

>  
> @@ -10637,9 +10643,16 @@ qemuBuildStorageSourceAttachPrepareCommon(virStorageSource *src,
>              qemuBuildSecretInfoProps(srcpriv->secinfo, &data->authsecretProps) < 0)
>              return -1;
>  
> -        if (srcpriv->encinfo &&
> -            qemuBuildSecretInfoProps(srcpriv->encinfo, &data->encryptsecretProps) < 0)
> -            return -1;
> +        if (srcpriv->encinfo) {
> +           if (!data->encryptsecretProps) {
> +               data->encryptsecretCount = 1;
> +               data->encryptsecretProps = g_new0(virJSONValue *, 1);
> +               data->encryptsecretAlias = g_new0(char *, 1);
> +           }
> +
> +           if (qemuBuildSecretInfoProps(srcpriv->encinfo, &data->encryptsecretProps[0]) < 0)
> +               return -1;
> +        }

Same as above. Although it's not that easy to see that the condition is
tautological as the object is allocated outside of the funciton.

Reviewed-by: Peter Krempa <pkrempa at redhat.com>