thr3ads.net - Xen devel - [PATCH] Use new Xen public header for product numbers and names [Sep 2012]

If this information is useful, please help other people find it:
Share via:

Paul Durrant

2012-Sep-26 12:33 UTC

[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.

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

Paul Durrant

2012-Sep-26 13:54 UTC

head link

[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.

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

Ian Jackson

2012-Sep-28 17:55 UTC

head link

Re: [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.

Ian.

Paul Durrant

2012-Oct-01 08:38 UTC

head link

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

> -----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

Paul Durrant

2012-Oct-01 09:43 UTC

head link

[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.

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

Ian Jackson

2012-Oct-01 10:13 UTC

head link

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

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.

Paul Durrant

2012-Oct-01 10:28 UTC

head link

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

> -----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

Keir Fraser

2012-Oct-01 10:36 UTC

head link

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

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

Ian Jackson

2012-Oct-01 10:40 UTC

head link

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

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.

Ian Jackson

2012-Oct-01 15:14 UTC

head link

Re: [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.

Ian.

Paul Durrant

2012-Oct-01 16:08 UTC

head link

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

> -----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

Ian Campbell

2012-Oct-02 09:25 UTC

head link

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

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.

Xen devel - Sep 2012 - [PATCH] Use new Xen public header for product numbers and names

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

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

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

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

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

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

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

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

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

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

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

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