Re: [PATCH v2 09/18] OvmfPkg/XenBusDxe: Add Event Channel Notify.

[Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>]

On Thu, Sep 04, 2014 at 05:51:04PM +0100, Anthony PERARD wrote:
> This first function is used to notify the other side that there is
> something to do. The other side is another Xen domain.
> 
> Contributed-under: TianoCore Contribution Agreement 1.0
> Signed-off-by: Anthony PERARD <anthony.perard@citrix.com>
> 
> ---
> Change in V2:
> - file header
> - coding style
> - adding comment to functions
> - Licenses
> ---
>  OvmfPkg/XenBusDxe/EventChannel.c | 49 +++++++++++++++++++++++++++++++++++++++
>  OvmfPkg/XenBusDxe/EventChannel.h | 50 ++++++++++++++++++++++++++++++++++++++++
>  OvmfPkg/XenBusDxe/XenBusDxe.inf  |  2 ++
>  3 files changed, 101 insertions(+)
>  create mode 100644 OvmfPkg/XenBusDxe/EventChannel.c
>  create mode 100644 OvmfPkg/XenBusDxe/EventChannel.h
> 
> diff --git a/OvmfPkg/XenBusDxe/EventChannel.c b/OvmfPkg/XenBusDxe/EventChannel.c
> new file mode 100644
> index 0000000..f34f9b8
> --- /dev/null
> +++ b/OvmfPkg/XenBusDxe/EventChannel.c
> @@ -0,0 +1,49 @@
> +/** @file
> +  Event Channel function implementation.
> +
> +  Event channel are use to notify of an event that happend in a shared
> +  structure for example.
> +
> +  Copyright (C) 2014, Citrix Ltd.
> +
> +  Redistribution and use in source and binary forms, with or without
> +  modification, are permitted provided that the following conditions
> +  are met:
> +
> +  * Redistributions of source code must retain the above copyright
> +    notice, this list of conditions and the following disclaimer.
> +  * Redistributions in binary form must reproduce the above copyright
> +    notice, this list of conditions and the following disclaimer in
> +    the documentation and/or other materials provided with the
> +    distribution.
> +
> +  THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND CONTRIBUTORS
> +  "AS IS" AND ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT
> +  LIMITED TO, THE IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS
> +  FOR A PARTICULAR PURPOSE ARE DISCLAIMED. IN NO EVENT SHALL THE
> +  COPYRIGHT HOLDER OR CONTRIBUTORS BE LIABLE FOR ANY DIRECT, INDIRECT,
> +  INCIDENTAL, SPECIAL, EXEMPLARY, OR CONSEQUENTIAL DAMAGES (INCLUDING,
> +  BUT NOT LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES;
> +  LOSS OF USE, DATA, OR PROFITS; OR BUSINESS INTERRUPTION) HOWEVER
> +  CAUSED AND ON ANY THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT
> +  LIABILITY, OR TORT (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN
> +  ANY WAY OUT OF THE USE OF THIS SOFTWARE, EVEN IF ADVISED OF THE
> +  POSSIBILITY OF SUCH DAMAGE.
> +
> +**/
> +#include "EventChannel.h"
> +#include "XenHypercall.h"
> +
> +VOID
> +XenEventChannelNotify (
> +  IN XENBUS_DEVICE *Dev,
> +  IN evtchn_port_t Port
> +  )
> +{
> +  INTN ReturnCode;
> +  evtchn_send_t Send;
> +
> +  Send.port = Port;
> +  ReturnCode = XenHypercallEventChannelOp (Dev, EVTCHNOP_send, &Send);
> +  ASSERT (ReturnCode == 0);

Ouch. How about returning the return code. For example it might be -EINVAL
due to the port value being wrong (or not yet set). Shouldn't the caller
decided whether to ASSERT at that point?