Re: [PATCH v2 14/14] qga: Fix qmp_guest_suspend_{disk, ram}() error handling

[Philippe Mathieu-Daudé]

On 4/22/20 3:07 PM, Markus Armbruster wrote:
> The Error ** argument must be NULL, &error_abort, &error_fatal, or a
> pointer to a variable containing NULL.  Passing an argument of the
> latter kind twice without clearing it in between is wrong: if the
> first call sets an error, it no longer points to NULL for the second
>
> qmp_guest_suspend_disk() and qmp_guest_suspend_ram() pass @local_err
> first to check_suspend_mode(), then to acquire_privilege(), then to
> execute_async().  Continuing after errors here can only end in tears.
> For instance, we risk tripping error_setv()'s assertion.
>
> Fixes: aa59637ea1c6a4c83430933f9c44c43e6c3f1b69
> Fixes: f54603b6aa765514b2519e74114a2f417759d727
> Cc: Michael Roth <address@hidden>
> Signed-off-by: Markus Armbruster <address@hidden>
> ---
>   qga/commands-win32.c | 14 ++++++++++++++
>   1 file changed, 14 insertions(+)
>
> diff --git a/qga/commands-win32.c b/qga/commands-win32.c
> index 9717a8d52d..5ba56327dd 100644
> --- a/qga/commands-win32.c
> +++ b/qga/commands-win32.c
> @@ -1322,9 +1322,16 @@ void qmp_guest_suspend_disk(Error **errp)
>   
>       *mode = GUEST_SUSPEND_MODE_DISK;
>       check_suspend_mode(*mode, &local_err);
> +    if (local_err) {
> +        goto out;
> +    }
>       acquire_privilege(SE_SHUTDOWN_NAME, &local_err);
> +    if (local_err) {
> +        goto out;
> +    }
>       execute_async(do_suspend, mode, &local_err);
>   
> +out:
>       if (local_err) {

https://www.mail-archive.com/address@hidden/msg695647.html is 
slightly different by removing the if() check.

>           error_propagate(errp, local_err);
>           g_free(mode);
> @@ -1338,9 +1345,16 @@ void qmp_guest_suspend_ram(Error **errp)
>   
>       *mode = GUEST_SUSPEND_MODE_RAM;
>       check_suspend_mode(*mode, &local_err);
> +    if (local_err) {
> +        goto out;
> +    }
>       acquire_privilege(SE_SHUTDOWN_NAME, &local_err);
> +    if (local_err) {
> +        goto out;
> +    }
>       execute_async(do_suspend, mode, &local_err);
>   
> +out:
>       if (local_err) {
>           error_propagate(errp, local_err);
>           g_free(mode);