On 14 April 2017 at 14:17, Alexey Perevalov <a.pereva...@samsung.com> 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 <a.pereva...@samsung.com>
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 0000000..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 0000000..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 to code which uses GLib, as far as I can tell. > + * > + * This work is licensed under the terms of the GNU GPL, version 2 or later. > + * See the COPYING file in the top-level directory. > + Stray blank line. > + */ This is also missing the copyright line. > + > +#include "glib/glib-helper.h" Every C file should start by including "qemu/osdep.h" as the first thing it does. > + > +gint g_int_cmp64(gconstpointer a, gconstpointer b, > + gpointer __attribute__((unused)) user_data) > +{ > + guint64 ua = GPOINTER_TO_UINT64(a); > + guint64 ub = GPOINTER_TO_UINT64(b); > + return (ua > ub) - (ua < ub); > +} > + > +/* > + * return 1 in case of a > b, -1 otherwise and 0 if equeal > + */ > +gint g_int_cmp(gconstpointer a, gconstpointer b, > + gpointer __attribute__((unused)) user_data) > +{ > + return g_int_cmp64(a, b, user_data); > +} > + > -- > 1.8.3.1 > > thanks -- PMM