Re: [Qemu-devel] [PATCH 2/6] util: introduce glib-helper.c

[Dr. David Alan Gilbert]

* Peter Maydell (peter.mayd...@linaro.org) wrote:
> On 21 April 2017 at 16:10, Alexey  wrote:
> > Hello, thank you for so  detailed comment,
> >
> > On Fri, Apr 21, 2017 at 11:27:55AM +0100, Peter Maydell wrote:
> 
> >> Can we have a proper doc comment format comment, please,
> >> since this is now a function available to all of QEMU?
> >>
> >> > +gint g_int_cmp64(gconstpointer a, gconstpointer b,
> >> > +gpointer __attribute__((unused)) user_data);
> >>
> >> What is this actually for? Looking at the original uses
> >> I can tell that this is a GCompareDataFunc function, but
> >> the comment should tell me that.
> > I looked at another functions comments in QEMU, I didn't find
> > some common style, and decided keep it as is. Maybe I omitted some
> > best practice here.
> 
> See include/qemu/bitops.h for an example of the comment style.
> More important than just the style is that the comment
> should clearly explain the purpose of the function in detail.
> 
> Certainly many of our existing functions are poorly documented,
> but we're trying to raise the bar gradually here.
> 
> > yes, it was copy pasted,
> > right now, after mingw build check I think to use intptr_t as a type
> > for comparision in this function or even keep gpointer and merge these two
> > functions into _direct_.
> > I saw intptr_t is widely used in QEMU.
> >
> > The intent of this function was a comparator for case when client code
> > want to keep integers in pointer field. xen_disk.c uses UINT32 so it
> > wasn't a problem, but migration uses 64 address (kernel provides it in
> > __u64, long long), so on 32 platform it's a problem.
> 
> Code which tries to put a genuinely 64 bit value into a pointer
> is buggy and needs to be fixed. I'm not clear if that is the
> case here, or if the ABI from the kernel guarantees that the
> value is really a pointer type and fits in uintptr_t / gpointer.

It's a (probably masked) HVA, so always a valid pointer.

Dave

> I don't think we need more than one of these functions.
> 
> >> This is also missing the copyright line.
> > Yes, maybe it was better for me to ask before send.
> > I found in util files with reference to GNU GPL, version 2, like
> > in this file, also I found that
> >
> >  * This library is free software; you can redistribute it and/or
> >  * modify it under the terms of the GNU Lesser General Public
> >  * License as published by the Free Software Foundation; either
> >  * version 2 of the License, or (at your option) any later version.
> >
> > So I just copied copyright reference from glib-compat.h.
> 
> Yes, that's the license statement, which is fine. What is
> missing is the copyright line, which in glib-compat.h looks
> like:
>  Copyright IBM, Corp. 2013
> 
> For code you write, you want either your personal or (more likely)
> a Samsung copyright line -- check with your company about what
> their preferred form is.
> 
> thanks
> -- PMM
--
Dr. David Alan Gilbert / dgilb...@redhat.com / Manchester, UK

Re: [Qemu-devel] [PATCH 2/6] util: introduce glib-helper.c

[Peter Maydell]

On 21 April 2017 at 16:10, Alexey  wrote:
> Hello, thank you for so  detailed comment,
>
> On Fri, Apr 21, 2017 at 11:27:55AM +0100, Peter Maydell wrote:

>> Can we have a proper doc comment format comment, please,
>> since this is now a function available to all of QEMU?
>>
>> > +gint g_int_cmp64(gconstpointer a, gconstpointer b,
>> > +gpointer __attribute__((unused)) user_data);
>>
>> What is this actually for? Looking at the original uses
>> I can tell that this is a GCompareDataFunc function, but
>> the comment should tell me that.
> I looked at another functions comments in QEMU, I didn't find
> some common style, and decided keep it as is. Maybe I omitted some
> best practice here.

See include/qemu/bitops.h for an example of the comment style.
More important than just the style is that the comment
should clearly explain the purpose of the function in detail.

Certainly many of our existing functions are poorly documented,
but we're trying to raise the bar gradually here.

> yes, it was copy pasted,
> right now, after mingw build check I think to use intptr_t as a type
> for comparision in this function or even keep gpointer and merge these two
> functions into _direct_.
> I saw intptr_t is widely used in QEMU.
>
> The intent of this function was a comparator for case when client code
> want to keep integers in pointer field. xen_disk.c uses UINT32 so it
> wasn't a problem, but migration uses 64 address (kernel provides it in
> __u64, long long), so on 32 platform it's a problem.

Code which tries to put a genuinely 64 bit value into a pointer
is buggy and needs to be fixed. I'm not clear if that is the
case here, or if the ABI from the kernel guarantees that the
value is really a pointer type and fits in uintptr_t / gpointer.

I don't think we need more than one of these functions.

>> This is also missing the copyright line.
> Yes, maybe it was better for me to ask before send.
> I found in util files with reference to GNU GPL, version 2, like
> in this file, also I found that
>
>  * This library is free software; you can redistribute it and/or
>  * modify it under the terms of the GNU Lesser General Public
>  * License as published by the Free Software Foundation; either
>  * version 2 of the License, or (at your option) any later version.
>
> So I just copied copyright reference from glib-compat.h.

Yes, that's the license statement, which is fine. What is
missing is the copyright line, which in glib-compat.h looks
like:
 Copyright IBM, Corp. 2013

For code you write, you want either your personal or (more likely)
a Samsung copyright line -- check with your company about what
their preferred form is.

thanks
-- PMM

Re: [Qemu-devel] [PATCH 2/6] util: introduce glib-helper.c

[Alexey]

Hello, thank you for so  detailed comment,

On Fri, Apr 21, 2017 at 11:27:55AM +0100, Peter Maydell wrote:
> On 14 April 2017 at 14:17, Alexey Perevalov  wrote:
> > There is a lack of g_int_cmp which compares pointers value in glib,
> > xen_disk.c introduced its own, so the same function now requires
> > in migration.c. So logically to move it into common place.
> > Futher: maybe extend glib.
> >
> > Also this commit moves existing glib-compat.h into util/glib
> > folder for consolidation purpose.
> >
> > Signed-off-by: Alexey Perevalov 
> 
> Hi; thanks for this patch. I have some comments below, mostly
> aimed at improving the documentation in comments of what these
> new header files and functions are for -- the bar for "how
> much explanation do we need" moves up when a function is
> moved from being local to a single file to being available
> to all of QEMU.
> 
> > diff --git a/include/glib/glib-helper.h b/include/glib/glib-helper.h
> > new file mode 100644
> > index 000..db740fb
> > --- /dev/null
> > +++ b/include/glib/glib-helper.h
> > @@ -0,0 +1,30 @@
> > +/*
> > + * Helpers for GLIB
> > + *
> > + * This work is licensed under the terms of the GNU GPL, version 2 or 
> > later.
> > + * See the COPYING file in the top-level directory.
> > + *
> > + */
> 
> So glib-compat.h is for functions which exist in newer versions
> of glib but not older ones. What's this header for? Ideally the
> comment at the top of the file should make it clear what kinds
> of functions go here rather than elsewhere.
> 
> Also, GLib is capitalized like that, and you should have a
> Copyright line here.
> 
> > +
> > +#ifndef QEMU_GLIB_HELPER_H
> > +#define QEMU_GLIB_HELPER_H
> > +
> > +
> > +#include "glib/glib-compat.h"
> 
> Nothing needs to include glib-compat.h directly, because osdep.h does.
> 
> > +
> > +#define GPOINTER_TO_UINT64(a) ((guint64) (a))
> > +
> > +/*
> > + * return 1 in case of a > b, -1 otherwise and 0 if equeal
> > + */
> 
> Can we have a proper doc comment format comment, please,
> since this is now a function available to all of QEMU?
> 
> > +gint g_int_cmp64(gconstpointer a, gconstpointer b,
> > +gpointer __attribute__((unused)) user_data);
> 
> What is this actually for? Looking at the original uses
> I can tell that this is a GCompareDataFunc function, but
> the comment should tell me that.
I looked at another functions comments in QEMU, I didn't find
some common style, and decided keep it as is. Maybe I omitted some
best practice here.


> 
> It also looks very fishy because the function name suggests
> a 64 bit compare but gconstpointer may only be 32 bits.
> 
> I'm not sure it makes sense to specify the unused attribute
> on the function prototype -- that is a property of the
> implementation, not of the API exposed to callers, so it
> should go on the function definition IMHO.
> 
> > +
> > +/*
> > + * return 1 in case of a > b, -1 otherwise and 0 if equeal
> > + */
> 
> This is the same comment as above, so it doesn't explain
> what the difference between the two functions is.
> 

yes, it was copy pasted,
right now, after mingw build check I think to use intptr_t as a type
for comparision in this function or even keep gpointer and merge these two
functions into _direct_.
I saw intptr_t is widely used in QEMU.

The intent of this function was a comparator for case when client code
want to keep integers in pointer field. xen_disk.c uses UINT32 so it
wasn't a problem, but migration uses 64 address (kernel provides it in
__u64, long long), so on 32 platform it's a problem.
Fortunately userfaultfd handler is linux specific code, 
and I'm going to keep there just cast, like that GUINT_TO_POINTER

#define GUINT_TO_POINTER(u) ((gpointer) ${glib_gpui_cast} (u))

on 64 architecture glib_gpui_cast is guint64.

> > +int g_int_cmp(gconstpointer a, gconstpointer b,
> > +gpointer __attribute__((unused)) user_data);
> > +
> > +#endif /* QEMU_GLIB_HELPER_H */
> > +
> > diff --git a/include/qemu/osdep.h b/include/qemu/osdep.h
> > index 122ff06..36f8a89 100644
> > --- a/include/qemu/osdep.h
> > +++ b/include/qemu/osdep.h
> > @@ -104,7 +104,7 @@ extern int daemon(int, int);
> >  #include "sysemu/os-posix.h"
> >  #endif
> >
> > -#include "glib-compat.h"
> > +#include "glib/glib-compat.h"
> >  #include "qemu/typedefs.h"
> >
> >  #ifndef O_LARGEFILE
> > diff --git a/linux-user/main.c b/linux-user/main.c
> > index 10a3bb3..7cea6bc 100644
> > --- a/linux-user/main.c
> > +++ b/linux-user/main.c
> > @@ -35,7 +35,7 @@
> >  #include "elf.h"
> >  #include "exec/log.h"
> >  #include "trace/control.h"
> > -#include "glib-compat.h"
> > +#include "glib/glib-compat.h"
> 
> osdep.h includes glib-compat.h so we should just delete the #include,
> not change it.
> 
> This patch looks like it will break bsd-user compiles, because
> bsd-user/main.c has the same unnecessary glib-compat.h include
> and the patch doesn't change or delete it.
> 
> >
> >  char 

Re: [Qemu-devel] [PATCH 2/6] util: introduce glib-helper.c

[Peter Maydell]

On 14 April 2017 at 14:17, Alexey Perevalov  wrote:
> There is a lack of g_int_cmp which compares pointers value in glib,
> xen_disk.c introduced its own, so the same function now requires
> in migration.c. So logically to move it into common place.
> Futher: maybe extend glib.
>
> Also this commit moves existing glib-compat.h into util/glib
> folder for consolidation purpose.
>
> Signed-off-by: Alexey Perevalov 

Hi; thanks for this patch. I have some comments below, mostly
aimed at improving the documentation in comments of what these
new header files and functions are for -- the bar for "how
much explanation do we need" moves up when a function is
moved from being local to a single file to being available
to all of QEMU.

> diff --git a/include/glib/glib-helper.h b/include/glib/glib-helper.h
> new file mode 100644
> index 000..db740fb
> --- /dev/null
> +++ b/include/glib/glib-helper.h
> @@ -0,0 +1,30 @@
> +/*
> + * Helpers for GLIB
> + *
> + * This work is licensed under the terms of the GNU GPL, version 2 or later.
> + * See the COPYING file in the top-level directory.
> + *
> + */

So glib-compat.h is for functions which exist in newer versions
of glib but not older ones. What's this header for? Ideally the
comment at the top of the file should make it clear what kinds
of functions go here rather than elsewhere.

Also, GLib is capitalized like that, and you should have a
Copyright line here.

> +
> +#ifndef QEMU_GLIB_HELPER_H
> +#define QEMU_GLIB_HELPER_H
> +
> +
> +#include "glib/glib-compat.h"

Nothing needs to include glib-compat.h directly, because osdep.h does.

> +
> +#define GPOINTER_TO_UINT64(a) ((guint64) (a))
> +
> +/*
> + * return 1 in case of a > b, -1 otherwise and 0 if equeal
> + */

Can we have a proper doc comment format comment, please,
since this is now a function available to all of QEMU?

> +gint g_int_cmp64(gconstpointer a, gconstpointer b,
> +gpointer __attribute__((unused)) user_data);

What is this actually for? Looking at the original uses
I can tell that this is a GCompareDataFunc function, but
the comment should tell me that.

It also looks very fishy because the function name suggests
a 64 bit compare but gconstpointer may only be 32 bits.

I'm not sure it makes sense to specify the unused attribute
on the function prototype -- that is a property of the
implementation, not of the API exposed to callers, so it
should go on the function definition IMHO.

> +
> +/*
> + * return 1 in case of a > b, -1 otherwise and 0 if equeal
> + */

This is the same comment as above, so it doesn't explain
what the difference between the two functions is.

> +int g_int_cmp(gconstpointer a, gconstpointer b,
> +gpointer __attribute__((unused)) user_data);
> +
> +#endif /* QEMU_GLIB_HELPER_H */
> +
> diff --git a/include/qemu/osdep.h b/include/qemu/osdep.h
> index 122ff06..36f8a89 100644
> --- a/include/qemu/osdep.h
> +++ b/include/qemu/osdep.h
> @@ -104,7 +104,7 @@ extern int daemon(int, int);
>  #include "sysemu/os-posix.h"
>  #endif
>
> -#include "glib-compat.h"
> +#include "glib/glib-compat.h"
>  #include "qemu/typedefs.h"
>
>  #ifndef O_LARGEFILE
> diff --git a/linux-user/main.c b/linux-user/main.c
> index 10a3bb3..7cea6bc 100644
> --- a/linux-user/main.c
> +++ b/linux-user/main.c
> @@ -35,7 +35,7 @@
>  #include "elf.h"
>  #include "exec/log.h"
>  #include "trace/control.h"
> -#include "glib-compat.h"
> +#include "glib/glib-compat.h"

osdep.h includes glib-compat.h so we should just delete the #include,
not change it.

This patch looks like it will break bsd-user compiles, because
bsd-user/main.c has the same unnecessary glib-compat.h include
and the patch doesn't change or delete it.

>
>  char *exec_path;
>
> diff --git a/scripts/clean-includes b/scripts/clean-includes
> index dd938da..b32b928 100755
> --- a/scripts/clean-includes
> +++ b/scripts/clean-includes
> @@ -123,7 +123,7 @@ for f in "$@"; do
>;;
>  *include/qemu/osdep.h | \
>  *include/qemu/compiler.h | \
> -*include/glib-compat.h | \
> +*include/glib/glib-compat.h | \
>  *include/sysemu/os-posix.h | \
>  *include/sysemu/os-win32.h | \
>  *include/standard-headers/ )
> diff --git a/util/Makefile.objs b/util/Makefile.objs
> index c6205eb..0080712 100644
> --- a/util/Makefile.objs
> +++ b/util/Makefile.objs
> @@ -43,3 +43,4 @@ util-obj-y += qdist.o
>  util-obj-y += qht.o
>  util-obj-y += range.o
>  util-obj-y += systemd.o
> +util-obj-y += glib-helper.o
> diff --git a/util/glib-helper.c b/util/glib-helper.c
> new file mode 100644
> index 000..2557009
> --- /dev/null
> +++ b/util/glib-helper.c
> @@ -0,0 +1,29 @@
> +/*
> + * Implementation for GLIB helpers
> + * this file is intented to commulate and later reuse
> + * additional glib functions

Did you mean "accumulate" ?

More detailed description of what functions live in this
file would be useful -- these aren't actually GLib
functions, just utility routines that are useful 

Re: [Qemu-devel] [PATCH 2/6] util: introduce glib-helper.c

[Dr. David Alan Gilbert]

* Philippe Mathieu-Daudé (f4...@amsat.org) wrote:
> Hi Alexey,
> 
> On 04/14/2017 10:17 AM, Alexey Perevalov wrote:
> > There is a lack of g_int_cmp which compares pointers value in glib,
> > xen_disk.c introduced its own, so the same function now requires
> > in migration.c. So logically to move it into common place.
> > Futher: maybe extend glib.
> > 
> > Also this commit moves existing glib-compat.h into util/glib
> > folder for consolidation purpose.
> 
> Can you do this in two commits? First one moving files only, second move the
> function?

Yes, if you're lucky and do it with git mv  then perhaps git  will generate us 
a nice
small commit with a move in it rather than a vast delete/add.

> I'm not sure naming it "g_int_cmp()" won't clash with future _extended_
> glib, what do you think about naming it "qemu_g_int_cmp()"?

Also agreed.

Dave

> 
> > Signed-off-by: Alexey Perevalov 
> > ---
> >  hw/block/xen_disk.c|  10 +-
> >  include/glib-compat.h  | 352 
> > -
> >  include/glib/glib-compat.h | 352 
> > +
> >  include/glib/glib-helper.h |  30 
> >  include/qemu/osdep.h   |   2 +-
> >  linux-user/main.c  |   2 +-
> >  scripts/clean-includes |   2 +-
> >  util/Makefile.objs |   1 +
> >  util/glib-helper.c |  29 
> >  9 files changed, 417 insertions(+), 363 deletions(-)
> >  delete mode 100644 include/glib-compat.h
> >  create mode 100644 include/glib/glib-compat.h
> >  create mode 100644 include/glib/glib-helper.h
> >  create mode 100644 util/glib-helper.c
> > 
> > diff --git a/hw/block/xen_disk.c b/hw/block/xen_disk.c
> > index 456a2d5..36f6396 100644
> > --- a/hw/block/xen_disk.c
> > +++ b/hw/block/xen_disk.c
> > @@ -20,6 +20,7 @@
> >   */
> > 
> >  #include "qemu/osdep.h"
> > +#include "glib/glib-helper.h"
> >  #include 
> >  #include 
> > 
> > @@ -154,13 +155,6 @@ static void ioreq_reset(struct ioreq *ioreq)
> >  qemu_iovec_reset(>v);
> >  }
> > 
> > -static gint int_cmp(gconstpointer a, gconstpointer b, gpointer user_data)
> > -{
> > -uint ua = GPOINTER_TO_UINT(a);
> > -uint ub = GPOINTER_TO_UINT(b);
> > -return (ua > ub) - (ua < ub);
> > -}
> > -
> >  static void destroy_grant(gpointer pgnt)
> >  {
> >  PersistentGrant *grant = pgnt;
> > @@ -1191,7 +1185,7 @@ static int blk_connect(struct XenDevice *xendev)
> >  if (blkdev->feature_persistent) {
> >  /* Init persistent grants */
> >  blkdev->max_grants = max_requests * BLKIF_MAX_SEGMENTS_PER_REQUEST;
> > -blkdev->persistent_gnts = 
> > g_tree_new_full((GCompareDataFunc)int_cmp,
> > +blkdev->persistent_gnts = 
> > g_tree_new_full((GCompareDataFunc)g_int_cmp,
> >   NULL, NULL,
> >   batch_maps ?
> >   (GDestroyNotify)g_free :
> > diff --git a/include/glib-compat.h b/include/glib-compat.h
> > deleted file mode 100644
> > index 863c8cf..000
> > --- a/include/glib-compat.h
> > +++ /dev/null
> > @@ -1,352 +0,0 @@
> > -/*
> > - * GLIB Compatibility Functions
> > - *
> > - * Copyright IBM, Corp. 2013
> > - *
> > - * Authors:
> > - *  Anthony Liguori   
> > - *  Michael Tokarev   
> > - *  Paolo Bonzini 
> > - *
> > - * This work is licensed under the terms of the GNU GPL, version 2 or 
> > later.
> > - * See the COPYING file in the top-level directory.
> > - *
> > - */
> > -
> > -#ifndef QEMU_GLIB_COMPAT_H
> > -#define QEMU_GLIB_COMPAT_H
> > -
> > -#include 
> > -
> > -/* GLIB version compatibility flags */
> > -#if !GLIB_CHECK_VERSION(2, 26, 0)
> > -#define G_TIME_SPAN_SECOND  (G_GINT64_CONSTANT(100))
> > -#endif
> > -
> > -#if !GLIB_CHECK_VERSION(2, 28, 0)
> > -static inline gint64 qemu_g_get_monotonic_time(void)
> > -{
> > -/* g_get_monotonic_time() is best-effort so we can use the wall clock 
> > as a
> > - * fallback.
> > - */
> > -
> > -GTimeVal time;
> > -g_get_current_time();
> > -
> > -return time.tv_sec * G_TIME_SPAN_SECOND + time.tv_usec;
> > -}
> > -/* work around distro backports of this interface */
> > -#define g_get_monotonic_time() qemu_g_get_monotonic_time()
> > -#endif
> > -
> > -#if defined(_WIN32) && !GLIB_CHECK_VERSION(2, 50, 0)
> > -/*
> > - * g_poll has a problem on Windows when using
> > - * timeouts < 10ms, so use wrapper.
> > - */
> > -#define g_poll(fds, nfds, timeout) g_poll_fixed(fds, nfds, timeout)
> > -gint g_poll_fixed(GPollFD *fds, guint nfds, gint timeout);
> > -#endif
> > -
> > -#if !GLIB_CHECK_VERSION(2, 30, 0)
> > -/* Not a 100% compatible implementation, but good enough for most
> > - * cases. Placeholders are only supported at the end of the
> > - * template. */
> > -static inline gchar *qemu_g_dir_make_tmp(gchar const *tmpl, GError **error)
> > -{
> > -

Re: [Qemu-devel] [PATCH 2/6] util: introduce glib-helper.c

[Alexey]

Hi Philippe, 

On Fri, Apr 14, 2017 at 01:05:52PM -0300, Philippe Mathieu-Daudé wrote:
> Hi Alexey,
> 
> On 04/14/2017 10:17 AM, Alexey Perevalov wrote:
> >There is a lack of g_int_cmp which compares pointers value in glib,
> >xen_disk.c introduced its own, so the same function now requires
> >in migration.c. So logically to move it into common place.
> >Futher: maybe extend glib.
> >
> >Also this commit moves existing glib-compat.h into util/glib
> >folder for consolidation purpose.
> 
> Can you do this in two commits? First one moving files only, second
> move the function?
> 
Ok
> I'm not sure naming it "g_int_cmp()" won't clash with future
> _extended_ glib, what do you think about naming it
> "qemu_g_int_cmp()"?
> 
Why not, if it will have better maintainability.

> >Signed-off-by: Alexey Perevalov 
> >---
> > hw/block/xen_disk.c|  10 +-
> > include/glib-compat.h  | 352 
> > -
> > include/glib/glib-compat.h | 352 
> > +
> > include/glib/glib-helper.h |  30 
> > include/qemu/osdep.h   |   2 +-
> > linux-user/main.c  |   2 +-
> > scripts/clean-includes |   2 +-
> > util/Makefile.objs |   1 +
> > util/glib-helper.c |  29 
> > 9 files changed, 417 insertions(+), 363 deletions(-)
> > delete mode 100644 include/glib-compat.h
> > create mode 100644 include/glib/glib-compat.h
> > create mode 100644 include/glib/glib-helper.h
> > create mode 100644 util/glib-helper.c
> >
> >diff --git a/hw/block/xen_disk.c b/hw/block/xen_disk.c
> >index 456a2d5..36f6396 100644
> >--- a/hw/block/xen_disk.c
> >+++ b/hw/block/xen_disk.c
> >@@ -20,6 +20,7 @@
> >  */
> >
> > #include "qemu/osdep.h"
> >+#include "glib/glib-helper.h"
> > #include 
> > #include 
> >
> >@@ -154,13 +155,6 @@ static void ioreq_reset(struct ioreq *ioreq)
> > qemu_iovec_reset(>v);
> > }
> >
> >-static gint int_cmp(gconstpointer a, gconstpointer b, gpointer user_data)
> >-{
> >-uint ua = GPOINTER_TO_UINT(a);
> >-uint ub = GPOINTER_TO_UINT(b);
> >-return (ua > ub) - (ua < ub);
> >-}
> >-
> > static void destroy_grant(gpointer pgnt)
> > {
> > PersistentGrant *grant = pgnt;
> >@@ -1191,7 +1185,7 @@ static int blk_connect(struct XenDevice *xendev)
> > if (blkdev->feature_persistent) {
> > /* Init persistent grants */
> > blkdev->max_grants = max_requests * BLKIF_MAX_SEGMENTS_PER_REQUEST;
> >-blkdev->persistent_gnts = g_tree_new_full((GCompareDataFunc)int_cmp,
> >+blkdev->persistent_gnts = 
> >g_tree_new_full((GCompareDataFunc)g_int_cmp,
> >  NULL, NULL,
> >  batch_maps ?
> >  (GDestroyNotify)g_free :
> >diff --git a/include/glib-compat.h b/include/glib-compat.h
> >deleted file mode 100644
> >index 863c8cf..000
> >--- a/include/glib-compat.h
> >+++ /dev/null
> >@@ -1,352 +0,0 @@
> >-/*
> >- * GLIB Compatibility Functions
> >- *
> >- * Copyright IBM, Corp. 2013
> >- *
> >- * Authors:
> >- *  Anthony Liguori   
> >- *  Michael Tokarev   
> >- *  Paolo Bonzini 
> >- *
> >- * This work is licensed under the terms of the GNU GPL, version 2 or later.
> >- * See the COPYING file in the top-level directory.
> >- *
> >- */
> >-
> >-#ifndef QEMU_GLIB_COMPAT_H
> >-#define QEMU_GLIB_COMPAT_H
> >-
> >-#include 
> >-
> >-/* GLIB version compatibility flags */
> >-#if !GLIB_CHECK_VERSION(2, 26, 0)
> >-#define G_TIME_SPAN_SECOND  (G_GINT64_CONSTANT(100))
> >-#endif
> >-
> >-#if !GLIB_CHECK_VERSION(2, 28, 0)
> >-static inline gint64 qemu_g_get_monotonic_time(void)
> >-{
> >-/* g_get_monotonic_time() is best-effort so we can use the wall clock 
> >as a
> >- * fallback.
> >- */
> >-
> >-GTimeVal time;
> >-g_get_current_time();
> >-
> >-return time.tv_sec * G_TIME_SPAN_SECOND + time.tv_usec;
> >-}
> >-/* work around distro backports of this interface */
> >-#define g_get_monotonic_time() qemu_g_get_monotonic_time()
> >-#endif
> >-
> >-#if defined(_WIN32) && !GLIB_CHECK_VERSION(2, 50, 0)
> >-/*
> >- * g_poll has a problem on Windows when using
> >- * timeouts < 10ms, so use wrapper.
> >- */
> >-#define g_poll(fds, nfds, timeout) g_poll_fixed(fds, nfds, timeout)
> >-gint g_poll_fixed(GPollFD *fds, guint nfds, gint timeout);
> >-#endif
> >-
> >-#if !GLIB_CHECK_VERSION(2, 30, 0)
> >-/* Not a 100% compatible implementation, but good enough for most
> >- * cases. Placeholders are only supported at the end of the
> >- * template. */
> >-static inline gchar *qemu_g_dir_make_tmp(gchar const *tmpl, GError **error)
> >-{
> >-gchar *path = g_build_filename(g_get_tmp_dir(), tmpl ?: ".XX", 
> >NULL);
> >-
> >-if (mkdtemp(path) != NULL) {
> >-return path;
> >-}
> >-/* Error occurred, clean up. */
> >-

Re: [Qemu-devel] [PATCH 2/6] util: introduce glib-helper.c

[Philippe Mathieu-Daudé]

Hi Alexey,

On 04/14/2017 10:17 AM, Alexey Perevalov wrote:

There is a lack of g_int_cmp which compares pointers value in glib,
xen_disk.c introduced its own, so the same function now requires
in migration.c. So logically to move it into common place.
Futher: maybe extend glib.

Also this commit moves existing glib-compat.h into util/glib
folder for consolidation purpose.


Can you do this in two commits? First one moving files only, second move 
the function?


I'm not sure naming it "g_int_cmp()" won't clash with future _extended_ 
glib, what do you think about naming it "qemu_g_int_cmp()"?



Signed-off-by: Alexey Perevalov 
---
 hw/block/xen_disk.c|  10 +-
 include/glib-compat.h  | 352 -
 include/glib/glib-compat.h | 352 +
 include/glib/glib-helper.h |  30 
 include/qemu/osdep.h   |   2 +-
 linux-user/main.c  |   2 +-
 scripts/clean-includes |   2 +-
 util/Makefile.objs |   1 +
 util/glib-helper.c |  29 
 9 files changed, 417 insertions(+), 363 deletions(-)
 delete mode 100644 include/glib-compat.h
 create mode 100644 include/glib/glib-compat.h
 create mode 100644 include/glib/glib-helper.h
 create mode 100644 util/glib-helper.c

diff --git a/hw/block/xen_disk.c b/hw/block/xen_disk.c
index 456a2d5..36f6396 100644
--- a/hw/block/xen_disk.c
+++ b/hw/block/xen_disk.c
@@ -20,6 +20,7 @@
  */

 #include "qemu/osdep.h"
+#include "glib/glib-helper.h"
 #include 
 #include 

@@ -154,13 +155,6 @@ static void ioreq_reset(struct ioreq *ioreq)
 qemu_iovec_reset(>v);
 }

-static gint int_cmp(gconstpointer a, gconstpointer b, gpointer user_data)
-{
-uint ua = GPOINTER_TO_UINT(a);
-uint ub = GPOINTER_TO_UINT(b);
-return (ua > ub) - (ua < ub);
-}
-
 static void destroy_grant(gpointer pgnt)
 {
 PersistentGrant *grant = pgnt;
@@ -1191,7 +1185,7 @@ static int blk_connect(struct XenDevice *xendev)
 if (blkdev->feature_persistent) {
 /* Init persistent grants */
 blkdev->max_grants = max_requests * BLKIF_MAX_SEGMENTS_PER_REQUEST;
-blkdev->persistent_gnts = g_tree_new_full((GCompareDataFunc)int_cmp,
+blkdev->persistent_gnts = g_tree_new_full((GCompareDataFunc)g_int_cmp,
  NULL, NULL,
  batch_maps ?
  (GDestroyNotify)g_free :
diff --git a/include/glib-compat.h b/include/glib-compat.h
deleted file mode 100644
index 863c8cf..000
--- a/include/glib-compat.h
+++ /dev/null
@@ -1,352 +0,0 @@
-/*
- * GLIB Compatibility Functions
- *
- * Copyright IBM, Corp. 2013
- *
- * Authors:
- *  Anthony Liguori   
- *  Michael Tokarev   
- *  Paolo Bonzini 
- *
- * This work is licensed under the terms of the GNU GPL, version 2 or later.
- * See the COPYING file in the top-level directory.
- *
- */
-
-#ifndef QEMU_GLIB_COMPAT_H
-#define QEMU_GLIB_COMPAT_H
-
-#include 
-
-/* GLIB version compatibility flags */
-#if !GLIB_CHECK_VERSION(2, 26, 0)
-#define G_TIME_SPAN_SECOND  (G_GINT64_CONSTANT(100))
-#endif
-
-#if !GLIB_CHECK_VERSION(2, 28, 0)
-static inline gint64 qemu_g_get_monotonic_time(void)
-{
-/* g_get_monotonic_time() is best-effort so we can use the wall clock as a
- * fallback.
- */
-
-GTimeVal time;
-g_get_current_time();
-
-return time.tv_sec * G_TIME_SPAN_SECOND + time.tv_usec;
-}
-/* work around distro backports of this interface */
-#define g_get_monotonic_time() qemu_g_get_monotonic_time()
-#endif
-
-#if defined(_WIN32) && !GLIB_CHECK_VERSION(2, 50, 0)
-/*
- * g_poll has a problem on Windows when using
- * timeouts < 10ms, so use wrapper.
- */
-#define g_poll(fds, nfds, timeout) g_poll_fixed(fds, nfds, timeout)
-gint g_poll_fixed(GPollFD *fds, guint nfds, gint timeout);
-#endif
-
-#if !GLIB_CHECK_VERSION(2, 30, 0)
-/* Not a 100% compatible implementation, but good enough for most
- * cases. Placeholders are only supported at the end of the
- * template. */
-static inline gchar *qemu_g_dir_make_tmp(gchar const *tmpl, GError **error)
-{
-gchar *path = g_build_filename(g_get_tmp_dir(), tmpl ?: ".XX", NULL);
-
-if (mkdtemp(path) != NULL) {
-return path;
-}
-/* Error occurred, clean up. */
-g_set_error(error, G_FILE_ERROR, g_file_error_from_errno(errno),
-"mkdtemp() failed");
-g_free(path);
-return NULL;
-}
-#define g_dir_make_tmp(tmpl, error) qemu_g_dir_make_tmp(tmpl, error)
-#endif /* glib 2.30 */
-
-#if !GLIB_CHECK_VERSION(2, 31, 0)
-/* before glib-2.31, GMutex and GCond was dynamic-only (there was a separate
- * GStaticMutex, but it didn't work with condition variables).
- *
- * Our implementation uses GOnce to fake a static implementation that does
- * not require separate initialization.
- * We need to rename the types to