Re: [Xen-devel] [PATCH v2] xen/keyhandler: Rework keyhandler infrastructure

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 23.09.15 at 17:55, <andrew.cooper3@xxxxxxxxxx> wrote:
> +#define KEYHANDLER(k, f, desc, diag)            \
> +    [k] = { .fn = (f), (desc), 0, (diag) }
> +
> +#define IRQ_KEYHANDLER(k, f, desc, diag)        \
> +    [k] = { .irq_fn = (f), (desc), 1, (diag) }

Parenthesizing desc and diag is really unnecessary.

> -void register_keyhandler(unsigned char key, struct keyhandler *handler)
> +void register_keyhandler(unsigned char key, keyhandler_fn_t fn,
> +                         const char *desc, bool_t diagnostic)
> +{
> +    BUG_ON(key >= ARRAY_SIZE(key_table)); /* Key in range? */
> +    ASSERT(!key_table[key].fn);           /* Clobbering something else? */
> +
> +    key_table[key] = (struct keyhandler){ .fn = fn, desc, 0, diagnostic };

This is quite odd an initializer (mixing member designators with classic
initializers), but what's worse - it won't build with old gcc.

>  static void show_handlers(unsigned char key)
>  {
> -    int i;
> +    unsigned int i;
>      printk("'%c' pressed -> showing installed handlers\n", key);

Please also add the blank line missing above.

> --- a/xen/drivers/passthrough/vtd/extern.h
> +++ b/xen/drivers/passthrough/vtd/extern.h
> @@ -29,7 +29,7 @@
>  
>  void print_iommu_regs(struct acpi_drhd_unit *drhd);
>  void print_vtd_entries(struct iommu *iommu, int bus, int devfn, u64 gmfn);
> -extern struct keyhandler dump_iommu_info_keyhandler;
> +void vtd_dump_iommu_info(unsigned char key);

Any reason not to use the typedef here?

> +void register_irq_keyhandler(unsigned char key,
> +                             irq_keyhandler_fn_t *fn,
> +                             const char *desc,
> +                             bool_t diagnostic);

I wonder whether the last parameter is really useful here.

Jan


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