Re: [Xen-devel] [PATCH v3 2/2] xen/vm-events: Move parts of monitor_domctl code to common-side.

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 15.02.16 at 07:37, <czuzu@xxxxxxxxxxxxxxx> wrote:
>      default:
> -        return -EOPNOTSUPP;
> +        /*
> +         * Should not be reached unless arch_monitor_get_capabilities() is 
> not
> +         * properly implemented. In that case, since reaching this point does
> +         * not really break anything, don't crash the hypervisor, issue a
> +         * warning instead of BUG().
> +         */
> +        printk(XENLOG_WARNING
> +                "WARNING, BUG: arch_monitor_get_capabilities() not 
> implemented"
> +                "properly.\n");
>  
> -    };
> +        return -EOPNOTSUPP;
> +    }

I disagree with the issuing of a message here. At the very least this
should be a dprintk(). Perhaps an ASSERT_UNREACHABLE() would be
the way to go? What's worse though is that I can't see the checking
which would make true the "should not be reached" statement above
(not that you must not rely on the caller of the hypercall to be well
behaved).

> --- /dev/null
> +++ b/xen/include/xen/monitor.h
> @@ -0,0 +1,30 @@
> +/*
> + * include/xen/monitor.h
> + *
> + * Common monitor_op domctl handler.
> + *
> + * Copyright (c) 2015 Tamas K Lengyel (tamas@xxxxxxxxxxxxx)
> + * Copyright (c) 2016, Bitdefender S.R.L.
> + *
> + * This program is free software; you can redistribute it and/or
> + * modify it under the terms of the GNU General Public
> + * License v2 as published by the Free Software Foundation.
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
> + * General Public License for more details.
> + *
> + * You should have received a copy of the GNU General Public
> + * License along with this program; If not, see 
> <http://www.gnu.org/licenses/>.
> + */
> +
> +#ifndef __MONITOR_H__
> +#define __MONITOR_H__

__XEN_MONITOR_H__ please.

> +#include <xen/sched.h>
> +#include <public/domctl.h>
> +
> +int monitor_domctl(struct domain *d, struct xen_domctl_monitor_op *op);

Neither of the two includes seem necessary for this prototype, all
you need are forward declarations of the two involved structures.

Jan


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