Re: [PATCH 2/3] xen/granttable: Grant tables V2 implementation

[Ian Campbell]

On Fri, 2011-11-18 at 10:13 +0000, ANNIE LI wrote:
> static void gnttab_request_version(void)
>  {
> -       grant_table_version = 1;
> -       gnttab_interface = &gnttab_v1_ops;
> +       int rc;
> +       struct gnttab_set_version gsv;
> +       const char *str = "we need grant tables version 2, but only version 1 is available\n";
> +
> +       gsv.version = 2;
> +       rc = HYPERVISOR_grant_table_op(GNTTABOP_set_version, &gsv, 1);
> +       if (rc == 0) {
> +               grant_table_version = 2;
> +               gnttab_interface = &gnttab_v2_ops;
> +       } else {
> +               if (grant_table_version == 2) {

	  } else if (...) {
		panic
	  } else {
		setup v1
	  }

> +                       /*
> +                        * If we've already used version 2 features,
> +                        * but then suddenly discover that they're not
> +                        * available (e.g. migrating to an older
> +                        * version of Xen), almost unbounded badness
> +                        * can happen.
> +                        */
> +                       xen_raw_printk(str);
> +                       panic(str);

I expect you've just copied this style from elsewhere but I really
dislike this duplication of prints. If panic is not useful here we
really ought to address that at the root instead of going around
patching things to print every panic message twice. I thought
earlyprintk was supposed to solve this problem. Perhaps a generic
early_panic_print could be added to the panic code?

If we stick with this (which is fair enough since it is outside the
scope of this series) you should move the const char *str into this code
block so it is near to the use.

> +               }
> +               grant_table_version = 1;
> +               gnttab_interface = &gnttab_v1_ops;
> +       }
>         printk(KERN_INFO "Grant tables using version %d layout.\n",
>                 grant_table_version);
>  } 

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/