Re: [PATCH V2 1/2] x86/hvm: Add check when register io handler

[Paul Durrant <Paul.Durrant@citrix.com>]

> -----Original Message-----
> From: Suravee Suthikulpanit [mailto:Suravee.Suthikulpanit@amd.com]
> Sent: 16 May 2016 17:03
> To: Paul Durrant; xen-devel@lists.xen.org; George Dunlap;
> jbeulich@suse.com
> Subject: Re: [PATCH V2 1/2] x86/hvm: Add check when register io handler
> 
> Hi Paul,
> 
> On 05/16/2016 03:01 AM, Paul Durrant wrote:
> >> -----Original Message-----
> >> >From:suravee.suthikulpanit@amd.com
> >> >[mailto:suravee.suthikulpanit@amd.com]
> >> >Sent: 13 May 2016 20:37
> >> >To:xen-devel@lists.xen.org; George Dunlap;jbeulich@suse.com
> >> >Cc: Paul Durrant; Suravee Suthikulpanit; Suravee Suthikulpanit
> >> >Subject: [PATCH V2 1/2] x86/hvm: Add check when register io handler
> >> >
> >> >From: Suravee Suthikulpanit<Suravee.Suthikulpanit@amd.com>
> >> >
> >> >At the time of registering HVM I/O handler, the HVM domain might
> >> >not have been initialized,
> > I/O handler registration is internal to Xen so any caller that attempt to
> register before domain initialization should be removed and replaced with
> one that does it at the right time.
> 
> Ok. I'll just remove that call for now.
> 
> >> >which means the hvm_domain.io_handler
> >> >would be NULL. In the hvm_next_io_handler(), this should be checked
> >> >before returning and referencing the array. Also, the io_handler_count
> >> >should only be incremented on success.
> >> >
> >> >So, this patch adds error handling in hvm_next_io_handler.
> >> >
> > This isn't necessary. An ASSERT would be preferable so that buggy callers
> can be easily caught.
> 
> Ok, I'll update the patch to ASSERT() and send it out. Although, just
> want to make sure that you think it should really be doing assert and
> not warning + handling error? It seems quite aggressive to crash the
> hypervisor simply because some io handler are not properly call.
> 

That's a reasonable argument, but IMO since all the callers are in the hypervisor and ASSERTs only affect debug builds I think it's reasonable.

  Paul

> Thanks,
> Suravee

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