Re: [Xen-devel] [PATCH] x86/HVM: handle_{mmio*, pio}() return value adjustments

[Paul Durrant <Paul.Durrant@xxxxxxxxxx>]

> -----Original Message-----
> From: Jan Beulich [mailto:JBeulich@xxxxxxxx]
> Sent: 16 December 2016 10:25
> To: Paul Durrant <Paul.Durrant@xxxxxxxxxx>
> Cc: Andrew Cooper <Andrew.Cooper3@xxxxxxxxxx>; xen-devel <xen-
> devel@xxxxxxxxxxxxxxxxxxxx>
> Subject: RE: [PATCH] x86/HVM: handle_{mmio*,pio}() return value
> adjustments
> 
> >>> On 16.12.16 at 10:52, <Paul.Durrant@xxxxxxxxxx> wrote:
> >> From: Jan Beulich [mailto:JBeulich@xxxxxxxx]
> >> Sent: 16 December 2016 09:31
> >> @@ -100,22 +100,21 @@ int handle_mmio(void)
> >>      {
> >>      case X86EMUL_UNHANDLEABLE:
> >>          hvm_dump_emulation_state(XENLOG_G_WARNING "MMIO",
> &ctxt);
> >> -        return 0;
> >> +        return false;
> >> +
> >>      case X86EMUL_EXCEPTION:
> >>          if ( ctxt.ctxt.event_pending )
> >>              hvm_inject_event(&ctxt.ctxt.event);
> >>          break;
> >> -    default:
> >> -        break;
> >
> > Should there not be some sort of default case, even if it's simply to assert
> > that it's not reachable?
> 
> A default case doing nothing when the switch expression is not of
> an enum type is pointless. And it _is_ reachable (namely for
> X86EMUL_OKAY).

Then my preference would still be an explicit case for X86EMUL_OKAY and an 
unreachable default, but if you don't think it's worth it then the code is ok 
as-is.

> 
> >> --- a/xen/arch/x86/hvm/ioreq.c
> >> +++ b/xen/arch/x86/hvm/ioreq.c
> >> @@ -156,13 +156,14 @@ bool_t handle_hvm_io_completion(struct v
> >
> > Do you not want to change this to from bool_t to bool while you're at it?
> 
> Well, while I first was inclined to do so, I then didn't want to do
> too many things at once (iirc there would be a few more which then
> would want updating at the same time).
> 

Fair enough.

Reviewed-by: Paul Durrant <paul.durrant@xxxxxxxxxx>

> Jan


_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
https://lists.xen.org/xen-devel