Xen devel - Apr 2013 - [PATCH] libxl: unconst the event argument to the event_occurs hook.

The event is supposed to become owned, and therefore freed, by the application
and the const prevents this.

Unfortunately there is no way to remove the const without breaking existing
callers. The best we can do is use the LIBXL_API_VERSION provisions to remove
the const for callers who wish only to support the 4.3 API and newer.

Callers who wish to support 4.2 will need to live with casting away the const.

Signed-off-by: Ian Campbell <ian.campbell@citrix.com>
Cc: Ian.Jackson@citrix.com
Cc: George.Dunlap@citix.com
Cc: Jim Fehlig <jfehlig@suse.com>
Cc: Rob Hoes <Rob.Hoes@citrix.com>
---
 tools/libxl/libxl.h       |   14 ++++++++++++--
 tools/libxl/libxl_event.h |    6 +++++-
 2 files changed, 17 insertions(+), 3 deletions(-)

diff --git a/tools/libxl/libxl.h b/tools/libxl/libxl.h
index 25efa76..ef96bce 100644
--- a/tools/libxl/libxl.h
+++ b/tools/libxl/libxl.h
@@ -273,9 +273,9 @@
 #include <libxl_uuid.h>
 #include <_libxl_list.h>
 
-/* API compatibility. Only 0x040200 is supported at this time. */
+/* API compatibility. */
 #ifdef LIBXL_API_VERSION
-#if LIBXL_API_VERSION != 0x040200
+#if LIBXL_API_VERSION != 0x040200 && LIBXL_API_VERSION != 0x040300
 #error Unknown LIBXL_API_VERSION
 #endif
 #endif
@@ -308,6 +308,16 @@
  */
 #define LIBXL_HAVE_DEVICE_BACKEND_DOMNAME 1
 
+/*
+ * LIBXL_HAVE_NONCONST_EVENT_OCCURS_EVENT_ARG
+ *
+ * This argument was erroneously "const" in the 4.2 release despite
+ * the requirement for the callback to free the event.
+ */
+#if LIBXL_API_VERSION != 0x040200
+#define LIBXL_HAVE_NONCONST_EVENT_OCCURS_EVENT_ARG 1
+#endif
+
 /* Functions annotated with LIBXL_EXTERNAL_CALLERS_ONLY may not be
  * called from within libxl itself. Callers outside libxl, who
  * do not #include libxl_internal.h, are fine. */
diff --git a/tools/libxl/libxl_event.h b/tools/libxl/libxl_event.h
index 51f2721..27a65dc 100644
--- a/tools/libxl/libxl_event.h
+++ b/tools/libxl/libxl_event.h
@@ -64,7 +64,11 @@ void libxl_event_free(libxl_ctx *ctx, libxl_event *event);
 
 typedef struct libxl_event_hooks {
     uint64_t event_occurs_mask;
-    void (*event_occurs)(void *user, const libxl_event *event);
+    void (*event_occurs)(void *user,
+#ifndef LIBXL_HAVE_NONCONST_EVENT_OCCURS_EVENT_ARG
+                         const
+#endif
+                         libxl_event *event);
     void (*disaster)(void *user, libxl_event_type type,
                      const char *msg, int errnoval);
 } libxl_event_hooks;
-- 
1.7.2.5

Ian Campbell wrote:
> The event is supposed to become owned, and therefore freed, by the
application
> and the const prevents this.
>
> Unfortunately there is no way to remove the const without breaking existing
> callers. The best we can do is use the LIBXL_API_VERSION provisions to
remove
> the const for callers who wish only to support the 4.3 API and newer.
>
> Callers who wish to support 4.2 will need to live with casting away the
const.
>
> Signed-off-by: Ian Campbell <ian.campbell@citrix.com>
> Cc: Ian.Jackson@citrix.com
> Cc: George.Dunlap@citix.com
> Cc: Jim Fehlig <jfehlig@suse.com>
> Cc: Rob Hoes <Rob.Hoes@citrix.com>
> ---
>  tools/libxl/libxl.h       |   14 ++++++++++++--
>  tools/libxl/libxl_event.h |    6 +++++-
>  2 files changed, 17 insertions(+), 3 deletions(-)
>
> diff --git a/tools/libxl/libxl.h b/tools/libxl/libxl.h
> index 25efa76..ef96bce 100644
> --- a/tools/libxl/libxl.h
> +++ b/tools/libxl/libxl.h
> @@ -273,9 +273,9 @@
>  #include <libxl_uuid.h>
>  #include <_libxl_list.h>
>  
> -/* API compatibility. Only 0x040200 is supported at this time. */
> +/* API compatibility. */
>  #ifdef LIBXL_API_VERSION
> -#if LIBXL_API_VERSION != 0x040200
> +#if LIBXL_API_VERSION != 0x040200 && LIBXL_API_VERSION != 0x040300
>  #error Unknown LIBXL_API_VERSION
>  #endif
>  #endif
>   
Should this hunk be in a separate patch? It seems to be introducing a
new API version :).
> @@ -308,6 +308,16 @@
>   */
>  #define LIBXL_HAVE_DEVICE_BACKEND_DOMNAME 1
>  
> +/*
> + * LIBXL_HAVE_NONCONST_EVENT_OCCURS_EVENT_ARG
> + *
> + * This argument was erroneously "const" in the 4.2 release
despite
> + * the requirement for the callback to free the event.
> + */
> +#if LIBXL_API_VERSION != 0x040200
> +#define LIBXL_HAVE_NONCONST_EVENT_OCCURS_EVENT_ARG 1
> +#endif
> +
>  /* Functions annotated with LIBXL_EXTERNAL_CALLERS_ONLY may not be
>   * called from within libxl itself. Callers outside libxl, who
>   * do not #include libxl_internal.h, are fine. */
> diff --git a/tools/libxl/libxl_event.h b/tools/libxl/libxl_event.h
> index 51f2721..27a65dc 100644
> --- a/tools/libxl/libxl_event.h
> +++ b/tools/libxl/libxl_event.h
> @@ -64,7 +64,11 @@ void libxl_event_free(libxl_ctx *ctx, libxl_event
*event);
>  
>  typedef struct libxl_event_hooks {
>      uint64_t event_occurs_mask;
> -    void (*event_occurs)(void *user, const libxl_event *event);
> +    void (*event_occurs)(void *user,
> +#ifndef LIBXL_HAVE_NONCONST_EVENT_OCCURS_EVENT_ARG
> +                         const
> +#endif
> +                         libxl_event *event);
>      void (*disaster)(void *user, libxl_event_type type,
>                       const char *msg, int errnoval);
>  } libxl_event_hooks;
>   
Otherwise,

Reviewed-by: Jim Fehlig <jfehlig@suse.com>

> > diff --git a/tools/libxl/libxl.h b/tools/libxl/libxl.h
> > index 25efa76..ef96bce 100644
> > --- a/tools/libxl/libxl.h
> > +++ b/tools/libxl/libxl.h
> > @@ -273,9 +273,9 @@
> >  #include <libxl_uuid.h>
> >  #include <_libxl_list.h>
> >  
> > -/* API compatibility. Only 0x040200 is supported at this time. */
> > +/* API compatibility. */
> >  #ifdef LIBXL_API_VERSION
> > -#if LIBXL_API_VERSION != 0x040200
> > +#if LIBXL_API_VERSION != 0x040200 && LIBXL_API_VERSION !=
0x040300
> >  #error Unknown LIBXL_API_VERSION
> >  #endif
> >  #endif
> >   
> 
> Should this hunk be in a separate patch? It seems to be introducing a
> new API version :).
I suppose it could be, or we could argue that this patch is introducing
the first user of this API version?
> > @@ -308,6 +308,16 @@
> >   */
> >  #define LIBXL_HAVE_DEVICE_BACKEND_DOMNAME 1
> >  
> > +/*
> > + * LIBXL_HAVE_NONCONST_EVENT_OCCURS_EVENT_ARG
> > + *
> > + * This argument was erroneously "const" in the 4.2 release
despite
> > + * the requirement for the callback to free the event.
> > + */
> > +#if LIBXL_API_VERSION != 0x040200
Looking at this again today this happens to be true if LIBXL_API_VERSION
is undefined but I wonder if we ought to explicitly define
LIBXL_API_VERSION to 0xffffff if the user doesn''t supply it?
> Reviewed-by: Jim Fehlig <jfehlig@suse.com>
> 
Thanks,
Ian.

> > diff --git a/tools/libxl/libxl.h b/tools/libxl/libxl.h
> > index 25efa76..ef96bce 100644
> > --- a/tools/libxl/libxl.h
> > +++ b/tools/libxl/libxl.h
> > @@ -273,9 +273,9 @@
> >  #include <libxl_uuid.h>
> >  #include <_libxl_list.h>
> >  
> > -/* API compatibility. Only 0x040200 is supported at this time. */
> > +/* API compatibility. */
> >  #ifdef LIBXL_API_VERSION
> > -#if LIBXL_API_VERSION != 0x040200
> > +#if LIBXL_API_VERSION != 0x040200 && LIBXL_API_VERSION !=
0x040300
> >  #error Unknown LIBXL_API_VERSION
> >  #endif
> >  #endif
> >   
> 
> Should this hunk be in a separate patch? It seems to be introducing a
> new API version :).
I suppose it could be, or we could argue that this patch is introducing
the first user of this API version?
> > @@ -308,6 +308,16 @@
> >   */
> >  #define LIBXL_HAVE_DEVICE_BACKEND_DOMNAME 1
> >  
> > +/*
> > + * LIBXL_HAVE_NONCONST_EVENT_OCCURS_EVENT_ARG
> > + *
> > + * This argument was erroneously "const" in the 4.2 release
despite
> > + * the requirement for the callback to free the event.
> > + */
> > +#if LIBXL_API_VERSION != 0x040200
Looking at this again today this happens to be true if LIBXL_API_VERSION
is undefined but I wonder if we ought to explicitly define
LIBXL_API_VERSION to 0xffffff if the user doesn''t supply it?
> Reviewed-by: Jim Fehlig <jfehlig@suse.com>
> 
Thanks,
Ian.

On Fri, Apr 26, 2013 at 12:42 PM, Ian Campbell <ian.campbell@citrix.com>
wrote:
> The event is supposed to become owned, and therefore freed, by the
application
> and the const prevents this.
>
> Unfortunately there is no way to remove the const without breaking existing
> callers. The best we can do is use the LIBXL_API_VERSION provisions to
remove
> the const for callers who wish only to support the 4.3 API and newer.
>
> Callers who wish to support 4.2 will need to live with casting away the
const.
>
> Signed-off-by: Ian Campbell <ian.campbell@citrix.com>
> Cc: Ian.Jackson@citrix.com
> Cc: George.Dunlap@citix.com
Ah, I wondered why I didn''t receive this in my work inbox --
you''re
missing the ''r'' in Citrix.

It seems like this is necessary for a bug fix in the libvirt side; it
would have been nice to have this earlier in the release cycle, but
it''s too late to worry about that now. :-)

Re the release:

Acked-by: George Dunlap <george.dunlap@eu.citrix.com>

Ian Campbell wrote:
>>> diff --git a/tools/libxl/libxl.h b/tools/libxl/libxl.h
>>> index 25efa76..ef96bce 100644
>>> --- a/tools/libxl/libxl.h
>>> +++ b/tools/libxl/libxl.h
>>> @@ -273,9 +273,9 @@
>>>  #include <libxl_uuid.h>
>>>  #include <_libxl_list.h>
>>>  
>>> -/* API compatibility. Only 0x040200 is supported at this time. */
>>> +/* API compatibility. */
>>>  #ifdef LIBXL_API_VERSION
>>> -#if LIBXL_API_VERSION != 0x040200
>>> +#if LIBXL_API_VERSION != 0x040200 && LIBXL_API_VERSION !=
0x040300
>>>  #error Unknown LIBXL_API_VERSION
>>>  #endif
>>>  #endif
>>>   
>>>       
>> Should this hunk be in a separate patch? It seems to be introducing a
>> new API version :).
>>     
>
> I suppose it could be, or we could argue that this patch is introducing
> the first user of this API version?
>   
Ok.
>   
>>> @@ -308,6 +308,16 @@
>>>   */
>>>  #define LIBXL_HAVE_DEVICE_BACKEND_DOMNAME 1
>>>  
>>> +/*
>>> + * LIBXL_HAVE_NONCONST_EVENT_OCCURS_EVENT_ARG
>>> + *
>>> + * This argument was erroneously "const" in the 4.2
release despite
>>> + * the requirement for the callback to free the event.
>>> + */
>>> +#if LIBXL_API_VERSION != 0x040200
>>>       
>
> Looking at this again today this happens to be true if LIBXL_API_VERSION
> is undefined but I wonder if we ought to explicitly define
> LIBXL_API_VERSION to 0xffffff if the user doesn''t supply it?
>   
Ah, good point.  But setting LIBXL_API_VERSION to 0xffffffff if the app
doesn''t supply it means the app, which e.g. built fine on 4.2, would no
longer compile on 4.3 right?

Regards,
Jim

On Tue, 2013-04-30 at 05:24 +0100, Jim Fehlig wrote:
> > Looking at this again today this happens to be true if
LIBXL_API_VERSION
> > is undefined but I wonder if we ought to explicitly define
> > LIBXL_API_VERSION to 0xffffff if the user doesn''t supply it?
> >   
> 
> Ah, good point.  But setting LIBXL_API_VERSION to 0xffffffff if the app
> doesn''t supply it means the app, which e.g. built fine on 4.2,
would no
> longer compile on 4.3 right?
Yes, this is the same as if LIBXL_API_VERSION remains undefined though,
an app which doesn''t ask for a specific version automatically gets the
latest version.

From libxl.h:

 * In order to make such compatibility possible it is required that
 * application which want to be exposed to a particular API #define
 * LIBXL_API_VERSION before including libxl.h or any other libxl
 * header. The syntax of the LIBXL_API_VERSION is:
 *    0xVVSSEE
 * where ($(XEN_xxx) from xen/Makefile):
 *   VV is the Xen major release number, $(XEN_VERSION)
 *   SS is the Xen sub version number, $(XEN_SUBVERSION)
 *   EE is the Xen extra version digit, first numeric part of
 *     $(XEN_EXTRAVERSION) not including the leading "."
 * For example the first stable API version, supported by Xen 4.2.0,
 * is 0x040200.
 *
 * Lack of LIBXL_API_VERSION means "the latest" which will
 * change. Specifying an unknown LIBXL_API_VERSION will result in a
 * compile time error.
(the whole comment is probably worth a read)

If you are concerned about supporting 4.2 onwards then you need to
#define LIBXL_API_VERSION to 0x040200.

Ian.

Ian Campbell wrote:
> On Tue, 2013-04-30 at 05:24 +0100, Jim Fehlig wrote:
>   
>>> Looking at this again today this happens to be true if
LIBXL_API_VERSION
>>> is undefined but I wonder if we ought to explicitly define
>>> LIBXL_API_VERSION to 0xffffff if the user doesn''t supply
it?
>>>   
>>>       
>> Ah, good point.  But setting LIBXL_API_VERSION to 0xffffffff if the app
>> doesn''t supply it means the app, which e.g. built fine on 4.2,
would no
>> longer compile on 4.3 right?
>>     
>
> Yes, this is the same as if LIBXL_API_VERSION remains undefined though,
> an app which doesn''t ask for a specific version automatically gets
the
> latest version.
>
> From libxl.h:
>
>  * In order to make such compatibility possible it is required that
>  * application which want to be exposed to a particular API #define
>  * LIBXL_API_VERSION before including libxl.h or any other libxl
>  * header. The syntax of the LIBXL_API_VERSION is:
>  *    0xVVSSEE
>  * where ($(XEN_xxx) from xen/Makefile):
>  *   VV is the Xen major release number, $(XEN_VERSION)
>  *   SS is the Xen sub version number, $(XEN_SUBVERSION)
>  *   EE is the Xen extra version digit, first numeric part of
>  *     $(XEN_EXTRAVERSION) not including the leading "."
>  * For example the first stable API version, supported by Xen 4.2.0,
>  * is 0x040200.
>  *
>  * Lack of LIBXL_API_VERSION means "the latest" which will
>  * change. Specifying an unknown LIBXL_API_VERSION will result in a
>  * compile time error.
> (the whole comment is probably worth a read)
>   
Yes, I did read it (again) :). And a later paragraph in the comment
explains bumping the version on the first incompatible change

* These guarantees apply only to stable releases of Xen. When an
* incompatible change is made in the unstable tree then
* LIBXL_API_VERSION will be bumped to the next expected stable
* release number on the first such change only.

which addresses my comment earlier in this thread about the version bump
being in a separate patch.
> If you are concerned about supporting 4.2 onwards then you need to
> #define LIBXL_API_VERSION to 0x040200.
>   
Yes, understood.

Regards,
Jim

Ian Campbell writes ("[PATCH] libxl: unconst the event argument to the
event_occurs hook."):
> The event is supposed to become owned, and therefore freed, by the
application
> and the const prevents this.
> 
> Unfortunately there is no way to remove the const without breaking existing
> callers. The best we can do is use the LIBXL_API_VERSION provisions to
remove
> the const for callers who wish only to support the 4.3 API and newer.
> 
> Callers who wish to support 4.2 will need to live with casting away the
const.
> 
> Signed-off-by: Ian Campbell <ian.campbell@citrix.com>
> Cc: Ian.Jackson@citrix.com
> Cc: George.Dunlap@citix.com
> Cc: Jim Fehlig <jfehlig@suse.com>
> Cc: Rob Hoes <Rob.Hoes@citrix.com>
Acked-by: Ian Jackson <ian.jackson@eu.citrix.com>

Jim Fehlig writes ("Re: [Xen-devel] [PATCH] libxl: unconst the event
argument to the event_occurs hook."):
> > -/* API compatibility. Only 0x040200 is supported at this time. */
> > +/* API compatibility. */
> >  #ifdef LIBXL_API_VERSION
> > -#if LIBXL_API_VERSION != 0x040200
> > +#if LIBXL_API_VERSION != 0x040200 && LIBXL_API_VERSION !=
0x040300
> >  #error Unknown LIBXL_API_VERSION
...
> Should this hunk be in a separate patch? It seems to be introducing a
> new API version :).
Certainly not.  The actual API actually changes in the rest of the
patch.

Thanks,
Ian.

On Wed, 2013-05-01 at 11:48 +0100, Ian Jackson wrote:
> Ian Campbell writes ("[PATCH] libxl: unconst the event argument to the
event_occurs hook."):
> > The event is supposed to become owned, and therefore freed, by the
application
> > and the const prevents this.
> > 
> > Unfortunately there is no way to remove the const without breaking
existing
> > callers. The best we can do is use the LIBXL_API_VERSION provisions to
remove
> > the const for callers who wish only to support the 4.3 API and newer.
> > 
> > Callers who wish to support 4.2 will need to live with casting away
the const.
> > 
> > Signed-off-by: Ian Campbell <ian.campbell@citrix.com>
> > Cc: Ian.Jackson@citrix.com
> > Cc: George.Dunlap@citix.com
> > Cc: Jim Fehlig <jfehlig@suse.com>
> > Cc: Rob Hoes <Rob.Hoes@citrix.com>
> 
> Acked-by: Ian Jackson <ian.jackson@eu.citrix.com>
Applied with this + Jim''s previous R-b, thanks both.

Ian.