[libvirt] [PATCH 0/7] fix debian 9 incompatibility with recent changes
Previous commits to eliminate gnulib modules accidentally used some glib APIs not available in Debian 9. GLib has support for compile time version checking of APIs but we mistakenly havent enabled it in libvirt. This series fixes the bad API usage, then enables version checkihng, and finally adds Debian to the Travis config Daniel P. Berrangé (7): util: always replace g_fsync usage with our wrapper util: keep glib compat methods in alphabetical order util: fix indent depth for glib compat impls util: add compat impl of g_canonicalize_filename src: remove use of g_date_time_new_from_iso8601 function util: introduce compile time API version checking travis: add build for Debian 9 .travis.yml | 7 ++ config-post.h | 10 ++ src/conf/domain_conf.c | 28 ++++-- src/esx/esx_vi_types.c | 56 +++++++++-- src/libvirt_private.syms | 1 + src/util/glibcompat.c | 196 ++++++++++++++++++++++++++++++++++----- src/util/glibcompat.h | 10 +- src/vz/vz_sdk.c | 24 ++++- 8 files changed, 286 insertions(+), 46 deletions(-) -- 2.24.1
g_fsync was introduced in 2.63 which is newer than our minimum glib version. A future commit will introduce compile time checking of API versions to prevent accidental usage of APIs from glib newer than our min declared. To avoid triggering this warning, however, we need to ensure that we always use our wrapper function via glibcompat.c, which will disable the API version warnings. Signed-off-by: Daniel P. Berrangé <berrange@redhat.com> --- src/util/glibcompat.c | 1 + src/util/glibcompat.h | 5 ++--- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/src/util/glibcompat.c b/src/util/glibcompat.c index 4ebefb4478..9fba54cb79 100644 --- a/src/util/glibcompat.c +++ b/src/util/glibcompat.c @@ -55,6 +55,7 @@ vir_g_strdup_vprintf(const char *msg, va_list args) } +/* Drop when min glib >= 2.63.0 */ gint vir_g_fsync(gint fd) { diff --git a/src/util/glibcompat.h b/src/util/glibcompat.h index d6b83f4b93..7878ad24ed 100644 --- a/src/util/glibcompat.h +++ b/src/util/glibcompat.h @@ -32,6 +32,5 @@ gint vir_g_fsync(gint fd); # define g_strdup_vprintf vir_g_strdup_vprintf #endif -#if !GLIB_CHECK_VERSION(2, 63, 0) -# define g_fsync vir_g_fsync -#endif +#undef g_fsync +#define g_fsync vir_g_fsync -- 2.24.1
On Mon, Jan 06, 2020 at 05:26:49PM +0000, Daniel P. Berrangé wrote:
g_fsync was introduced in 2.63 which is newer than our minimum glib version. A future commit will introduce compile time checking of API versions to prevent accidental usage of APIs from glib newer than our min declared.
To avoid triggering this warning, however, we need to ensure that we always use our wrapper function via glibcompat.c, which will disable the API version warnings.
Signed-off-by: Daniel P. Berrangé <berrange@redhat.com> --- src/util/glibcompat.c | 1 + src/util/glibcompat.h | 5 ++--- 2 files changed, 3 insertions(+), 3 deletions(-)
diff --git a/src/util/glibcompat.c b/src/util/glibcompat.c index 4ebefb4478..9fba54cb79 100644 --- a/src/util/glibcompat.c +++ b/src/util/glibcompat.c @@ -55,6 +55,7 @@ vir_g_strdup_vprintf(const char *msg, va_list args) }
+/* Drop when min glib >= 2.63.0 */
These "drop when something >= version" are usually overlooked and stick around longer then necessary, should we add a check that would produce compile time warning if we bump the minimal version of glib? Pavel
On Tue, Jan 07, 2020 at 09:08:03AM +0100, Pavel Hrdina wrote:
On Mon, Jan 06, 2020 at 05:26:49PM +0000, Daniel P. Berrangé wrote:
g_fsync was introduced in 2.63 which is newer than our minimum glib version. A future commit will introduce compile time checking of API versions to prevent accidental usage of APIs from glib newer than our min declared.
To avoid triggering this warning, however, we need to ensure that we always use our wrapper function via glibcompat.c, which will disable the API version warnings.
Signed-off-by: Daniel P. Berrangé <berrange@redhat.com> --- src/util/glibcompat.c | 1 + src/util/glibcompat.h | 5 ++--- 2 files changed, 3 insertions(+), 3 deletions(-)
diff --git a/src/util/glibcompat.c b/src/util/glibcompat.c index 4ebefb4478..9fba54cb79 100644 --- a/src/util/glibcompat.c +++ b/src/util/glibcompat.c @@ -55,6 +55,7 @@ vir_g_strdup_vprintf(const char *msg, va_list args) }
+/* Drop when min glib >= 2.63.0 */
These "drop when something >= version" are usually overlooked and stick around longer then necessary, should we add a check that would produce compile time warning if we bump the minimal version of glib?
I don't think that's a real problem with the way we do things here. The key value of having a glibcompat.{c,h} file is that it isolates the conditional logic in one place, so when bumping min glib we only need scan the glibcompat.{c,h} files for things to eliminate. So chance of missing this is minimal. Regards, Daniel -- |: https://berrange.com -o- https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o- https://fstop138.berrange.com :| |: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|
Signed-off-by: Daniel P. Berrangé <berrange@redhat.com> --- src/util/glibcompat.c | 26 +++++++++++++------------- src/util/glibcompat.h | 2 +- 2 files changed, 14 insertions(+), 14 deletions(-) diff --git a/src/util/glibcompat.c b/src/util/glibcompat.c index 9fba54cb79..e19cf74e72 100644 --- a/src/util/glibcompat.c +++ b/src/util/glibcompat.c @@ -23,9 +23,21 @@ #include "glibcompat.h" +#undef g_fsync #undef g_strdup_printf #undef g_strdup_vprintf -#undef g_fsync + + +gint +vir_g_fsync(gint fd) +{ +#ifdef G_OS_WIN32 + return _commit(fd); +#else + return fsync(fd); +#endif +} + /* Due to a bug in glib, g_strdup_printf() nor g_strdup_vprintf() * abort on OOM. It's fixed in glib's upstream. Provide our own @@ -53,15 +65,3 @@ vir_g_strdup_vprintf(const char *msg, va_list args) abort(); return ret; } - - -/* Drop when min glib >= 2.63.0 */ -gint -vir_g_fsync(gint fd) -{ -#ifdef G_OS_WIN32 - return _commit(fd); -#else - return fsync(fd); -#endif -} diff --git a/src/util/glibcompat.h b/src/util/glibcompat.h index 7878ad24ed..ce31a4de04 100644 --- a/src/util/glibcompat.h +++ b/src/util/glibcompat.h @@ -21,11 +21,11 @@ #include <glib.h> #include <glib/gstdio.h> +gint vir_g_fsync(gint fd); char *vir_g_strdup_printf(const char *msg, ...) G_GNUC_PRINTF(1, 2); char *vir_g_strdup_vprintf(const char *msg, va_list args) G_GNUC_PRINTF(1, 0); -gint vir_g_fsync(gint fd); #if !GLIB_CHECK_VERSION(2, 64, 0) # define g_strdup_printf vir_g_strdup_printf -- 2.24.1
On Mon, Jan 06, 2020 at 05:26:50PM +0000, Daniel P. Berrangé wrote:
Signed-off-by: Daniel P. Berrangé <berrange@redhat.com> --- src/util/glibcompat.c | 26 +++++++++++++------------- src/util/glibcompat.h | 2 +- 2 files changed, 14 insertions(+), 14 deletions(-)
diff --git a/src/util/glibcompat.c b/src/util/glibcompat.c index 9fba54cb79..e19cf74e72 100644 --- a/src/util/glibcompat.c +++ b/src/util/glibcompat.c @@ -23,9 +23,21 @@
#include "glibcompat.h"
+#undef g_fsync #undef g_strdup_printf #undef g_strdup_vprintf -#undef g_fsync + +
Missing comment when this function should be dropped.
+gint +vir_g_fsync(gint fd) +{ +#ifdef G_OS_WIN32 + return _commit(fd); +#else + return fsync(fd); +#endif +} +
/* Due to a bug in glib, g_strdup_printf() nor g_strdup_vprintf() * abort on OOM. It's fixed in glib's upstream. Provide our own @@ -53,15 +65,3 @@ vir_g_strdup_vprintf(const char *msg, va_list args) abort(); return ret; } - - -/* Drop when min glib >= 2.63.0 */ -gint -vir_g_fsync(gint fd) -{ -#ifdef G_OS_WIN32 - return _commit(fd); -#else - return fsync(fd); -#endif -} diff --git a/src/util/glibcompat.h b/src/util/glibcompat.h index 7878ad24ed..ce31a4de04 100644 --- a/src/util/glibcompat.h +++ b/src/util/glibcompat.h @@ -21,11 +21,11 @@ #include <glib.h> #include <glib/gstdio.h>
+gint vir_g_fsync(gint fd); char *vir_g_strdup_printf(const char *msg, ...) G_GNUC_PRINTF(1, 2); char *vir_g_strdup_vprintf(const char *msg, va_list args) G_GNUC_PRINTF(1, 0); -gint vir_g_fsync(gint fd);
#if !GLIB_CHECK_VERSION(2, 64, 0) # define g_strdup_printf vir_g_strdup_printf -- 2.24.1
-- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Signed-off-by: Daniel P. Berrangé <berrange@redhat.com> --- src/util/glibcompat.c | 30 +++++++++++++++--------------- 1 file changed, 15 insertions(+), 15 deletions(-) diff --git a/src/util/glibcompat.c b/src/util/glibcompat.c index e19cf74e72..42f9353c8f 100644 --- a/src/util/glibcompat.c +++ b/src/util/glibcompat.c @@ -32,9 +32,9 @@ gint vir_g_fsync(gint fd) { #ifdef G_OS_WIN32 - return _commit(fd); + return _commit(fd); #else - return fsync(fd); + return fsync(fd); #endif } @@ -45,23 +45,23 @@ vir_g_fsync(gint fd) char * vir_g_strdup_printf(const char *msg, ...) { - va_list args; - char *ret; - va_start(args, msg); - ret = g_strdup_vprintf(msg, args); - if (!ret) - abort(); - va_end(args); - return ret; + va_list args; + char *ret; + va_start(args, msg); + ret = g_strdup_vprintf(msg, args); + if (!ret) + abort(); + va_end(args); + return ret; } char * vir_g_strdup_vprintf(const char *msg, va_list args) { - char *ret; - ret = g_strdup_vprintf(msg, args); - if (!ret) - abort(); - return ret; + char *ret; + ret = g_strdup_vprintf(msg, args); + if (!ret) + abort(); + return ret; } -- 2.24.1
g_canonicalize_filename was not introduced until glib 2.58 so we need a temporary backport of its impl. Signed-off-by: Daniel P. Berrangé <berrange@redhat.com> --- src/libvirt_private.syms | 1 + src/util/glibcompat.c | 106 +++++++++++++++++++++++++++++++++++++++ src/util/glibcompat.h | 3 ++ 3 files changed, 110 insertions(+) diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index 951ba7f0ca..bb2b461cea 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -1504,6 +1504,7 @@ virSecurityManagerVerify; # util/glibcompat.h +vir_g_canonicalize_filename; vir_g_fsync; vir_g_strdup_printf; vir_g_strdup_vprintf; diff --git a/src/util/glibcompat.c b/src/util/glibcompat.c index 42f9353c8f..c6390c5c2e 100644 --- a/src/util/glibcompat.c +++ b/src/util/glibcompat.c @@ -19,15 +19,121 @@ #include <config.h> #include <stdlib.h> +#include <string.h> #include <unistd.h> #include "glibcompat.h" +#undef g_canonicalize_filename #undef g_fsync #undef g_strdup_printf #undef g_strdup_vprintf +gchar * +vir_g_canonicalize_filename(const gchar *filename, + const gchar *relative_to) +{ +#if GLIB_CHECK_VERSION(2, 58, 0) + return g_canonicalize_filename(filename, relative_to); +#else /* ! GLIB_CHECK_VERSION(2, 58, 0) */ + gchar *canon, *start, *p, *q; + guint i; + + g_return_val_if_fail(relative_to == NULL || g_path_is_absolute(relative_to), NULL); + + if (!g_path_is_absolute(filename)) { + gchar *cwd_allocated = NULL; + const gchar *cwd; + + if (relative_to != NULL) + cwd = relative_to; + else + cwd = cwd_allocated = g_get_current_dir(); + + canon = g_build_filename(cwd, filename, NULL); + g_free(cwd_allocated); + } else { + canon = g_strdup(filename); + } + + start = (char *)g_path_skip_root(canon); + + if (start == NULL) { + /* This shouldn't really happen, as g_get_current_dir() should + return an absolute pathname, but bug 573843 shows this is + not always happening */ + g_free(canon); + return g_build_filename(G_DIR_SEPARATOR_S, filename, NULL); + } + + /* POSIX allows double slashes at the start to + * mean something special (as does windows too). + * So, "//" != "/", but more than two slashes + * is treated as "/". + */ + i = 0; + for (p = start - 1; + (p >= canon) && + G_IS_DIR_SEPARATOR(*p); + p--) + i++; + if (i > 2) { + i -= 1; + start -= i; + memmove(start, start+i, strlen(start+i) + 1); + } + + /* Make sure we're using the canonical dir separator */ + p++; + while (p < start && G_IS_DIR_SEPARATOR(*p)) + *p++ = G_DIR_SEPARATOR; + + p = start; + while (*p != 0) { + if (p[0] == '.' && (p[1] == 0 || G_IS_DIR_SEPARATOR(p[1]))) { + memmove(p, p+1, strlen(p+1)+1); + } else if (p[0] == '.' && p[1] == '.' && + (p[2] == 0 || G_IS_DIR_SEPARATOR(p[2]))) { + q = p + 2; + /* Skip previous separator */ + p = p - 2; + if (p < start) + p = start; + while (p > start && !G_IS_DIR_SEPARATOR(*p)) + p--; + if (G_IS_DIR_SEPARATOR(*p)) + *p++ = G_DIR_SEPARATOR; + memmove(p, q, strlen(q)+1); + } else { + /* Skip until next separator */ + while (*p != 0 && !G_IS_DIR_SEPARATOR(*p)) + p++; + + if (*p != 0) { + /* Canonicalize one separator */ + *p++ = G_DIR_SEPARATOR; + } + } + + /* Remove additional separators */ + q = p; + while (*q && G_IS_DIR_SEPARATOR(*q)) + q++; + + if (p != q) + memmove(p, q, strlen(q) + 1); + } + + /* Remove trailing slashes */ + if (p > start && G_IS_DIR_SEPARATOR(*(p-1))) + *(p-1) = 0; + + return canon; +#endif /* ! GLIB_CHECK_VERSION(2, 58, 0) */ +} + + gint vir_g_fsync(gint fd) { diff --git a/src/util/glibcompat.h b/src/util/glibcompat.h index ce31a4de04..6f50a76f3c 100644 --- a/src/util/glibcompat.h +++ b/src/util/glibcompat.h @@ -21,6 +21,8 @@ #include <glib.h> #include <glib/gstdio.h> +gchar * vir_g_canonicalize_filename(const gchar *filename, + const gchar *relative_to); gint vir_g_fsync(gint fd); char *vir_g_strdup_printf(const char *msg, ...) G_GNUC_PRINTF(1, 2); @@ -32,5 +34,6 @@ char *vir_g_strdup_vprintf(const char *msg, va_list args) # define g_strdup_vprintf vir_g_strdup_vprintf #endif +#define g_canonicalize_filename vir_g_canonicalize_filename #undef g_fsync #define g_fsync vir_g_fsync -- 2.24.1
The g_date_time_new_from_iso8601() function was introduced as a replacement for strptime in commit 810613a60efe3924c536b3663246900bc08910a5 Author: Daniel P. Berrangé <berrange@redhat.com> Date: Mon Dec 23 15:37:26 2019 +0000 src: replace strptime()/timegm()/mktime() with GDateTime APIs set Unfortunately g_date_time_new_from_iso8601 isn't available until glib 2.56, and backporting it requires alot of code copying and poking at private glib structs. This reverts domain_conf.c back to its original parsing logic prior to 810613a60efe3924c536b3663246900bc08910a5, but using g_date_time_new() instead of gmtime(). The other files are then adapted to follow a similar approach. --- src/conf/domain_conf.c | 28 ++++++++++++++++----- src/esx/esx_vi_types.c | 56 +++++++++++++++++++++++++++++++++++++----- src/vz/vz_sdk.c | 24 ++++++++++++++---- 3 files changed, 91 insertions(+), 17 deletions(-) diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index ee33b7caf0..69967d4cb7 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -13675,15 +13675,31 @@ virDomainGraphicsAuthDefParseXML(xmlNodePtr node, if (validTo) { g_autoptr(GDateTime) then = NULL; g_autoptr(GTimeZone) tz = g_time_zone_new_utc(); - - then = g_date_time_new_from_iso8601(validTo, tz); - if (!then) { - virReportError(VIR_ERR_INVALID_ARG, - _("password validity time '%s' values out of range"), validTo); + char *tmp; + int year, mon, mday, hour, min, sec; + + /* Expect: YYYY-MM-DDTHH:MM:SS (%d-%d-%dT%d:%d:%d) eg 2010-11-28T14:29:01 */ + if (/* year */ + virStrToLong_i(validTo, &tmp, 10, &year) < 0 || *tmp != '-' || + /* month */ + virStrToLong_i(tmp+1, &tmp, 10, &mon) < 0 || *tmp != '-' || + /* day */ + virStrToLong_i(tmp+1, &tmp, 10, &mday) < 0 || *tmp != 'T' || + /* hour */ + virStrToLong_i(tmp+1, &tmp, 10, &hour) < 0 || *tmp != ':' || + /* minute */ + virStrToLong_i(tmp+1, &tmp, 10, &min) < 0 || *tmp != ':' || + /* second */ + virStrToLong_i(tmp+1, &tmp, 10, &sec) < 0 || *tmp != '\0') { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("cannot parse password validity time '%s', expect YYYY-MM-DDTHH:MM:SS"), + validTo); + VIR_FREE(def->passwd); return -1; } - def->validTo = (int)g_date_time_to_unix(then); + then = g_date_time_new(tz, year, mon, mday, hour, min, sec); + def->validTo = (time_t)g_date_time_to_unix(then); def->expires = true; } diff --git a/src/esx/esx_vi_types.c b/src/esx/esx_vi_types.c index 434313dfa4..ad40ddf54b 100644 --- a/src/esx/esx_vi_types.c +++ b/src/esx/esx_vi_types.c @@ -1473,8 +1473,10 @@ int esxVI_DateTime_ConvertToCalendarTime(esxVI_DateTime *dateTime, long long *secondsSinceEpoch) { + char *tmp; g_autoptr(GDateTime) then = NULL; g_autoptr(GTimeZone) tz = NULL; + int year, mon, mday, hour, min, sec, milliseconds; if (!dateTime || !secondsSinceEpoch) { virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("Invalid argument")); @@ -1489,22 +1491,64 @@ esxVI_DateTime_ConvertToCalendarTime(esxVI_DateTime *dateTime, * * map negative years to 0, since the base for time_t is the year 1970. */ - if (*(dateTime->value) == '-') { + if (dateTime->value[0] == '-') { *secondsSinceEpoch = 0; return 0; } - tz = g_time_zone_new_utc(); - then = g_date_time_new_from_iso8601(dateTime->value, tz); - - if (!then) { + if (/* year */ + virStrToLong_i(dateTime->value, &tmp, 10, &year) < 0 || *tmp != '-' || + /* month */ + virStrToLong_i(tmp+1, &tmp, 10, &mon) < 0 || *tmp != '-' || + /* day */ + virStrToLong_i(tmp+1, &tmp, 10, &mday) < 0 || *tmp != 'T' || + /* hour */ + virStrToLong_i(tmp+1, &tmp, 10, &hour) < 0 || *tmp != ':' || + /* minute */ + virStrToLong_i(tmp+1, &tmp, 10, &min) < 0 || *tmp != ':' || + /* second */ + virStrToLong_i(tmp+1, &tmp, 10, &sec) < 0) { virReportError(VIR_ERR_INTERNAL_ERROR, _("xsd:dateTime value '%s' has unexpected format"), dateTime->value); return -1; } - *secondsSinceEpoch = g_date_time_to_unix(then); + if (*tmp != '\0') { + /* skip .ssssss part if present */ + if (*tmp == '.' && + virStrToLong_i(tmp + 1, &tmp, 10, &milliseconds) < 0) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("xsd:dateTime value '%s' has unexpected format"), + dateTime->value); + return -1; + } + + /* parse timezone offset if present. if missing assume UTC */ + if (*tmp == '+' || *tmp == '-') { + tz = g_time_zone_new(tmp); + } else if (STREQ(tmp, "Z")) { + tz = g_time_zone_new_utc(); + } else { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("xsd:dateTime value '%s' has unexpected format"), + dateTime->value); + return -1; + } + } else { + tz = g_time_zone_new_utc(); + } + + /* + * xsd:dateTime represents local time relative to the optional timezone + * given as offset. pretend the local time is in UTC and use timegm in + * order to avoid interference with the timezone to this computer. + * apply timezone correction afterwards, because it's simpler than + * handling all the possible over- and underflows when trying to apply + * it to the tm struct. + */ + then = g_date_time_new(tz, year, mon, mday, hour, min, sec); + *secondsSinceEpoch = (long long)g_date_time_to_unix(then); return 0; } diff --git a/src/vz/vz_sdk.c b/src/vz/vz_sdk.c index c98542c244..26e9e38729 100644 --- a/src/vz/vz_sdk.c +++ b/src/vz/vz_sdk.c @@ -4608,16 +4608,30 @@ static long long prlsdkParseDateTime(const char *str) { g_autoptr(GDateTime) then = NULL; - g_autoptr(GTimeZone) tz = g_time_zone_new_local(); - - then = g_date_time_new_from_iso8601(str, tz); - if (!then) { + g_autoptr(GTimeZone) tz = g_time_zone_new_utc(); + char *tmp; + int year, mon, mday, hour, min, sec; + + /* Expect: YYYY-MM-DD HH:MM:SS (%d-%d-%dT%d:%d:%d) eg 2010-11-28 14:29:01 */ + if (/* year */ + virStrToLong_i(str, &tmp, 10, &year) < 0 || *tmp != '-' || + /* month */ + virStrToLong_i(tmp+1, &tmp, 10, &mon) < 0 || *tmp != '-' || + /* day */ + virStrToLong_i(tmp+1, &tmp, 10, &mday) < 0 || *tmp != ' ' || + /* hour */ + virStrToLong_i(tmp+1, &tmp, 10, &hour) < 0 || *tmp != ':' || + /* minute */ + virStrToLong_i(tmp+1, &tmp, 10, &min) < 0 || *tmp != ':' || + /* second */ + virStrToLong_i(tmp+1, &tmp, 10, &sec) < 0 || *tmp != '\0') { virReportError(VIR_ERR_INTERNAL_ERROR, _("unexpected DateTime format: '%s'"), str); return -1; } - return g_date_time_to_unix(then); + then = g_date_time_new(tz, year, mon, mday, hour, min, sec); + return (long long)g_date_time_to_unix(then); } static virDomainSnapshotObjListPtr -- 2.24.1
GLib header files annotate every API with a version number. It is possible to define some constants before including glib.h which will result in useful compile time warnings. Setting GLIB_VERSION_MIN_REQUIRED will result in a warning if libvirt uses an API that was deprecated in the declared version, or before. Such API usage should be rewritten to use the documented new replacement API. Setting GLIB_VERSION_MAX_ALLOWED will result in a warning if libvirt uses an API that was not introduced until a version of GLib that's newer than our minimum declared version. This avoids accidentally using functionality that is not available on some supported platforms. Signed-off-by: Daniel P. Berrangé <berrange@redhat.com> --- config-post.h | 10 ++++++++++ src/util/glibcompat.c | 39 +++++++++++++++++++++++++++++++++++++++ 2 files changed, 49 insertions(+) diff --git a/config-post.h b/config-post.h index 415cc8cc72..de007393da 100644 --- a/config-post.h +++ b/config-post.h @@ -49,3 +49,13 @@ #else # error You either need at least GCC 4.8 or Clang 3.4 or XCode Clang 5.1 to compile libvirt #endif + +/* Ask for warnings for anything that was marked deprecated in + * the defined version, or before. It is a candidate for rewrite. + */ +#define GLIB_VERSION_MIN_REQUIRED GLIB_VERSION_2_48 + +/* Ask for warnings if code tries to use function that did not + * exist in the defined version. These risk breaking builds + */ +#define GLIB_VERSION_MAX_ALLOWED GLIB_VERSION_2_48 diff --git a/src/util/glibcompat.c b/src/util/glibcompat.c index c6390c5c2e..06fe5b3d33 100644 --- a/src/util/glibcompat.c +++ b/src/util/glibcompat.c @@ -24,6 +24,45 @@ #include "glibcompat.h" +/* + * Note that because of the GLIB_VERSION_MAX_ALLOWED constant in + * config-post.h, allowing use of functions from newer GLib via + * this compat impl needs a little trickery to prevent warnings + * being emitted. + * + * Consider a function from newer glib-X.Y that we want to use + * + * int g_foo(const char *wibble) + * + * We must define a function with the same signature that does + * what we need, but with a "vir_" prefix e.g. + * + * void vir_g_foo(const char *wibble) + * { + * #if GLIB_CHECK_VERSION(X, Y, 0) + * g_foo(wibble) + * #else + * g_something_equivalent_in_older_glib(wibble); + * #endif + * } + * + * The #pragma at the top of this file turns off -Wdeprecated-declarations, + * ensuring this wrapper function impl doesn't trigger the compiler + * warning about using too new glib APIs. Finally in glibcompat.hu we can + * add + * + * #define g_foo(a) vir_g_foo(a) + * + * Thus all the code elsewhere in libvirt, which *does* have the + * -Wdeprecated-declarations warning active, can call g_foo(...) as + * normal, without generating warnings. The cost is an extra function + * call when using new glib, but this compat code will go away over + * time as we update the supported platforms target. + */ + +#pragma GCC diagnostic push +#pragma GCC diagnostic ignored "-Wdeprecated-declarations" + #undef g_canonicalize_filename #undef g_fsync #undef g_strdup_printf -- 2.24.1
Just a small nit: On 1/6/20 2:26 PM, Daniel P. Berrangé wrote:
GLib header files annotate every API with a version number.
It is possible to define some constants before including glib.h which will result in useful compile time warnings.
Setting GLIB_VERSION_MIN_REQUIRED will result in a warning if libvirt uses an API that was deprecated in the declared version, or before. Such API usage should be rewritten to use the documented new replacement API.
Setting GLIB_VERSION_MAX_ALLOWED will result in a warning if libvirt uses an API that was not introduced until a version of GLib that's newer than our minimum declared version. This avoids accidentally using functionality that is not available on some supported platforms.
Signed-off-by: Daniel P. Berrangé <berrange@redhat.com> --- config-post.h | 10 ++++++++++ src/util/glibcompat.c | 39 +++++++++++++++++++++++++++++++++++++++ 2 files changed, 49 insertions(+)
diff --git a/config-post.h b/config-post.h index 415cc8cc72..de007393da 100644 --- a/config-post.h +++ b/config-post.h @@ -49,3 +49,13 @@ #else # error You either need at least GCC 4.8 or Clang 3.4 or XCode Clang 5.1 to compile libvirt #endif + +/* Ask for warnings for anything that was marked deprecated in + * the defined version, or before. It is a candidate for rewrite. + */ +#define GLIB_VERSION_MIN_REQUIRED GLIB_VERSION_2_48 + +/* Ask for warnings if code tries to use function that did not + * exist in the defined version. These risk breaking builds + */ +#define GLIB_VERSION_MAX_ALLOWED GLIB_VERSION_2_48 diff --git a/src/util/glibcompat.c b/src/util/glibcompat.c index c6390c5c2e..06fe5b3d33 100644 --- a/src/util/glibcompat.c +++ b/src/util/glibcompat.c @@ -24,6 +24,45 @@
#include "glibcompat.h"
+/* + * Note that because of the GLIB_VERSION_MAX_ALLOWED constant in + * config-post.h, allowing use of functions from newer GLib via + * this compat impl needs a little trickery to prevent warnings + * being emitted. + * + * Consider a function from newer glib-X.Y that we want to use + * + * int g_foo(const char *wibble) + * + * We must define a function with the same signature that does + * what we need, but with a "vir_" prefix e.g. + * + * void vir_g_foo(const char *wibble) + * { + * #if GLIB_CHECK_VERSION(X, Y, 0) + * g_foo(wibble) + * #else + * g_something_equivalent_in_older_glib(wibble); + * #endif + * } + * + * The #pragma at the top of this file turns off -Wdeprecated-declarations, + * ensuring this wrapper function impl doesn't trigger the compiler + * warning about using too new glib APIs. Finally in glibcompat.hu we can
s/glibcompat.hu/glibcompat.h
+ * add + * + * #define g_foo(a) vir_g_foo(a) + * + * Thus all the code elsewhere in libvirt, which *does* have the + * -Wdeprecated-declarations warning active, can call g_foo(...) as + * normal, without generating warnings. The cost is an extra function + * call when using new glib, but this compat code will go away over + * time as we update the supported platforms target. + */ + +#pragma GCC diagnostic push +#pragma GCC diagnostic ignored "-Wdeprecated-declarations" + #undef g_canonicalize_filename #undef g_fsync #undef g_strdup_printf
Debian 9 ships the oldest versions of some of our dependent packages so can highlight bugs not seen elsewhere. CentOS 7, despite being quite old, has rebased some packages to much newer versions. Signed-off-by: Daniel P. Berrangé <berrange@redhat.com> --- .travis.yml | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/.travis.yml b/.travis.yml index e1e8cb4f80..b243e3d5c4 100644 --- a/.travis.yml +++ b/.travis.yml @@ -32,6 +32,13 @@ matrix: - MAKE_ARGS="syntax-check distcheck" script: - make -C ci/ ci-build@$IMAGE CI_MAKE_ARGS="$MAKE_ARGS" + - services: + - docker + env: + - IMAGE="debian-9" + - MAKE_ARGS="syntax-check distcheck" + script: + - make -C ci/ ci-build@$IMAGE CI_MAKE_ARGS="$MAKE_ARGS" - services: - docker env: -- 2.24.1
On 1/6/20 2:26 PM, Daniel P. Berrangé wrote:
Previous commits to eliminate gnulib modules accidentally used some glib APIs not available in Debian 9.
GLib has support for compile time version checking of APIs but we mistakenly havent enabled it in libvirt.
This series fixes the bad API usage, then enables version checkihng, and finally adds Debian to the Travis config
Daniel P. Berrangé (7): util: always replace g_fsync usage with our wrapper util: keep glib compat methods in alphabetical order util: fix indent depth for glib compat impls util: add compat impl of g_canonicalize_filename src: remove use of g_date_time_new_from_iso8601 function util: introduce compile time API version checking travis: add build for Debian 9
All patches: Reviewed-by: Daniel Henrique Barboza <danielhb413@gmail.com>
.travis.yml | 7 ++ config-post.h | 10 ++ src/conf/domain_conf.c | 28 ++++-- src/esx/esx_vi_types.c | 56 +++++++++-- src/libvirt_private.syms | 1 + src/util/glibcompat.c | 196 ++++++++++++++++++++++++++++++++++----- src/util/glibcompat.h | 10 +- src/vz/vz_sdk.c | 24 ++++- 8 files changed, 286 insertions(+), 46 deletions(-)
On Mon, Jan 06, 2020 at 05:26:48PM +0000, Daniel P. Berrangé wrote:
Previous commits to eliminate gnulib modules accidentally used some glib APIs not available in Debian 9.
GLib has support for compile time version checking of APIs but we mistakenly havent enabled it in libvirt.
This series fixes the bad API usage, then enables version checkihng, and finally adds Debian to the Travis config
Daniel P. Berrangé (7): util: always replace g_fsync usage with our wrapper util: keep glib compat methods in alphabetical order util: fix indent depth for glib compat impls util: add compat impl of g_canonicalize_filename src: remove use of g_date_time_new_from_iso8601 function util: introduce compile time API version checking travis: add build for Debian 9
Looks good, there are only some minor issues pointed out. Reviewed-by: Pavel Hrdina <phrdina@redhat.com>
participants (3)
-
Daniel Henrique Barboza -
Daniel P. Berrangé -
Pavel Hrdina