diff mbox

[2/2] Compile time checks for newer glib

Message ID 1432911708-22845-3-git-send-email-dgilbert@redhat.com
State New
Headers show

Commit Message

Dr. David Alan Gilbert May 29, 2015, 3:01 p.m. UTC 
From: "Dr. David Alan Gilbert" <dgilbert@redhat.com>

Newer glib has support for checking that applications aren't
using newer glib calls than they should be.

The support for the check only went into glib 2.32 and it only
has macros for version 2.26 upwards; although we only insist
on 2.22 at the moment, I set the glib checks to the earliest of 2.26,
it wont cause problems on anything <2.32 since the checks aren't
there.

While mainly we're interested in the check from GLIB_VERSION_MAX_ALLOWED
which checks we're not using anything too recent, the glib macros
require that if we set that then we must also define
GLIB_VERSION_MIN_REQUIRED which checks we're not using anything
deprecated too long ago.

Signed-off-by: Dr. David Alan Gilbert <dgilbert@redhat.com>
---
 configure                      | 5 +++++
 include/glib-compat.h          | 6 ++++++
 tests/test-qdev-global-props.c | 2 +-
 tests/vhost-user-test.c        | 2 ++
 4 files changed, 14 insertions(+), 1 deletion(-)

Comments

Paolo Bonzini May 29, 2015, 3:37 p.m. UTC | #1 
On 29/05/2015 17:01, Dr. David Alan Gilbert (git) wrote:
> From: "Dr. David Alan Gilbert" <dgilbert@redhat.com>
> 
> Newer glib has support for checking that applications aren't
> using newer glib calls than they should be.
> 
> The support for the check only went into glib 2.32 and it only
> has macros for version 2.26 upwards; although we only insist
> on 2.22 at the moment, I set the glib checks to the earliest of 2.26,
> it wont cause problems on anything <2.32 since the checks aren't
> there.

I think we need 2.32 minimum, because we use the 2.32 mutex/condvar
functions if available.

Paolo
Dr. David Alan Gilbert May 29, 2015, 3:49 p.m. UTC | #2 
* Paolo Bonzini (pbonzini@redhat.com) wrote:
> 
> 
> On 29/05/2015 17:01, Dr. David Alan Gilbert (git) wrote:
> > From: "Dr. David Alan Gilbert" <dgilbert@redhat.com>
> > 
> > Newer glib has support for checking that applications aren't
> > using newer glib calls than they should be.
> > 
> > The support for the check only went into glib 2.32 and it only
> > has macros for version 2.26 upwards; although we only insist
> > on 2.22 at the moment, I set the glib checks to the earliest of 2.26,
> > it wont cause problems on anything <2.32 since the checks aren't
> > there.
> 
> I think we need 2.32 minimum, because we use the 2.32 mutex/condvar
> functions if available.

Are all those done via glib-compat.h ?
Note that in the patch I turn off the version checks at the top of glib-compat.h
on the assumption that anything trying to use it knows what it is doing.

Dave

> 
> Paolo
--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK
Peter Maydell May 29, 2015, 3:53 p.m. UTC | #3 
On 29 May 2015 at 16:01, Dr. David Alan Gilbert (git)
<dgilbert@redhat.com> wrote:
> --- a/include/glib-compat.h
> +++ b/include/glib-compat.h
> @@ -16,6 +16,12 @@
>  #ifndef QEMU_GLIB_COMPAT_H
>  #define QEMU_GLIB_COMPAT_H
>
> +/*
> + * The source file including this compat header knows it's using newer glib
> + * functions than we generally allow, so don't warn about it.
> + */
> +#undef GLIB_VERSION_MIN_REQUIRED
> +#undef GLIB_VERSION_MAX_ALLOWED
>  #include <glib.h>

...but glib-compat.h is included by qemu-common.h and in general
the point is that code anywhere in QEMU can assume the compat
versions exist, so it feels like this will defeat the point.

-- PMM
Dr. David Alan Gilbert May 29, 2015, 4:01 p.m. UTC | #4 
* Peter Maydell (peter.maydell@linaro.org) wrote:
> On 29 May 2015 at 16:01, Dr. David Alan Gilbert (git)
> <dgilbert@redhat.com> wrote:
> > --- a/include/glib-compat.h
> > +++ b/include/glib-compat.h
> > @@ -16,6 +16,12 @@
> >  #ifndef QEMU_GLIB_COMPAT_H
> >  #define QEMU_GLIB_COMPAT_H
> >
> > +/*
> > + * The source file including this compat header knows it's using newer glib
> > + * functions than we generally allow, so don't warn about it.
> > + */
> > +#undef GLIB_VERSION_MIN_REQUIRED
> > +#undef GLIB_VERSION_MAX_ALLOWED
> >  #include <glib.h>
> 
> ...but glib-compat.h is included by qemu-common.h and in general
> the point is that code anywhere in QEMU can assume the compat
> versions exist, so it feels like this will defeat the point.

Hmm, a lot of files seem to include glib.h directly.  But OK,
so then Paolo is suggesting setting the minimum to 2.32
which I can do, but then that wouldn't stop something that's
in 2.22-2.32 but not in glib-compat.h

Dave

> 
> -- PMM
--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK
John Snow June 1, 2015, 6:01 p.m. UTC | #5 
On 05/29/2015 12:01 PM, Dr. David Alan Gilbert wrote:
> * Peter Maydell (peter.maydell@linaro.org) wrote:
>> On 29 May 2015 at 16:01, Dr. David Alan Gilbert (git)
>> <dgilbert@redhat.com> wrote:
>>> --- a/include/glib-compat.h
>>> +++ b/include/glib-compat.h
>>> @@ -16,6 +16,12 @@
>>>  #ifndef QEMU_GLIB_COMPAT_H
>>>  #define QEMU_GLIB_COMPAT_H
>>>
>>> +/*
>>> + * The source file including this compat header knows it's using newer glib
>>> + * functions than we generally allow, so don't warn about it.
>>> + */
>>> +#undef GLIB_VERSION_MIN_REQUIRED
>>> +#undef GLIB_VERSION_MAX_ALLOWED
>>>  #include <glib.h>
>>
>> ...but glib-compat.h is included by qemu-common.h and in general
>> the point is that code anywhere in QEMU can assume the compat
>> versions exist, so it feels like this will defeat the point.
> 
> Hmm, a lot of files seem to include glib.h directly.  But OK,
> so then Paolo is suggesting setting the minimum to 2.32
> which I can do, but then that wouldn't stop something that's
> in 2.22-2.32 but not in glib-compat.h
> 
> Dave
> 
>>
>> -- PMM
> --
> Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK
> 

When I bumped glib to 2.22, I played with using these macros and forcing
all files to include glib-compat and prohibiting direct includes of glib.

The problem is without a minimum version set to 2.3x, as you've stated,
the deprecated warnings will trigger for functions we added explicit
compat wrappers for, and I could think of no sane way to "forgive" these
usages as a one-time exception.

Perhaps -- /perhaps/ -- you could have a qemu-internal glib.h that sets
the version warnings and leave glib-compat alone without those includes,
but this seems like a setup that is inviting frustration for future devs.

--js
diff mbox

Patch

diff --git a/configure b/configure
index 24ee0a4..5c6230a 100755
--- a/configure
+++ b/configure
@@ -2781,6 +2781,11 @@  fi
 
 glib_req_ver=2.22
 glib_modules=gthread-2.0
+# Force warnings over use of newer glib features; 2.26 is the earliest
+# version macro that is defined; eventually we should match
+# glib_req_ver above
+QEMU_CFLAGS="-DGLIB_VERSION_MIN_REQUIRED=GLIB_VERSION_2_26 $QEMU_CFLAGS"
+QEMU_CFLAGS="-DGLIB_VERSION_MAX_ALLOWED=GLIB_VERSION_2_26 $QEMU_CFLAGS"
 if test "$modules" = yes; then
     glib_modules="$glib_modules gmodule-2.0"
 fi
diff --git a/include/glib-compat.h b/include/glib-compat.h
index 318e000..cbd660c 100644
--- a/include/glib-compat.h
+++ b/include/glib-compat.h
@@ -16,6 +16,12 @@ 
 #ifndef QEMU_GLIB_COMPAT_H
 #define QEMU_GLIB_COMPAT_H
 
+/*
+ * The source file including this compat header knows it's using newer glib
+ * functions than we generally allow, so don't warn about it.
+ */
+#undef GLIB_VERSION_MIN_REQUIRED
+#undef GLIB_VERSION_MAX_ALLOWED
 #include <glib.h>
 
 /* GLIB version compatibility flags */
diff --git a/tests/test-qdev-global-props.c b/tests/test-qdev-global-props.c
index 0be9835..896e6bf 100644
--- a/tests/test-qdev-global-props.c
+++ b/tests/test-qdev-global-props.c
@@ -22,7 +22,7 @@ 
  * THE SOFTWARE.
  */
 
-#include <glib.h>
+#include "glib-compat.h"
 #include <stdint.h>
 
 #include "hw/qdev.h"
diff --git a/tests/vhost-user-test.c b/tests/vhost-user-test.c
index 75fedf0..7aeb927 100644
--- a/tests/vhost-user-test.c
+++ b/tests/vhost-user-test.c
@@ -8,6 +8,8 @@ 
  *
  */
 
+#undef GLIB_VERSION_MIN_REQUIRED
+#undef GLIB_VERSION_MAX_ALLOWED
 #define QEMU_GLIB_COMPAT_H
 #include <glib.h>