Re: [win-pv-devel] [PATCH] Fixed improper translation of SCHEDOP_Shutdown return code

[Paul Durrant <Paul.Durrant@xxxxxxxxxx>]

> -----Original Message-----
> From: David Buches [mailto:davebuch@xxxxxxxxxx]
> Sent: 14 November 2016 22:49
> To: win-pv-devel@xxxxxxxxxxxxxxxxxxxx
> Cc: Paul Durrant <Paul.Durrant@xxxxxxxxxx>; David Buches
> <davebuch@xxxxxxxxxx>
> Subject: [PATCH] Fixed improper translation of SCHEDOP_Shutdown return
> code
> 
> The documentation for the SCHEDOP_Shutdown hyper-call states that when
> invoked with the SHUTDOWN_Suspend reason code, the return value
> indicates
> that the guest domain either suspended (and resumed) in a new domain (0),
> or that the operation was canceled (1).
> 
> The problem - the SchedShutdown() wrapper wasn't properly translating the
> return value for SHUTDOWN_Suspend - it returned a success value for both
> successful and canceled suspend operations, which resulted in suspend
> callbacks erroneously being invoked for canceled operations, producing
> undesirable side effects (suspend callbacks are only supposed to be
> invoked when resuming on a new domain).
> 
> The code now returns an appropriate status value when
> SHUTDOWN_Suspend
> operations are canceled.
> ---
>  src/xen/sched.c | 15 +++++++++------
>  1 file changed, 9 insertions(+), 6 deletions(-)
> 
> diff --git a/src/xen/sched.c b/src/xen/sched.c
> index 07e03ce..3adede7 100644
> --- a/src/xen/sched.c
> +++ b/src/xen/sched.c
> @@ -92,13 +92,16 @@ SchedShutdown(
> 
>      if (rc < 0) {
>          ERRNO_TO_STATUS(-rc, status);
> -        goto fail1;
> +        Error("SCHEDOP_shutdown(%d) failed. (%08x)\n", Reason, status);

The error message will be prefixed with the function name so no need to state 
the hypercall, I think. Also I'd prefer to keep the forward-jump-on-error 
coding style so just leave the goto fail alone and get rid of the 'else' below.

> +    }
> +    else {
> +        /*
> +         * When a SCHEDOP_shutdown hypercall is issued with
> SHUTDOWN_suspend
> +         * reason, return value of 1 indicates that the operation was 
> cancelled
> +         */
> +        status = (SHUTDOWN_suspend == Reason && 1 == rc) ?
> +                            STATUS_CANCELLED : STATUS_SUCCESS;

None of the other code uses constant-on-the-left if style so can you please 
swap those round. All recent MSVC compilers warn on assignments in if clauses 
where not accompanied by a test.

Cheers,

  Paul

>      }
> -
> -    return STATUS_SUCCESS;
> -
> -fail1:
> -    Error("fail1 (%08x)\n", status);
> 
>      return status;
>  }
> --
> 2.10.2.windows.1


_______________________________________________
win-pv-devel mailing list
win-pv-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/cgi-bin/mailman/listinfo/win-pv-devel