Re: [PATCH 01/12] qemuMonitorJSONSetMigrationParams: Take double pointer for @params

[Michal Privoznik <mprivozn@xxxxxxxxxx>]

On 1/6/21 3:03 PM, Peter Krempa wrote:
> This allows simplification of the caller as well as will enable a later
> refactor of qemuMonitorJSONMakeCommandInternal.
>
> Signed-off-by: Peter Krempa <pkrempa@xxxxxxxxxx>
> ---
>   src/qemu/qemu_migration_params.c |  9 +++------
>   src/qemu/qemu_monitor.c          | 11 +++--------
>   src/qemu/qemu_monitor.h          |  2 +-
>   src/qemu/qemu_monitor_json.c     |  5 +++--
>   src/qemu/qemu_monitor_json.h     |  2 +-
>   5 files changed, 11 insertions(+), 18 deletions(-)
>
> diff --git a/src/qemu/qemu_migration_params.c b/src/qemu/qemu_migration_params.c
> index df5560d39f..d1d59aeb01 100644
> --- a/src/qemu/qemu_migration_params.c
> +++ b/src/qemu/qemu_migration_params.c
> @@ -843,12 +843,9 @@ qemuMigrationParamsApply(virQEMUDriverPtr driver,
>       if (!(params = qemuMigrationParamsToJSON(migParams)))
>           goto cleanup;
>
> -    if (virJSONValueObjectKeysNumber(params) > 0) {
> -        rc = qemuMonitorSetMigrationParams(priv->mon, params);
> -        params = NULL;
> -        if (rc < 0)
> -            goto cleanup;
> -    }
> +    if (virJSONValueObjectKeysNumber(params) > 0 &&
> +        qemuMonitorSetMigrationParams(priv->mon, &params) < 0)
> +        goto cleanup;
>
>       ret = 0;
>
> diff --git a/src/qemu/qemu_monitor.c b/src/qemu/qemu_monitor.c
> index 40f2997cb6..a81cd5fff5 100644
> --- a/src/qemu/qemu_monitor.c
> +++ b/src/qemu/qemu_monitor.c
> @@ -2464,22 +2464,17 @@ qemuMonitorGetMigrationParams(qemuMonitorPtr mon,
>    * @mon: Pointer to the monitor object.
>    * @params: Migration parameters.
>    *
> - * The @params object is consumed and should not be referenced by the caller
> - * after this function returns.
> + * The @params object is consumed and cleared.

This is not entirely true. If QEMU_CHECK_MONITOR() fails, then @params 
is not touched at all. However, since callers will almost certainly us 
g_auto() (because they need to construct virJSONValue object) I guess 
it's okay. Maybe change it to "consumed and cleared on success and 
potentially on some error cases too"?

>    *
>    * Returns 0 on success, -1 on error.
>    */
>   int
>   qemuMonitorSetMigrationParams(qemuMonitorPtr mon,
> -                              virJSONValuePtr params)
> +                              virJSONValuePtr *params)
>   {
> -    QEMU_CHECK_MONITOR_GOTO(mon, error);
> +    QEMU_CHECK_MONITOR(mon);
>
>       return qemuMonitorJSONSetMigrationParams(mon, params);
> -
> - error:
> -    virJSONValueFree(params);
> -    return -1;
>   }

Michal