[PATCH] Use new Xen public header for product numbers and names

[PATCH] Use new Xen public header for product numbers and names

[Paul Durrant]

xen/include/public/hvm/pvdrivers.h has been added as the
register of product numbers used by the blacklisting protocol.
Use the definitions therein rather then locally coded values.

Signed-off-by: Paul Durrant <paul.durrant@citrix.com>
---
 xenstore.c |   30 ++++++++++++++----------------
 1 files changed, 14 insertions(+), 16 deletions(-)

diff --git a/xenstore.c b/xenstore.c
index 1857160..04ae199 100644
--- a/xenstore.c
+++ b/xenstore.c
@@ -23,6 +23,8 @@
 #include "qemu-timer.h"
 #include "qemu-xen.h"
 
+#include <xen/hvm/pvdrivers.h>
+
 struct xs_handle *xsh = NULL;
 static char *media_filename[MAX_DRIVES+1];
 static QEMUTimer *insert_timer = NULL;
@@ -961,22 +963,18 @@ xenstore_pv_driver_build_blacklisted(uint16_t product_nr,
     const char *product;
 
     switch (product_nr) {
-    /*
-     * In qemu-xen-unstable, this is the master registry of product
-     * numbers.  If you need a new product number allocating, please
-     * post to xen-devel@lists.xensource.com.  You should NOT use
-     * an existing product number without allocating one.
-     *
-     * If you maintain a seaparate versioning and distribution path
-     * for PV drivers you should have a separate product number so
-     * that your drivers can be separated from others'.
-     *
-     * During development, you may use the product ID 0xffff to
-     * indicate a driver which is yet to be released.
-     */
-    case 1: product = "xensource-windows";  break; /* Citrix */
-    case 2: product = "gplpv-windows";      break; /* James Harper */
-    case 0xffff: product = "experimental";  break;
+    case PVDRIVERS_XENSOURCE_WINDOWS_ID:
+        product = PVDRIVERS_XENSOURCE_WINDOWS_NAME;
+        break;
+    case PVDRIVERS_GPLPV_WINDOWS_ID:
+        product = PVDRIVERS_GPLPV_WINDOWS_NAME;
+        break;
+    case PVDRIVERS_LINUX_ID:
+        product = PVDRIVERS_LINUX_NAME;
+        break;
+    case PVDRIVERS_EXPERIMENTAL_ID:
+        product = PVDRIVERS_EXPERIMENTAL_NAME; 
+        break;
     default:
         /* Don't know what product this is -> we can't blacklist
          * it. */
-- 
1.7.2.5

Re: [PATCH] Use new Xen public header for product numbers and names

[Ian Jackson]

Paul Durrant writes ("[Xen-devel] [PATCH] Use new Xen public header for product numbers and names"):
> xen/include/public/hvm/pvdrivers.h has been added as the
> register of product numbers used by the blacklisting protocol.
> Use the definitions therein rather then locally coded values.

This approach duplicates the list as is IMO not acceptable:

> +    case PVDRIVERS_XENSOURCE_WINDOWS_ID:
> +        product = PVDRIVERS_XENSOURCE_WINDOWS_NAME;
> +        break;
> +    case PVDRIVERS_GPLPV_WINDOWS_ID:
> +        product = PVDRIVERS_GPLPV_WINDOWS_NAME;
> +        break;
> +    case PVDRIVERS_LINUX_ID:
> +        product = PVDRIVERS_LINUX_NAME;
> +        break;
> +    case PVDRIVERS_EXPERIMENTAL_ID:
> +        product = PVDRIVERS_EXPERIMENTAL_NAME; 
> +        break;

We need to avoid this formulaic code which needs changing every time a
new entry is added.

Ian.

Re: [PATCH] Use new Xen public header for product numbers and names

[Paul Durrant]

> -----Original Message-----
> From: Ian Jackson [mailto:Ian.Jackson@eu.citrix.com]
> Sent: 28 September 2012 18:56
> To: Paul Durrant
> Cc: xen-devel@lists.xen.org
> Subject: Re: [Xen-devel] [PATCH] Use new Xen public header for product
> numbers and names
> 
> Paul Durrant writes ("[Xen-devel] [PATCH] Use new Xen public header for
> product numbers and names"):
> > xen/include/public/hvm/pvdrivers.h has been added as the register of
> > product numbers used by the blacklisting protocol.
> > Use the definitions therein rather then locally coded values.
> 
> This approach duplicates the list as is IMO not acceptable:
> 
> > +    case PVDRIVERS_XENSOURCE_WINDOWS_ID:
> > +        product = PVDRIVERS_XENSOURCE_WINDOWS_NAME;
> > +        break;
> > +    case PVDRIVERS_GPLPV_WINDOWS_ID:
> > +        product = PVDRIVERS_GPLPV_WINDOWS_NAME;
> > +        break;
> > +    case PVDRIVERS_LINUX_ID:
> > +        product = PVDRIVERS_LINUX_NAME;
> > +        break;
> > +    case PVDRIVERS_EXPERIMENTAL_ID:
> > +        product = PVDRIVERS_EXPERIMENTAL_NAME;
> > +        break;
> 
> We need to avoid this formulaic code which needs changing every time a
> new entry is added.
> 

Ok. I'll come up with something we can use to define an array and then we can iterate over that.

  Paul

Re: [PATCH] Use new Xen public header for product numbers and names

[Ian Jackson]

Paul Durrant writes ("RE: [Xen-devel] [PATCH] Use new Xen public header for product numbers	and names"):
> Ok. I'll come up with something we can use to define an array and
> then we can iterate over that.

Great.


What do people think of this preprocessor approach, which I suggested
earlier:

  #define PVDRIVERS_LIST(EACH)                              \
    EACH(WINDOWS, 0x0001 /* Citrix */, "xensource-windows") \
    ...

That can be used to generate arrays, or switch statements, or
whatever.  For example in pvdrivers.h:

  #define PVDRIVERS_EACH_ENUM(name,num,string) \
      PVDRIVERS_ID_##name = num,
  enum {
      PVDRIVERS_LIST(PVDRIVERS_EACH_ENUM)
  };

and then in qemu:

  #define PVDRIVERS_EACH_SWITCH(name,num,string) \
     case PVDRIVERS_ID_##name:  product = string;  break;

  switch (id) {
  PVDRIVERS_LIST(PVDRIVERS_EACH_SWITCH)
  default:
    sprintf blah blah
  }


If the indirect macro trick is too abstruse it's also possible to do
this:

  #define PVDRIVERS_LIST                                              \
    PVDRIVERS_EACH(WINDOWS, 0x0001 /* Citrix */, "xensource-windows") \
    ...

That can be used to generate arrays, or switch statements, or
whatever.  For example in pvdrivers.h:

  #define PVDRIVERS_EACH(name,num,string) \
      PVDRIVERS_ID_##name = num,
  enum {
      PVDRIVERS_LIST
  };
  #undef PVDRIVERS_EACH

and then in qemu:

  #define PVDRIVERS_EACH(name,num,string) \
     case PVDRIVERS_ID_##name:  product = string;  break;

  switch (id) {
  PVDRIVERS_LIST
  default:
    sprintf blah blah
  }

  #undef PVDRIVERS_EACH


A #define containing an array initialiser is the other obvious
alternative although of course the macro-based approaches I sketch
above can be used to define arrays but not vice versa.


Ian.

Re: [PATCH] Use new Xen public header for product numbers and names

[Paul Durrant]

> -----Original Message-----
> 
> What do people think of this preprocessor approach, which I suggested
> earlier:
> 
>   #define PVDRIVERS_LIST(EACH)                              \
>     EACH(WINDOWS, 0x0001 /* Citrix */, "xensource-windows") \
>     ...
> 

I liked it so that's what I did :-) I added a terminator entry for convenience when defining an array. Not needed when defining a switch statement but harmless since it's a case that should never be hit.

  Paul

Re: [PATCH] Use new Xen public header for product numbers and names [and 1 more messages]

[Ian Jackson]

Paul Durrant writes ("RE: [Xen-devel] [PATCH] Use new Xen public header for product numbers	and names"):
> I liked it so that's what I did :-) I added a terminator entry for
> convenience when defining an array. Not needed when defining a
> switch statement but harmless since it's a case that should never be
> hit.

Very well.

Keir Fraser writes ("Re: [Xen-devel] [PATCH] Use new Xen public header for product numbers and names"):
> My usual way to do this would be to place the list in its own header file:
> 
> pvdrivers_list.h:
> EACH(WINDOWS, 0x0001, "xensource-windows")
> EACH(...)
> ...
> 
> Then e.g.,
> #define EACH(name,num,string) PVDRIVERS_ID_##name = num,
> enum {
> #include "pvdrivers_list.h"
> }
> #undef EACH
> 
> Which is a bit more abstruse on the consuming side, but does avoid the list
> itself being an ugly multi-line macro monster.

This is fine by me too.

Thanks,
Ian.

Re: [PATCH] Use new Xen public header for product numbers and names

[Keir Fraser]

On 01/10/2012 11:13, "Ian Jackson" <Ian.Jackson@eu.citrix.com> wrote:

> Paul Durrant writes ("RE: [Xen-devel] [PATCH] Use new Xen public header for
> product numbers and names"):
>> Ok. I'll come up with something we can use to define an array and
>> then we can iterate over that.
> 
> Great.
> 
> 
> What do people think of this preprocessor approach, which I suggested
> earlier:

My usual way to do this would be to place the list in its own header file:

pvdrivers_list.h:
EACH(WINDOWS, 0x0001, "xensource-windows")
EACH(...)
...

Then e.g.,
#define EACH(name,num,string) PVDRIVERS_ID_##name = num,
enum {
#include "pvdrivers_list.h"
}
#undef EACH

Which is a bit more abstruse on the consuming side, but does avoid the list
itself being an ugly multi-line macro monster.

I don't have a strong opinion on this by the way. :) Just saying...

 -- Keir

>   #define PVDRIVERS_LIST(EACH)                              \
>     EACH(WINDOWS, 0x0001 /* Citrix */, "xensource-windows") \
>     ...
> 
> That can be used to generate arrays, or switch statements, or
> whatever.  For example in pvdrivers.h:
> 
>   #define PVDRIVERS_EACH_ENUM(name,num,string) \
>       PVDRIVERS_ID_##name = num,
>   enum {
>       PVDRIVERS_LIST(PVDRIVERS_EACH_ENUM)
>   };
> 
> and then in qemu:
> 
>   #define PVDRIVERS_EACH_SWITCH(name,num,string) \
>      case PVDRIVERS_ID_##name:  product = string;  break;
> 
>   switch (id) {
>   PVDRIVERS_LIST(PVDRIVERS_EACH_SWITCH)
>   default:
>     sprintf blah blah
>   }
> 
> 
> If the indirect macro trick is too abstruse it's also possible to do
> this:
> 
>   #define PVDRIVERS_LIST                                              \
>     PVDRIVERS_EACH(WINDOWS, 0x0001 /* Citrix */, "xensource-windows") \
>     ...
> 
> That can be used to generate arrays, or switch statements, or
> whatever.  For example in pvdrivers.h:
> 
>   #define PVDRIVERS_EACH(name,num,string) \
>       PVDRIVERS_ID_##name = num,
>   enum {
>       PVDRIVERS_LIST
>   };
>   #undef PVDRIVERS_EACH
> 
> and then in qemu:
> 
>   #define PVDRIVERS_EACH(name,num,string) \
>      case PVDRIVERS_ID_##name:  product = string;  break;
> 
>   switch (id) {
>   PVDRIVERS_LIST
>   default:
>     sprintf blah blah
>   }
> 
>   #undef PVDRIVERS_EACH
> 
> 
> A #define containing an array initialiser is the other obvious
> alternative although of course the macro-based approaches I sketch
> above can be used to define arrays but not vice versa.
> 
> 
> Ian.
> 
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xen.org
> http://lists.xen.org/xen-devel

[PATCH] Use new Xen public header for product numbers and names

[Paul Durrant]

xen/include/public/hvm/pvdrivers.h has been added as the
register of product numbers used by the blacklisting protocol.
Use the definitions therein rather then locally coded values.

Signed-off-by: Paul Durrant <paul.durrant@citrix.com>
---
 xenstore.c |   23 +++++++----------------
 1 files changed, 7 insertions(+), 16 deletions(-)

diff --git a/xenstore.c b/xenstore.c
index 1857160..4382674 100644
--- a/xenstore.c
+++ b/xenstore.c
@@ -23,6 +23,8 @@
 #include "qemu-timer.h"
 #include "qemu-xen.h"
 
+#include <xen/hvm/pvdrivers.h>
+
 struct xs_handle *xsh = NULL;
 static char *media_filename[MAX_DRIVES+1];
 static QEMUTimer *insert_timer = NULL;
@@ -960,28 +962,17 @@ xenstore_pv_driver_build_blacklisted(uint16_t product_nr,
     char *tmp;
     const char *product;
 
+#define PRODUCT(_name, _nr) case _nr: product = _name; break;
     switch (product_nr) {
-    /*
-     * In qemu-xen-unstable, this is the master registry of product
-     * numbers.  If you need a new product number allocating, please
-     * post to xen-devel@lists.xensource.com.  You should NOT use
-     * an existing product number without allocating one.
-     *
-     * If you maintain a seaparate versioning and distribution path
-     * for PV drivers you should have a separate product number so
-     * that your drivers can be separated from others'.
-     *
-     * During development, you may use the product ID 0xffff to
-     * indicate a driver which is yet to be released.
-     */
-    case 1: product = "xensource-windows";  break; /* Citrix */
-    case 2: product = "gplpv-windows";      break; /* James Harper */
-    case 0xffff: product = "experimental";  break;
+    PVDRIVERS_PRODUCT_LIST(PRODUCT)
     default:
         /* Don't know what product this is -> we can't blacklist
          * it. */
         return 0;
     }
+
+#undef  PRODUCT
+
     if (asprintf(&buf, "/mh/driver-blacklist/%s/%d", product, build_nr) < 0)
         return 0;
     tmp = xs_read(xsh, XBT_NULL, buf, NULL);
-- 
1.7.2.5

Re: [PATCH] Use new Xen public header for product numbers and names

[Ian Jackson]

Paul Durrant writes ("[Xen-devel] [PATCH] Use new Xen public header for product numbers and names"):
> xen/include/public/hvm/pvdrivers.h has been added as the
> register of product numbers used by the blacklisting protocol.
> Use the definitions therein rather then locally coded values.
...
> +#define PRODUCT(_name, _nr) case _nr: product = _name; break;
>      switch (product_nr) {
> -    /*
> -     * In qemu-xen-unstable, this is the master registry of product
> -     * numbers.  If you need a new product number allocating, please
> -     * post to xen-devel@lists.xensource.com.  You should NOT use
> -     * an existing product number without allocating one.
> -     *
> -     * If you maintain a seaparate versioning and distribution path
> -     * for PV drivers you should have a separate product number so
> -     * that your drivers can be separated from others'.
> -     *
> -     * During development, you may use the product ID 0xffff to
> -     * indicate a driver which is yet to be released.
> -     */
> -    case 1: product = "xensource-windows";  break; /* Citrix */
> -    case 2: product = "gplpv-windows";      break; /* James Harper */
> -    case 0xffff: product = "experimental";  break;
> +    PVDRIVERS_PRODUCT_LIST(PRODUCT)

As a case in point, this generates:
       case 0: product = NULL; break;
and then passes the NULL to asprintf.

Ian.

Re: [PATCH] Use new Xen public header for product numbers and names

[Paul Durrant]

> -----Original Message-----
> From: Ian Jackson [mailto:Ian.Jackson@eu.citrix.com]
> Sent: 01 October 2012 16:14
> To: Paul Durrant
> Cc: xen-devel@lists.xen.org
> Subject: Re: [Xen-devel] [PATCH] Use new Xen public header for product
> numbers and names
> 
> Paul Durrant writes ("[Xen-devel] [PATCH] Use new Xen public header for
> product numbers and names"):
> > xen/include/public/hvm/pvdrivers.h has been added as the register of
> > product numbers used by the blacklisting protocol.
> > Use the definitions therein rather then locally coded values.
> ...
> > +#define PRODUCT(_name, _nr) case _nr: product = _name; break;
> >      switch (product_nr) {
> > -    /*
> > -     * In qemu-xen-unstable, this is the master registry of product
> > -     * numbers.  If you need a new product number allocating, please
> > -     * post to xen-devel@lists.xensource.com.  You should NOT use
> > -     * an existing product number without allocating one.
> > -     *
> > -     * If you maintain a seaparate versioning and distribution path
> > -     * for PV drivers you should have a separate product number so
> > -     * that your drivers can be separated from others'.
> > -     *
> > -     * During development, you may use the product ID 0xffff to
> > -     * indicate a driver which is yet to be released.
> > -     */
> > -    case 1: product = "xensource-windows";  break; /* Citrix */
> > -    case 2: product = "gplpv-windows";      break; /* James Harper */
> > -    case 0xffff: product = "experimental";  break;
> > +    PVDRIVERS_PRODUCT_LIST(PRODUCT)
> 
> As a case in point, this generates:
>        case 0: product = NULL; break;
> and then passes the NULL to asprintf.
> 

Nothing should be using product number 0, so is this a problem?

  Paul

Re: [PATCH] Use new Xen public header for product numbers and names

[Ian Campbell]

On Mon, 2012-10-01 at 17:08 +0100, Paul Durrant wrote:
> > -----Original Message-----
> > From: Ian Jackson [mailto:Ian.Jackson@eu.citrix.com]
> > Sent: 01 October 2012 16:14
> > To: Paul Durrant
> > Cc: xen-devel@lists.xen.org
> > Subject: Re: [Xen-devel] [PATCH] Use new Xen public header for product
> > numbers and names
> > 
> > Paul Durrant writes ("[Xen-devel] [PATCH] Use new Xen public header for
> > product numbers and names"):
> > > xen/include/public/hvm/pvdrivers.h has been added as the register of
> > > product numbers used by the blacklisting protocol.
> > > Use the definitions therein rather then locally coded values.
> > ...
> > > +#define PRODUCT(_name, _nr) case _nr: product = _name; break;
> > >      switch (product_nr) {
> > > -    /*
> > > -     * In qemu-xen-unstable, this is the master registry of product
> > > -     * numbers.  If you need a new product number allocating, please
> > > -     * post to xen-devel@lists.xensource.com.  You should NOT use
> > > -     * an existing product number without allocating one.
> > > -     *
> > > -     * If you maintain a seaparate versioning and distribution path
> > > -     * for PV drivers you should have a separate product number so
> > > -     * that your drivers can be separated from others'.
> > > -     *
> > > -     * During development, you may use the product ID 0xffff to
> > > -     * indicate a driver which is yet to be released.
> > > -     */
> > > -    case 1: product = "xensource-windows";  break; /* Citrix */
> > > -    case 2: product = "gplpv-windows";      break; /* James Harper */
> > > -    case 0xffff: product = "experimental";  break;
> > > +    PVDRIVERS_PRODUCT_LIST(PRODUCT)
> > 
> > As a case in point, this generates:
> >        case 0: product = NULL; break;
> > and then passes the NULL to asprintf.
> > 
> 
> Nothing should be using product number 0, so is this a problem?

In some circumstances (I think) the product number may come from the
guest, which may be malicious.

Ian.

[PATCH] Use new Xen public header for product numbers and names

[Paul Durrant]

xen/include/public/hvm/pvdrivers.h has been added as the
register of product numbers used by the blacklisting protocol.
Use the definitions therein rather then locally coded values.

Signed-off-by: Paul Durrant <paul.durrent@citrix.com>
---
 xenstore.c |   30 ++++++++++++++----------------
 1 files changed, 14 insertions(+), 16 deletions(-)

diff --git a/xenstore.c b/xenstore.c
index 1857160..04ae199 100644
--- a/xenstore.c
+++ b/xenstore.c
@@ -23,6 +23,8 @@
 #include "qemu-timer.h"
 #include "qemu-xen.h"
 
+#include <xen/hvm/pvdrivers.h>
+
 struct xs_handle *xsh = NULL;
 static char *media_filename[MAX_DRIVES+1];
 static QEMUTimer *insert_timer = NULL;
@@ -961,22 +963,18 @@ xenstore_pv_driver_build_blacklisted(uint16_t product_nr,
     const char *product;
 
     switch (product_nr) {
-    /*
-     * In qemu-xen-unstable, this is the master registry of product
-     * numbers.  If you need a new product number allocating, please
-     * post to xen-devel@lists.xensource.com.  You should NOT use
-     * an existing product number without allocating one.
-     *
-     * If you maintain a seaparate versioning and distribution path
-     * for PV drivers you should have a separate product number so
-     * that your drivers can be separated from others'.
-     *
-     * During development, you may use the product ID 0xffff to
-     * indicate a driver which is yet to be released.
-     */
-    case 1: product = "xensource-windows";  break; /* Citrix */
-    case 2: product = "gplpv-windows";      break; /* James Harper */
-    case 0xffff: product = "experimental";  break;
+    case PVDRIVERS_XENSOURCE_WINDOWS_ID:
+        product = PVDRIVERS_XENSOURCE_WINDOWS_NAME;
+        break;
+    case PVDRIVERS_GPLPV_WINDOWS_ID:
+        product = PVDRIVERS_GPLPV_WINDOWS_NAME;
+        break;
+    case PVDRIVERS_LINUX_ID:
+        product = PVDRIVERS_LINUX_NAME;
+        break;
+    case PVDRIVERS_EXPERIMENTAL_ID:
+        product = PVDRIVERS_EXPERIMENTAL_NAME; 
+        break;
     default:
         /* Don't know what product this is -> we can't blacklist
          * it. */
-- 
1.7.2.5