Prepare for linking with glib by probing for it at configure time. Per supported platforms target, the min glib versions on relevant distros are: RHEL-8: 2.56.1 RHEL-7: 2.50.3 Debian (Buster): 2.58.3 Debian (Stretch): 2.50.3 OpenBSD (Ports): 2.58.3 FreeBSD (Ports): 2.56.3 OpenSUSE Leap 15: 2.54.3 SLE12-SP2: 2.48.2 Ubuntu (Xenial): 2.48.0 macOS (Homebrew): 2.56.0 This suggests that a minimum glib of 2.48 is a reasonable target. This aligns with the minimum version required by qemu too. Signed-off-by: Daniel P. Berrangé <berrange@redhat.com> --- configure.ac | 2 ++ libvirt.spec.in | 1 + m4/virt-glib.m4 | 36 ++++++++++++++++++++++++++++++++++++ mingw-libvirt.spec.in | 2 ++ 4 files changed, 41 insertions(+) create mode 100644 m4/virt-glib.m4 diff --git a/configure.ac b/configure.ac index af8cbcdfd8..573c3d85f2 100644 --- a/configure.ac +++ b/configure.ac @@ -317,6 +317,7 @@ LIBVIRT_CHECK_DLOPEN LIBVIRT_CHECK_FIREWALLD LIBVIRT_CHECK_FIREWALLD_ZONE LIBVIRT_CHECK_FUSE +LIBVIRT_CHECK_GLIB LIBVIRT_CHECK_GLUSTER LIBVIRT_CHECK_GNUTLS LIBVIRT_CHECK_HAL @@ -1007,6 +1008,7 @@ LIBVIRT_RESULT_DLOPEN LIBVIRT_RESULT_FIREWALLD LIBVIRT_RESULT_FIREWALLD_ZONE LIBVIRT_RESULT_FUSE +LIBVIRT_RESULT_GLIB LIBVIRT_RESULT_GLUSTER LIBVIRT_RESULT_GNUTLS LIBVIRT_RESULT_HAL diff --git a/libvirt.spec.in b/libvirt.spec.in index 7f5183f341..dcad08cb5f 100644 --- a/libvirt.spec.in +++ b/libvirt.spec.in @@ -273,6 +273,7 @@ BuildRequires: systemd-units %if %{with_libxl} BuildRequires: xen-devel %endif +BuildRequires: glib2-devel >= 2.48 BuildRequires: libxml2-devel BuildRequires: libxslt BuildRequires: readline-devel diff --git a/m4/virt-glib.m4 b/m4/virt-glib.m4 new file mode 100644 index 0000000000..5a5bc19660 --- /dev/null +++ b/m4/virt-glib.m4 @@ -0,0 +1,36 @@ +dnl The glib.so library +dnl +dnl Copyright (C) 2016 Red Hat, Inc. +dnl +dnl This library is free software; you can redistribute it and/or +dnl modify it under the terms of the GNU Lesser General Public +dnl License as published by the Free Software Foundation; either +dnl version 2.1 of the License, or (at your option) any later version. +dnl +dnl This library is distributed in the hope that it will be useful, +dnl but WITHOUT ANY WARRANTY; without even the implied warranty of +dnl MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU +dnl Lesser General Public License for more details. +dnl +dnl You should have received a copy of the GNU Lesser General Public +dnl License along with this library. If not, see +dnl <http://www.gnu.org/licenses/>. +dnl + +AC_DEFUN([LIBVIRT_ARG_GLIB], [ + LIBVIRT_ARG_WITH([GLIB], [glib-2.0 location], [check]) +]) + +AC_DEFUN([LIBVIRT_CHECK_GLIB],[ + GLIB_REQUIRED=2.48.0 + + LIBVIRT_CHECK_PKG([GLIB], [glib-2.0], [$GLIB_REQUIRED]) + + if test "$with_glib" = "no" ; then + AC_MSG_ERROR([glib-2.0 >= $GLIB_REQUIRED is required for libvirt]) + fi +]) + +AC_DEFUN([LIBVIRT_RESULT_GLIB], [ + LIBVIRT_RESULT_LIB([GLIB]) +]) diff --git a/mingw-libvirt.spec.in b/mingw-libvirt.spec.in index a20c4b7d74..c29f3eeed2 100644 --- a/mingw-libvirt.spec.in +++ b/mingw-libvirt.spec.in @@ -52,6 +52,8 @@ BuildRequires: mingw32-gcc BuildRequires: mingw64-gcc BuildRequires: mingw32-binutils BuildRequires: mingw64-binutils +BuildRequires: mingw32-glib2 >= 2.48 +BuildRequires: mingw64-glib2 >= 2.48 BuildRequires: mingw32-libgpg-error BuildRequires: mingw64-libgpg-error BuildRequires: mingw32-libgcrypt -- 2.21.0

Add the main glib.h to internal.h so that all common code can use it. Historically glib allowed applications to register an alternative memory allocator, so mixing g_malloc/g_free with malloc/free was not safe. This was feature was dropped in 2.46.0 with: commit 3be6ed60aa58095691bd697344765e715a327fc1 Author: Alexander Larsson <alexl@redhat.com> Date: Sat Jun 27 18:38:42 2015 +0200 Deprecate and drop support for memory vtables Applications are still encourged to match g_malloc/g_free, but it is no longer a mandatory requirement for correctness, just stylistic. This is explicitly clarified in commit 1f24b36607bf708f037396014b2cdbc08d67b275 Author: Daniel P. Berrangé <berrange@redhat.com> Date: Thu Sep 5 14:37:54 2019 +0100 gmem: clarify that g_malloc always uses the system allocator Applications can still use custom allocators in general, but they must do this by linking to a library that replaces the core malloc/free implemenentation entirely, instead of via a glib specific call. This means that libvirt does not need to be concerned about use of g_malloc/g_free causing an ABI change in the public libary, and can avoid memory copying when talking to external libraries. This patch probes for glib, gobject and gio. Glib provides the foundation layer with a collection of data structures, helper APIs, and platform portability logic. GObject provides the object type system, built on glib. GIO builds on GObject, providing objects for various interesting tasks, most notably including DBus client and server support and portable sockets APIs, but much more too. Signed-off-by: Daniel P. Berrangé <berrange@redhat.com> --- m4/virt-glib.m4 | 4 ++-- src/Makefile.am | 2 ++ src/internal.h | 1 + src/lxc/Makefile.inc.am | 2 ++ src/remote/Makefile.inc.am | 1 + src/util/Makefile.inc.am | 1 + tests/Makefile.am | 3 ++- tools/Makefile.am | 1 + 8 files changed, 12 insertions(+), 3 deletions(-) diff --git a/m4/virt-glib.m4 b/m4/virt-glib.m4 index 5a5bc19660..e9992e9824 100644 --- a/m4/virt-glib.m4 +++ b/m4/virt-glib.m4 @@ -24,10 +24,10 @@ AC_DEFUN([LIBVIRT_ARG_GLIB], [ AC_DEFUN([LIBVIRT_CHECK_GLIB],[ GLIB_REQUIRED=2.48.0 - LIBVIRT_CHECK_PKG([GLIB], [glib-2.0], [$GLIB_REQUIRED]) + LIBVIRT_CHECK_PKG([GLIB], [glib-2.0 gobject-2.0 gio-2.0], [$GLIB_REQUIRED]) if test "$with_glib" = "no" ; then - AC_MSG_ERROR([glib-2.0 >= $GLIB_REQUIRED is required for libvirt]) + AC_MSG_ERROR([glib-2.0, gobject-2.0, gio-2.0 >= $GLIB_REQUIRED is required for libvirt]) fi ]) diff --git a/src/Makefile.am b/src/Makefile.am index cd955ee552..66ba710e50 100644 --- a/src/Makefile.am +++ b/src/Makefile.am @@ -34,6 +34,7 @@ AM_CPPFLAGS = -I../gnulib/lib \ WARN_CFLAGS += $(STRICT_FRAME_LIMIT_CFLAGS) AM_CFLAGS = $(LIBXML_CFLAGS) \ + $(GLIB_CFLAGS) \ $(WARN_CFLAGS) \ $(LOCK_CHECKING_CFLAGS) \ $(WIN32_EXTRA_CFLAGS) \ @@ -559,6 +560,7 @@ libvirt_admin_la_LIBADD += \ $(YAJL_LIBS) \ $(DEVMAPPER_LIBS) \ $(LIBXML_LIBS) \ + $(GLIB_LIBS) \ $(SSH2_LIBS) \ $(SASL_LIBS) \ $(GNUTLS_LIBS) \ diff --git a/src/internal.h b/src/internal.h index adc1e3f496..55aaf937cf 100644 --- a/src/internal.h +++ b/src/internal.h @@ -27,6 +27,7 @@ #include <stdint.h> #include <stdio.h> #include <string.h> +#include <glib.h> #if STATIC_ANALYSIS # undef NDEBUG /* Don't let a prior NDEBUG definition cause trouble. */ diff --git a/src/lxc/Makefile.inc.am b/src/lxc/Makefile.inc.am index b4d560702c..0c9618e185 100644 --- a/src/lxc/Makefile.inc.am +++ b/src/lxc/Makefile.inc.am @@ -184,6 +184,7 @@ libvirt_lxc_LDFLAGS = \ $(PIE_LDFLAGS) \ $(CAPNG_LIBS) \ $(LIBXML_LIBS) \ + $(GLIB_LIBS) \ $(NULL) libvirt_lxc_LDADD = \ libvirt.la \ @@ -200,6 +201,7 @@ libvirt_lxc_CFLAGS = \ $(PIE_CFLAGS) \ $(CAPNG_CFLAGS) \ $(LIBXML_CFLAGS) \ + $(GLIB_CFLAGS) \ $(LIBNL_CFLAGS) \ $(FUSE_CFLAGS) \ $(DBUS_CFLAGS) \ diff --git a/src/remote/Makefile.inc.am b/src/remote/Makefile.inc.am index 5a5c90a922..e02c20d47c 100644 --- a/src/remote/Makefile.inc.am +++ b/src/remote/Makefile.inc.am @@ -38,6 +38,7 @@ REMOTE_DAEMON_SOURCES = \ REMOTE_DAEMON_CFLAGS = \ $(LIBXML_CFLAGS) \ + $(GLIB_CFLAGS) \ $(GNUTLS_CFLAGS) \ $(SASL_CFLAGS) \ $(XDR_CFLAGS) \ diff --git a/src/util/Makefile.inc.am b/src/util/Makefile.inc.am index 482b657a90..a415378fe2 100644 --- a/src/util/Makefile.inc.am +++ b/src/util/Makefile.inc.am @@ -289,6 +289,7 @@ libvirt_util_la_LIBADD = \ $(DBUS_LIBS) \ $(WIN32_EXTRA_LIBS) \ $(LIBXML_LIBS) \ + $(GLIB_LIBS) \ $(SECDRIVER_LIBS) \ $(NUMACTL_LIBS) \ $(ACL_LIBS) \ diff --git a/tests/Makefile.am b/tests/Makefile.am index d88ad7f686..7b81ee88f1 100644 --- a/tests/Makefile.am +++ b/tests/Makefile.am @@ -35,6 +35,7 @@ AM_CFLAGS = \ -Dabs_srcdir="\"$(abs_srcdir)\"" \ -Dabs_top_srcdir="\"$(abs_top_srcdir)\"" \ $(LIBXML_CFLAGS) \ + $(GLIB_CFLAGS) \ $(LIBNL_CFLAGS) \ $(GNUTLS_CFLAGS) \ $(SASL_CFLAGS) \ @@ -525,7 +526,7 @@ libxlxml2domconfigtest_LDADD = $(libxl_LDADDS) $(LIBXML_LIBS) libxlmock_la_SOURCES = \ libxlmock.c -libxlmock_la_CFLAGS = $(LIBXL_CFLAGS) $(LIBXML_CFLAGS) +libxlmock_la_CFLAGS = $(LIBXL_CFLAGS) $(LIBXML_CFLAGS) $(GLIB_CFLAGS) libxlmock_la_LDFLAGS = $(MOCKLIBS_LDFLAGS) libxlmock_la_LIBADD = $(MOCKLIBS_LIBS) diff --git a/tools/Makefile.am b/tools/Makefile.am index 29fdbfe846..9489cb35db 100644 --- a/tools/Makefile.am +++ b/tools/Makefile.am @@ -36,6 +36,7 @@ AM_CFLAGS = \ $(COVERAGE_CFLAGS) \ $(PIE_CFLAGS) \ $(LIBXML_CFLAGS) \ + $(GLIB_CFLAGS) \ $(NULL) AM_LDFLAGS = \ -- 2.21.0

The VIR_TYPED_PARAM_* enum fields are defined in libvirt-common.h, not in the remote protcol, so shouldn't be part of the protocol structs output check. This avoids similar problems hitting when we add use of glib, which has other such anonymous enums. Signed-off-by: Daniel P. Berrangé <berrange@redhat.com> --- src/Makefile.am | 3 +-- src/admin_protocol-structs | 9 --------- src/remote_protocol-structs | 9 --------- 3 files changed, 1 insertion(+), 20 deletions(-) diff --git a/src/Makefile.am b/src/Makefile.am index 66ba710e50..9d21395892 100644 --- a/src/Makefile.am +++ b/src/Makefile.am @@ -241,8 +241,7 @@ PDWTAGS = \ else \ $(PERL) -0777 -n \ -e 'foreach my $$p (split m!\n*(?:$(r1)|$(r2))\n!) {' \ - -e ' if ($$p =~ /^(struct|enum) $(struct_prefix)/ ||' \ - -e ' $$p =~ /^enum \{/) {' \ + -e ' if ($$p =~ /^(struct|enum) $(struct_prefix)/) {' \ -e ' $$p =~ s!\t*/\*.*?\*/!!sg;' \ -e ' $$p =~ s!\s+\n!\n!sg;' \ -e ' $$p =~ s!\s+$$!!;' \ diff --git a/src/admin_protocol-structs b/src/admin_protocol-structs index a6c53807ee..983e6e5292 100644 --- a/src/admin_protocol-structs +++ b/src/admin_protocol-structs @@ -1,13 +1,4 @@ /* -*- c -*- */ -enum { - VIR_TYPED_PARAM_INT = 1, - VIR_TYPED_PARAM_UINT = 2, - VIR_TYPED_PARAM_LLONG = 3, - VIR_TYPED_PARAM_ULLONG = 4, - VIR_TYPED_PARAM_DOUBLE = 5, - VIR_TYPED_PARAM_BOOLEAN = 6, - VIR_TYPED_PARAM_STRING = 7, -}; struct admin_typed_param_value { int type; union { diff --git a/src/remote_protocol-structs b/src/remote_protocol-structs index eb689b3574..51606e7473 100644 --- a/src/remote_protocol-structs +++ b/src/remote_protocol-structs @@ -1,13 +1,4 @@ /* -*- c -*- */ -enum { - VIR_TYPED_PARAM_INT = 1, - VIR_TYPED_PARAM_UINT = 2, - VIR_TYPED_PARAM_LLONG = 3, - VIR_TYPED_PARAM_ULLONG = 4, - VIR_TYPED_PARAM_DOUBLE = 5, - VIR_TYPED_PARAM_BOOLEAN = 6, - VIR_TYPED_PARAM_STRING = 7, -}; struct remote_nonnull_domain { remote_nonnull_string name; remote_uuid uuid; -- 2.21.0

Convert the VIR_ALLOC family of APIs with use of the g_malloc family of APIs. Use of VIR_ALLOC related functions should be incrementally phased out over time, allowing return value checks to be dropped. Use of VIR_FREE should be replaced with auto-cleanup whenever possible. We previously used the 'calloc-posix' gnulib module because mingw does not set errno to ENOMEM on failure. Signed-off-by: Daniel P. Berrangé <berrange@redhat.com> --- bootstrap.conf | 1 - src/util/viralloc.c | 29 ++++++----------------------- 2 files changed, 6 insertions(+), 24 deletions(-) diff --git a/bootstrap.conf b/bootstrap.conf index 4f944a9c23..549d18c6d4 100644 --- a/bootstrap.conf +++ b/bootstrap.conf @@ -28,7 +28,6 @@ byteswap c-ctype c-strcase c-strcasestr -calloc-posix canonicalize-lgpl chown clock-time diff --git a/src/util/viralloc.c b/src/util/viralloc.c index 10a8d0fb73..b8ca850764 100644 --- a/src/util/viralloc.c +++ b/src/util/viralloc.c @@ -45,10 +45,7 @@ VIR_LOG_INIT("util.alloc"); int virAlloc(void *ptrptr, size_t size) { - *(void **)ptrptr = calloc(1, size); - if (*(void **)ptrptr == NULL) - abort(); - + *(void **)ptrptr = g_malloc0(size); return 0; } @@ -69,10 +66,7 @@ int virAllocN(void *ptrptr, size_t size, size_t count) { - *(void**)ptrptr = calloc(count, size); - if (*(void**)ptrptr == NULL) - abort(); - + *(void**)ptrptr = g_malloc0_n(count, size); return 0; } @@ -94,16 +88,7 @@ int virReallocN(void *ptrptr, size_t size, size_t count) { - void *tmp; - - if (xalloc_oversized(count, size)) - abort(); - - tmp = realloc(*(void**)ptrptr, size * count); - if (!tmp && ((size * count) != 0)) - abort(); - - *(void**)ptrptr = tmp; + *(void **)ptrptr = g_realloc_n(*(void**)ptrptr, size, count); return 0; } @@ -343,9 +328,7 @@ int virAllocVar(void *ptrptr, abort(); alloc_size = struct_size + (element_size * count); - *(void **)ptrptr = calloc(1, alloc_size); - if (*(void **)ptrptr == NULL) - abort(); + *(void **)ptrptr = g_malloc0(alloc_size); return 0; } @@ -362,7 +345,7 @@ void virFree(void *ptrptr) { int save_errno = errno; - free(*(void**)ptrptr); + g_free(*(void**)ptrptr); *(void**)ptrptr = NULL; errno = save_errno; } @@ -395,7 +378,7 @@ void virDispose(void *ptrptr, if (*(void**)ptrptr && count > 0) memset(*(void **)ptrptr, 0, count * element_size); - free(*(void**)ptrptr); + g_free(*(void**)ptrptr); *(void**)ptrptr = NULL; if (countptr) -- 2.21.0

Converting from virObject to GObject is reasonably straightforward, as illustrated by this patch for virIdentity In the header file - Remove typedef struct _virIdentity virIdentity - Add #define VIR_TYPE_IDENTITY virIdentity_get_type () G_DECLARE_FINAL_TYPE (virIdentity, virIdentity, VIR, IDENTITY, GObject); Which provides the typedef we just removed, and class declaration boilerplate and various other constants/macros. In the source file - Change 'virObject parent' to 'GObject parent' in the struct - Remove the virClass variable and its initializing call - Add G_DEFINE_TYPE(virIdentity, virIdentity, G_TYPE_OBJECT) which declares the instance & class constructor functions - Add an impl of the instance & class constructors wiring up the finalize method to point to our dispose impl In all files - Replace VIR_AUTOUNREF(virIdentityPtr) with g_autoptr(virIdentity) - Replace virObjectRef/Unref with g_object_ref/unref. Note the latter functions do *NOT* accept a NULL object where as libvirt's do. If you replace g_object_unref with g_clear_object it is NULL safe, but also clears the pointer. Signed-off-by: Daniel P. Berrangé <berrange@redhat.com> --- src/access/viraccessdriverpolkit.c | 21 +++---- src/admin/admin_server.c | 3 +- src/qemu/qemu_process.c | 4 +- src/remote/remote_daemon.c | 3 +- src/remote/remote_daemon_dispatch.c | 35 ++++-------- src/rpc/virnetserverclient.c | 57 ++++++++----------- src/rpc/virnetserverprogram.c | 13 +---- src/util/viridentity.c | 87 ++++++++++++++++------------- src/util/viridentity.h | 7 ++- tests/viridentitytest.c | 45 ++++++--------- tests/virnetserverclienttest.c | 3 +- 11 files changed, 122 insertions(+), 156 deletions(-) diff --git a/src/access/viraccessdriverpolkit.c b/src/access/viraccessdriverpolkit.c index e61ac6fa19..b39a28c834 100644 --- a/src/access/viraccessdriverpolkit.c +++ b/src/access/viraccessdriverpolkit.c @@ -78,8 +78,7 @@ virAccessDriverPolkitGetCaller(const char *actionid, unsigned long long *startTime, uid_t *uid) { - virIdentityPtr identity = virIdentityGetCurrent(); - int ret = -1; + g_autoptr(virIdentity) identity = virIdentityGetCurrent(); int rc; if (!identity) { @@ -90,37 +89,33 @@ virAccessDriverPolkitGetCaller(const char *actionid, } if ((rc = virIdentityGetProcessID(identity, pid)) < 0) - goto cleanup; + return -1; if (rc == 0) { virAccessError(VIR_ERR_INTERNAL_ERROR, "%s", _("No process ID available")); - goto cleanup; + return -1; } if ((rc = virIdentityGetProcessTime(identity, startTime)) < 0) - goto cleanup; + return -1; if (rc == 0) { virAccessError(VIR_ERR_INTERNAL_ERROR, "%s", _("No process start time available")); - goto cleanup; + return -1; } if ((rc = virIdentityGetUNIXUserID(identity, uid)) < 0) - goto cleanup; + return -1; if (rc == 0) { virAccessError(VIR_ERR_INTERNAL_ERROR, "%s", _("No UNIX caller UID available")); - goto cleanup; + return -1; } - ret = 0; - - cleanup: - virObjectUnref(identity); - return ret; + return 0; } diff --git a/src/admin/admin_server.c b/src/admin/admin_server.c index 248df3f795..7ae819b7b0 100644 --- a/src/admin/admin_server.c +++ b/src/admin/admin_server.c @@ -221,7 +221,7 @@ adminClientGetInfo(virNetServerClientPtr client, char *sock_addr = NULL; const char *attr = NULL; virTypedParameterPtr tmpparams = NULL; - virIdentityPtr identity = NULL; + g_autoptr(virIdentity) identity = NULL; int rc; virCheckFlags(0, -1); @@ -310,7 +310,6 @@ adminClientGetInfo(virNetServerClientPtr client, ret = 0; cleanup: - virObjectUnref(identity); VIR_FREE(sock_addr); return ret; } diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c index a50cd54393..872750ff4d 100644 --- a/src/qemu/qemu_process.c +++ b/src/qemu/qemu_process.c @@ -8051,7 +8051,7 @@ qemuProcessReconnect(void *opaque) bool tryMonReconn = false; virIdentitySetCurrent(data->identity); - virObjectUnref(data->identity); + g_clear_object(&data->identity); VIR_FREE(data); qemuDomainObjRestoreJob(obj, &oldjob); @@ -8364,7 +8364,7 @@ qemuProcessReconnectHelper(virDomainObjPtr obj, virDomainObjEndAPI(&obj); virNWFilterUnlockFilterUpdates(); - virObjectUnref(data->identity); + g_clear_object(&data->identity); VIR_FREE(data); return -1; } diff --git a/src/remote/remote_daemon.c b/src/remote/remote_daemon.c index 9281e226c4..7fcaa31c49 100644 --- a/src/remote/remote_daemon.c +++ b/src/remote/remote_daemon.c @@ -826,7 +826,7 @@ handleSystemMessageFunc(DBusConnection *connection ATTRIBUTE_UNUSED, static void daemonRunStateInit(void *opaque) { virNetDaemonPtr dmn = opaque; - virIdentityPtr sysident = virIdentityGetSystem(); + g_autoptr(virIdentity) sysident = virIdentityGetSystem(); #ifdef MODULE_NAME bool mandatory = true; #else /* ! MODULE_NAME */ @@ -879,7 +879,6 @@ static void daemonRunStateInit(void *opaque) cleanup: daemonInhibitCallback(false, dmn); virObjectUnref(dmn); - virObjectUnref(sysident); virIdentitySetCurrent(NULL); } diff --git a/src/remote/remote_daemon_dispatch.c b/src/remote/remote_daemon_dispatch.c index dbd2985c38..9fc5dc3998 100644 --- a/src/remote/remote_daemon_dispatch.c +++ b/src/remote/remote_daemon_dispatch.c @@ -159,7 +159,7 @@ remoteRelayDomainEventCheckACL(virNetServerClientPtr client, virConnectPtr conn, virDomainPtr dom) { virDomainDef def; - virIdentityPtr identity = NULL; + g_autoptr(virIdentity) identity = NULL; bool ret = false; /* For now, we just create a virDomainDef with enough contents to @@ -177,7 +177,6 @@ remoteRelayDomainEventCheckACL(virNetServerClientPtr client, cleanup: ignore_value(virIdentitySetCurrent(NULL)); - virObjectUnref(identity); return ret; } @@ -187,7 +186,7 @@ remoteRelayNetworkEventCheckACL(virNetServerClientPtr client, virConnectPtr conn, virNetworkPtr net) { virNetworkDef def; - virIdentityPtr identity = NULL; + g_autoptr(virIdentity) identity = NULL; bool ret = false; /* For now, we just create a virNetworkDef with enough contents to @@ -204,7 +203,6 @@ remoteRelayNetworkEventCheckACL(virNetServerClientPtr client, cleanup: ignore_value(virIdentitySetCurrent(NULL)); - virObjectUnref(identity); return ret; } @@ -214,7 +212,7 @@ remoteRelayStoragePoolEventCheckACL(virNetServerClientPtr client, virStoragePoolPtr pool) { virStoragePoolDef def; - virIdentityPtr identity = NULL; + g_autoptr(virIdentity) identity = NULL; bool ret = false; /* For now, we just create a virStoragePoolDef with enough contents to @@ -231,7 +229,6 @@ remoteRelayStoragePoolEventCheckACL(virNetServerClientPtr client, cleanup: ignore_value(virIdentitySetCurrent(NULL)); - virObjectUnref(identity); return ret; } @@ -241,7 +238,7 @@ remoteRelayNodeDeviceEventCheckACL(virNetServerClientPtr client, virNodeDevicePtr dev) { virNodeDeviceDef def; - virIdentityPtr identity = NULL; + g_autoptr(virIdentity) identity = NULL; bool ret = false; /* For now, we just create a virNodeDeviceDef with enough contents to @@ -257,7 +254,6 @@ remoteRelayNodeDeviceEventCheckACL(virNetServerClientPtr client, cleanup: ignore_value(virIdentitySetCurrent(NULL)); - virObjectUnref(identity); return ret; } @@ -267,7 +263,7 @@ remoteRelaySecretEventCheckACL(virNetServerClientPtr client, virSecretPtr secret) { virSecretDef def; - virIdentityPtr identity = NULL; + g_autoptr(virIdentity) identity = NULL; bool ret = false; /* For now, we just create a virSecretDef with enough contents to @@ -285,7 +281,6 @@ remoteRelaySecretEventCheckACL(virNetServerClientPtr client, cleanup: ignore_value(virIdentitySetCurrent(NULL)); - virObjectUnref(identity); return ret; } @@ -294,7 +289,7 @@ remoteRelayDomainQemuMonitorEventCheckACL(virNetServerClientPtr client, virConnectPtr conn, virDomainPtr dom) { virDomainDef def; - virIdentityPtr identity = NULL; + g_autoptr(virIdentity) identity = NULL; bool ret = false; /* For now, we just create a virDomainDef with enough contents to @@ -311,7 +306,6 @@ remoteRelayDomainQemuMonitorEventCheckACL(virNetServerClientPtr client, cleanup: ignore_value(virIdentitySetCurrent(NULL)); - virObjectUnref(identity); return ret; } @@ -1869,7 +1863,7 @@ void remoteRelayConnectionClosedEvent(virConnectPtr conn ATTRIBUTE_UNUSED, int r static void remoteClientFreePrivateCallbacks(struct daemonClientPrivate *priv) { - virIdentityPtr sysident = virIdentityGetSystem(); + g_autoptr(virIdentity) sysident = virIdentityGetSystem(); virIdentitySetCurrent(sysident); DEREG_CB(priv->conn, priv->domainEventCallbacks, @@ -1898,7 +1892,6 @@ remoteClientFreePrivateCallbacks(struct daemonClientPrivate *priv) } virIdentitySetCurrent(NULL); - virObjectUnref(sysident); } #undef DEREG_CB @@ -1965,7 +1958,7 @@ remoteOpenConn(const char *uri, } if (preserveIdentity) { - VIR_AUTOUNREF(virIdentityPtr) ident = NULL; + g_autoptr(virIdentity) ident = NULL; if (!(ident = virIdentityGetCurrent())) return -1; @@ -2436,7 +2429,7 @@ remoteDispatchConnectSetIdentity(virNetServerPtr server ATTRIBUTE_UNUSED, int nparams = 0; int rv = -1; virConnectPtr conn = remoteGetHypervisorConn(client); - VIR_AUTOUNREF(virIdentityPtr) ident = NULL; + g_autoptr(virIdentity) ident = NULL; if (!conn) goto cleanup; @@ -3982,7 +3975,7 @@ static int remoteSASLFinish(virNetServerPtr server, virNetServerClientPtr client) { - virIdentityPtr clnt_identity = NULL; + g_autoptr(virIdentity) clnt_identity = NULL; const char *identity; struct daemonClientPrivate *priv = virNetServerClientGetPrivateData(client); int ssf; @@ -3990,7 +3983,7 @@ remoteSASLFinish(virNetServerPtr server, /* TLS or UNIX domain sockets trivially OK */ if (!virNetServerClientIsSecure(client)) { if ((ssf = virNetSASLSessionGetKeySize(priv->sasl)) < 0) - goto error; + return -1; VIR_DEBUG("negotiated an SSF of %d", ssf); if (ssf < 56) { /* 56 is good for Kerberos */ @@ -4006,7 +3999,7 @@ remoteSASLFinish(virNetServerPtr server, return -2; if (!(clnt_identity = virNetServerClientGetIdentity(client))) - goto error; + return -1; virNetServerSetClientAuthenticated(server, client); virNetServerClientSetSASLSession(client, priv->sasl); @@ -4018,14 +4011,10 @@ remoteSASLFinish(virNetServerPtr server, "client=%p auth=%d identity=%s", client, REMOTE_AUTH_SASL, identity); - virObjectUnref(clnt_identity); virObjectUnref(priv->sasl); priv->sasl = NULL; return 0; - - error: - return -1; } /* diff --git a/src/rpc/virnetserverclient.c b/src/rpc/virnetserverclient.c index 171ee636dd..8482c5c29c 100644 --- a/src/rpc/virnetserverclient.c +++ b/src/rpc/virnetserverclient.c @@ -759,13 +759,13 @@ int virNetServerClientGetUNIXIdentity(virNetServerClientPtr client, static virIdentityPtr virNetServerClientCreateIdentity(virNetServerClientPtr client) { - char *username = NULL; - char *groupname = NULL; - char *seccontext = NULL; - virIdentityPtr ret = NULL; + g_autofree char *username = NULL; + g_autofree char *groupname = NULL; + g_autofree char *seccontext = NULL; + g_autoptr(virIdentity) ret = NULL; if (!(ret = virIdentityNew())) - goto error; + return NULL; if (client->sock && virNetSocketIsLocal(client->sock)) { gid_t gid; @@ -775,59 +775,50 @@ virNetServerClientCreateIdentity(virNetServerClientPtr client) if (virNetSocketGetUNIXIdentity(client->sock, &uid, &gid, &pid, ×tamp) < 0) - goto error; + return NULL; if (!(username = virGetUserName(uid))) - goto error; + return NULL; if (virIdentitySetUserName(ret, username) < 0) - goto error; + return NULL; if (virIdentitySetUNIXUserID(ret, uid) < 0) - goto error; + return NULL; if (!(groupname = virGetGroupName(gid))) - goto error; + return NULL; if (virIdentitySetGroupName(ret, groupname) < 0) - goto error; + return NULL; if (virIdentitySetUNIXGroupID(ret, gid) < 0) - goto error; + return NULL; if (virIdentitySetProcessID(ret, pid) < 0) - goto error; + return NULL; if (virIdentitySetProcessTime(ret, timestamp) < 0) - goto error; + return NULL; } #if WITH_SASL if (client->sasl) { const char *identity = virNetSASLSessionGetIdentity(client->sasl); if (virIdentitySetSASLUserName(ret, identity) < 0) - goto error; + return NULL; } #endif if (client->tls) { const char *identity = virNetTLSSessionGetX509DName(client->tls); if (virIdentitySetX509DName(ret, identity) < 0) - goto error; + return NULL; } if (client->sock && virNetSocketGetSELinuxContext(client->sock, &seccontext) < 0) - goto error; + return NULL; if (seccontext && virIdentitySetSELinuxContext(ret, seccontext) < 0) - goto error; - - cleanup: - VIR_FREE(username); - VIR_FREE(groupname); - VIR_FREE(seccontext); - return ret; + return NULL; - error: - virObjectUnref(ret); - ret = NULL; - goto cleanup; + return g_steal_pointer(&ret); } @@ -838,7 +829,7 @@ virIdentityPtr virNetServerClientGetIdentity(virNetServerClientPtr client) if (!client->identity) client->identity = virNetServerClientCreateIdentity(client); if (client->identity) - ret = virObjectRef(client->identity); + ret = g_object_ref(client->identity); virObjectUnlock(client); return ret; } @@ -848,10 +839,10 @@ void virNetServerClientSetIdentity(virNetServerClientPtr client, virIdentityPtr identity) { virObjectLock(client); - virObjectUnref(client->identity); + g_clear_object(&client->identity); client->identity = identity; if (client->identity) - virObjectRef(client->identity); + g_object_ref(client->identity); virObjectUnlock(client); } @@ -988,7 +979,7 @@ void virNetServerClientDispose(void *obj) if (client->privateData) client->privateDataFreeFunc(client->privateData); - virObjectUnref(client->identity); + g_clear_object(&client->identity); #if WITH_SASL virObjectUnref(client->sasl); @@ -1683,7 +1674,7 @@ virNetServerClientGetInfo(virNetServerClientPtr client, goto cleanup; } - *identity = virObjectRef(client->identity); + *identity = g_object_ref(client->identity); ret = 0; cleanup: diff --git a/src/rpc/virnetserverprogram.c b/src/rpc/virnetserverprogram.c index 7ae1d2e955..cca820ca5a 100644 --- a/src/rpc/virnetserverprogram.c +++ b/src/rpc/virnetserverprogram.c @@ -370,13 +370,13 @@ virNetServerProgramDispatchCall(virNetServerProgramPtr prog, virNetServerClientPtr client, virNetMessagePtr msg) { - char *arg = NULL; - char *ret = NULL; + g_autofree char *arg = NULL; + g_autofree char *ret = NULL; int rv = -1; virNetServerProgramProcPtr dispatcher; virNetMessageError rerr; size_t i; - virIdentityPtr identity = NULL; + g_autoptr(virIdentity) identity = NULL; memset(&rerr, 0, sizeof(rerr)); @@ -484,10 +484,7 @@ virNetServerProgramDispatchCall(virNetServerProgramPtr prog, } xdr_free(dispatcher->ret_filter, ret); - VIR_FREE(arg); - VIR_FREE(ret); - virObjectUnref(identity); /* Put reply on end of tx queue to send out */ return virNetServerClientSendMessage(client, msg); @@ -496,10 +493,6 @@ virNetServerProgramDispatchCall(virNetServerProgramPtr prog, * RPC error message we can send back to the client */ rv = virNetServerProgramSendReplyError(prog, client, msg, &rerr, &msg->header); - VIR_FREE(arg); - VIR_FREE(ret); - virObjectUnref(identity); - return rv; } diff --git a/src/util/viridentity.c b/src/util/viridentity.c index 22e2644c19..467d953e17 100644 --- a/src/util/viridentity.c +++ b/src/util/viridentity.c @@ -43,25 +43,29 @@ VIR_LOG_INIT("util.identity"); struct _virIdentity { - virObject parent; + GObject parent; int nparams; int maxparams; virTypedParameterPtr params; }; -static virClassPtr virIdentityClass; +G_DEFINE_TYPE(virIdentity, virIdentity, G_TYPE_OBJECT) + static virThreadLocal virIdentityCurrent; -static void virIdentityDispose(void *obj); +static void virIdentityDispose(GObject *obj); -static int virIdentityOnceInit(void) +static void virIdentityCurrentCleanup(void *ident) { - if (!VIR_CLASS_NEW(virIdentity, virClassForObject())) - return -1; + if (ident) + g_object_unref(ident); +} +static int virIdentityOnceInit(void) +{ if (virThreadLocalInit(&virIdentityCurrent, - (virThreadLocalCleanup)virObjectUnref) < 0) { + virIdentityCurrentCleanup) < 0) { virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("Cannot initialize thread local for current identity")); return -1; @@ -72,13 +76,24 @@ static int virIdentityOnceInit(void) VIR_ONCE_GLOBAL_INIT(virIdentity); +static void virIdentity_init(virIdentity *ident G_GNUC_UNUSED) +{ +} + +static void virIdentity_class_init(virIdentityClass *klass) +{ + GObjectClass *obj = G_OBJECT_CLASS(klass); + + obj->finalize = virIdentityDispose; +} + /** * virIdentityGetCurrent: * * Get the current identity associated with this thread. The * caller will own a reference to the returned identity, but * must not modify the object in any way, other than to - * release the reference when done with virObjectUnref + * release the reference when done with g_object_unref * * Returns: a reference to the current identity, or NULL */ @@ -90,7 +105,9 @@ virIdentityPtr virIdentityGetCurrent(void) return NULL; ident = virThreadLocalGet(&virIdentityCurrent); - return virObjectRef(ident); + if (ident) + g_object_ref(ident); + return ident; } @@ -105,7 +122,7 @@ virIdentityPtr virIdentityGetCurrent(void) */ int virIdentitySetCurrent(virIdentityPtr ident) { - virIdentityPtr old; + g_autoptr(virIdentity) old = NULL; if (virIdentityInitialize() < 0) return -1; @@ -113,15 +130,14 @@ int virIdentitySetCurrent(virIdentityPtr ident) old = virThreadLocalGet(&virIdentityCurrent); if (virThreadLocalSet(&virIdentityCurrent, - virObjectRef(ident)) < 0) { + ident ? g_object_ref(ident) : NULL) < 0) { virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("Unable to set thread local identity")); - virObjectUnref(ident); + if (ident) + g_object_unref(ident); return -1; } - virObjectUnref(old); - return 0; } @@ -139,36 +155,36 @@ virIdentityPtr virIdentityGetSystem(void) VIR_AUTOFREE(char *) username = NULL; VIR_AUTOFREE(char *) groupname = NULL; unsigned long long startTime; - virIdentityPtr ret = NULL; + g_autoptr(virIdentity) ret = NULL; #if WITH_SELINUX security_context_t con; #endif if (!(ret = virIdentityNew())) - goto error; + return NULL; if (virIdentitySetProcessID(ret, getpid()) < 0) - goto error; + return NULL; if (virProcessGetStartTime(getpid(), &startTime) < 0) - goto error; + return NULL; if (startTime != 0 && virIdentitySetProcessTime(ret, startTime) < 0) - goto error; + return NULL; if (!(username = virGetUserName(geteuid()))) return ret; if (virIdentitySetUserName(ret, username) < 0) - goto error; + return NULL; if (virIdentitySetUNIXUserID(ret, getuid()) < 0) - goto error; + return NULL; if (!(groupname = virGetGroupName(getegid()))) return ret; if (virIdentitySetGroupName(ret, groupname) < 0) - goto error; + return NULL; if (virIdentitySetUNIXGroupID(ret, getgid()) < 0) - goto error; + return NULL; #if WITH_SELINUX if (is_selinux_enabled() > 0) { @@ -179,17 +195,13 @@ virIdentityPtr virIdentityGetSystem(void) } if (virIdentitySetSELinuxContext(ret, con) < 0) { freecon(con); - goto error; + return NULL; } freecon(con); } #endif - return ret; - - error: - virObjectUnref(ret); - return NULL; + return g_steal_pointer(&ret); } @@ -203,26 +215,21 @@ virIdentityPtr virIdentityGetSystem(void) */ virIdentityPtr virIdentityNew(void) { - virIdentityPtr ident; - - if (virIdentityInitialize() < 0) - return NULL; - - if (!(ident = virObjectNew(virIdentityClass))) - return NULL; - - return ident; + return VIR_IDENTITY(g_object_new(VIR_TYPE_IDENTITY, NULL)); } -static void virIdentityDispose(void *object) +static void virIdentityDispose(GObject *object) { - virIdentityPtr ident = object; + virIdentityPtr ident = VIR_IDENTITY(object); virTypedParamsFree(ident->params, ident->nparams); + + G_OBJECT_CLASS(virIdentity_parent_class)->finalize(object); } + /* * Returns: 0 if not present, 1 if present, -1 on error */ diff --git a/src/util/viridentity.h b/src/util/viridentity.h index 861ecca736..b63a53e6c4 100644 --- a/src/util/viridentity.h +++ b/src/util/viridentity.h @@ -21,9 +21,12 @@ #pragma once -#include "virobject.h" +#include "internal.h" +#include <glib-object.h> + +#define VIR_TYPE_IDENTITY virIdentity_get_type () +G_DECLARE_FINAL_TYPE (virIdentity, virIdentity, VIR, IDENTITY, GObject); -typedef struct _virIdentity virIdentity; typedef virIdentity *virIdentityPtr; virIdentityPtr virIdentityGetCurrent(void); diff --git a/tests/viridentitytest.c b/tests/viridentitytest.c index 1eadd6173a..81db049fcb 100644 --- a/tests/viridentitytest.c +++ b/tests/viridentitytest.c @@ -38,58 +38,52 @@ VIR_LOG_INIT("tests.identitytest"); static int testIdentityAttrs(const void *data ATTRIBUTE_UNUSED) { - int ret = -1; - virIdentityPtr ident; + g_autoptr(virIdentity) ident = NULL; const char *val; int rc; - if (!(ident = virIdentityNew())) - goto cleanup; + ident = virIdentityNew(); if (virIdentitySetUserName(ident, "fred") < 0) - goto cleanup; + return -1; if ((rc = virIdentityGetUserName(ident, &val)) < 0) - goto cleanup; + return -1; if (STRNEQ_NULLABLE(val, "fred") || rc != 1) { VIR_DEBUG("Expected 'fred' got '%s'", NULLSTR(val)); - goto cleanup; + return -1; } if ((rc = virIdentityGetGroupName(ident, &val)) < 0) - goto cleanup; + return -1; if (val != NULL || rc != 0) { VIR_DEBUG("Unexpected groupname attribute"); - goto cleanup; + return -1; } if (virIdentitySetUserName(ident, "joe") >= 0) { VIR_DEBUG("Unexpectedly overwrote attribute"); - goto cleanup; + return -1; } if ((rc = virIdentityGetUserName(ident, &val)) < 0) - goto cleanup; + return -1; if (STRNEQ_NULLABLE(val, "fred") || rc != 1) { VIR_DEBUG("Expected 'fred' got '%s'", NULLSTR(val)); - goto cleanup; + return -1; } - ret = 0; - cleanup: - virObjectUnref(ident); - return ret; + return 0; } static int testIdentityGetSystem(const void *data) { const char *context = data; - int ret = -1; - virIdentityPtr ident = NULL; + g_autoptr(virIdentity) ident = NULL; const char *val; int rc; @@ -97,35 +91,32 @@ static int testIdentityGetSystem(const void *data) if (context) { VIR_DEBUG("libvirt not compiled with SELinux, skipping this test"); ret = EXIT_AM_SKIP; - goto cleanup; + return -1; } #endif if (!(ident = virIdentityGetSystem())) { VIR_DEBUG("Unable to get system identity"); - goto cleanup; + return -1; } if ((rc = virIdentityGetSELinuxContext(ident, &val)) < 0) - goto cleanup; + return -1; if (context == NULL) { if (val != NULL || rc != 0) { VIR_DEBUG("Unexpected SELinux context %s", NULLSTR(val)); - goto cleanup; + return -1; } } else { if (STRNEQ_NULLABLE(val, context) || rc != 1) { VIR_DEBUG("Want SELinux context '%s' got '%s'", context, val); - goto cleanup; + return -1; } } - ret = 0; - cleanup: - virObjectUnref(ident); - return ret; + return 0; } static int testSetFakeSELinuxContext(const void *data ATTRIBUTE_UNUSED) diff --git a/tests/virnetserverclienttest.c b/tests/virnetserverclienttest.c index d094de9840..42393d7dbe 100644 --- a/tests/virnetserverclienttest.c +++ b/tests/virnetserverclienttest.c @@ -51,7 +51,7 @@ static int testIdentity(const void *opaque ATTRIBUTE_UNUSED) int ret = -1; virNetSocketPtr sock = NULL; virNetServerClientPtr client = NULL; - virIdentityPtr ident = NULL; + g_autoptr(virIdentity) ident = NULL; const char *gotUsername = NULL; uid_t gotUserID; const char *gotGroupname = NULL; @@ -141,7 +141,6 @@ static int testIdentity(const void *opaque ATTRIBUTE_UNUSED) if (client) virNetServerClientClose(client); virObjectUnref(client); - virObjectUnref(ident); VIR_FORCE_CLOSE(sv[0]); VIR_FORCE_CLOSE(sv[1]); return ret; -- 2.21.0

Signed-off-by: Daniel P. Berrangé <berrange@redhat.com> --- src/conf/domain_event.c | 25 ++++++++----------- src/libxl/libxl_capabilities.c | 44 ++++++++++++++++------------------ 2 files changed, 31 insertions(+), 38 deletions(-) diff --git a/src/conf/domain_event.c b/src/conf/domain_event.c index b33589f472..f7d1a38e46 100644 --- a/src/conf/domain_event.c +++ b/src/conf/domain_event.c @@ -22,8 +22,6 @@ #include <config.h> -#include <regex.h> - #include "domain_event.h" #include "object_event.h" #include "object_event_private.h" @@ -2009,7 +2007,7 @@ virDomainQemuMonitorEventNew(int id, * deregisters. */ struct virDomainQemuMonitorEventData { char *event; - regex_t regex; + GRegex *regex; unsigned int flags; void *opaque; virFreeCallback freecb; @@ -2241,7 +2239,7 @@ virDomainQemuMonitorEventFilter(virConnectPtr conn ATTRIBUTE_UNUSED, if (data->flags == -1) return true; if (data->flags & VIR_CONNECT_DOMAIN_QEMU_MONITOR_EVENT_REGISTER_REGEX) - return regexec(&data->regex, monitorEvent->event, 0, NULL, 0) == 0; + return g_regex_match(data->regex, monitorEvent->event, 0, NULL) == TRUE; if (data->flags & VIR_CONNECT_DOMAIN_QEMU_MONITOR_EVENT_REGISTER_NOCASE) return STRCASEEQ(monitorEvent->event, data->event); return STREQ(monitorEvent->event, data->event); @@ -2255,7 +2253,7 @@ virDomainQemuMonitorEventCleanup(void *opaque) VIR_FREE(data->event); if (data->flags & VIR_CONNECT_DOMAIN_QEMU_MONITOR_EVENT_REGISTER_REGEX) - regfree(&data->regex); + g_regex_unref(data->regex); if (data->freecb) (data->freecb)(data->opaque); VIR_FREE(data); @@ -2306,20 +2304,17 @@ virDomainQemuMonitorEventStateRegisterID(virConnectPtr conn, return -1; data->flags = flags; if (event && flags != -1) { - int rflags = REG_NOSUB; - - if (flags & VIR_CONNECT_DOMAIN_QEMU_MONITOR_EVENT_REGISTER_NOCASE) - rflags |= REG_ICASE; if (flags & VIR_CONNECT_DOMAIN_QEMU_MONITOR_EVENT_REGISTER_REGEX) { - int err = regcomp(&data->regex, event, rflags); + int cflags = 0; + g_autoptr(GError) err = NULL; - if (err) { - char error[100]; - regerror(err, &data->regex, error, sizeof(error)); + if (flags & VIR_CONNECT_DOMAIN_QEMU_MONITOR_EVENT_REGISTER_NOCASE) + cflags |= G_REGEX_CASELESS; + data->regex = g_regex_new(event, cflags, 0, &err); + if (!data->regex) { virReportError(VIR_ERR_INVALID_ARG, _("failed to compile regex '%s': %s"), - event, error); - regfree(&data->regex); + event, err->message); VIR_FREE(data); return -1; } diff --git a/src/libxl/libxl_capabilities.c b/src/libxl/libxl_capabilities.c index 73ae0b3fa1..90d9bac9c7 100644 --- a/src/libxl/libxl_capabilities.c +++ b/src/libxl/libxl_capabilities.c @@ -21,7 +21,6 @@ #include <config.h> #include <libxl.h> -#include <regex.h> #include "internal.h" #include "virlog.h" @@ -374,10 +373,10 @@ static int libxlCapsInitGuests(libxl_ctx *ctx, virCapsPtr caps) { const libxl_version_info *ver_info; - int err; - regex_t regex; + g_autoptr(GRegex) regex = NULL; + g_autoptr(GError) err = NULL; + g_autoptr(GMatchInfo) info = NULL; char *str, *token; - regmatch_t subs[4]; char *saveptr = NULL; size_t i; @@ -398,12 +397,10 @@ libxlCapsInitGuests(libxl_ctx *ctx, virCapsPtr caps) return -1; } - err = regcomp(®ex, XEN_CAP_REGEX, REG_EXTENDED); - if (err != 0) { - char error[100]; - regerror(err, ®ex, error, sizeof(error)); + regex = g_regex_new(XEN_CAP_REGEX, G_REGEX_EXTENDED, 0, &err); + if (!regex) { virReportError(VIR_ERR_INTERNAL_ERROR, - _("Failed to compile regex %s"), error); + _("Failed to compile regex %s"), err->message); return -1; } @@ -436,31 +433,33 @@ libxlCapsInitGuests(libxl_ctx *ctx, virCapsPtr caps) nr_guest_archs < sizeof(guest_archs) / sizeof(guest_archs[0]) && (token = strtok_r(str, " ", &saveptr)) != NULL; str = NULL) { - if (regexec(®ex, token, sizeof(subs) / sizeof(subs[0]), - subs, 0) == 0) { - int hvm = STRPREFIX(&token[subs[1].rm_so], "hvm"); + if (g_regex_match(regex, token, 0, &info)) { + g_autofree char *modestr = g_match_info_fetch(info, 1); + g_autofree char *archstr = g_match_info_fetch(info, 2); + g_autofree char *suffixstr = g_match_info_fetch(info, 3); + int hvm = STRPREFIX(modestr, "hvm"); virArch arch; int pae = 0, nonpae = 0, ia64_be = 0; - if (STRPREFIX(&token[subs[2].rm_so], "x86_32")) { + if (STRPREFIX(archstr, "x86_32")) { arch = VIR_ARCH_I686; - if (subs[3].rm_so != -1 && - STRPREFIX(&token[subs[3].rm_so], "p")) + if (suffixstr != NULL && + STRPREFIX(suffixstr, "p")) pae = 1; else nonpae = 1; - } else if (STRPREFIX(&token[subs[2].rm_so], "x86_64")) { + } else if (STRPREFIX(archstr, "x86_64")) { arch = VIR_ARCH_X86_64; - } else if (STRPREFIX(&token[subs[2].rm_so], "ia64")) { + } else if (STRPREFIX(archstr, "ia64")) { arch = VIR_ARCH_ITANIUM; - if (subs[3].rm_so != -1 && - STRPREFIX(&token[subs[3].rm_so], "be")) + if (suffixstr != NULL && + STRPREFIX(suffixstr, "be")) ia64_be = 1; - } else if (STRPREFIX(&token[subs[2].rm_so], "powerpc64")) { + } else if (STRPREFIX(archstr, "powerpc64")) { arch = VIR_ARCH_PPC64; - } else if (STRPREFIX(&token[subs[2].rm_so], "armv7l")) { + } else if (STRPREFIX(archstr, "armv7l")) { arch = VIR_ARCH_ARMV7L; - } else if (STRPREFIX(&token[subs[2].rm_so], "aarch64")) { + } else if (STRPREFIX(archstr, "aarch64")) { arch = VIR_ARCH_AARCH64; } else { continue; @@ -515,7 +514,6 @@ libxlCapsInitGuests(libxl_ctx *ctx, virCapsPtr caps) #endif } } - regfree(®ex); for (i = 0; i < nr_guest_archs; ++i) { virCapsGuestPtr guest; -- 2.21.0

The GOptionContext API has the benefit over getopt_long that it will automatically handle --help output formatting. Signed-off-by: Daniel P. Berrangé <berrange@redhat.com> --- tools/virsh.c | 303 ++++++++++++++++++++++---------------------------- 1 file changed, 135 insertions(+), 168 deletions(-) diff --git a/tools/virsh.c b/tools/virsh.c index ec20f35a77..6c469ff576 100644 --- a/tools/virsh.c +++ b/tools/virsh.c @@ -23,7 +23,6 @@ #include <stdarg.h> #include <unistd.h> -#include <getopt.h> #include <sys/time.h> #include <fcntl.h> #include <time.h> @@ -445,53 +444,36 @@ virshDeinit(vshControl *ctl) } /* - * Print usage + * Build help description for commands */ -static void -virshUsage(void) +static char * +virshBuildDescription(void) { const vshCmdGrp *grp; const vshCmdDef *cmd; - - fprintf(stdout, _("\n%s [options]... [<command_string>]" - "\n%s [options]... <command> [args...]\n\n" - " options:\n" - " -c | --connect=URI hypervisor connection URI\n" - " -d | --debug=NUM debug level [0-4]\n" - " -e | --escape <char> set escape sequence for console\n" - " -h | --help this help\n" - " -k | --keepalive-interval=NUM\n" - " keepalive interval in seconds, 0 for disable\n" - " -K | --keepalive-count=NUM\n" - " number of possible missed keepalive messages\n" - " -l | --log=FILE output logging to file\n" - " -q | --quiet quiet mode\n" - " -r | --readonly connect readonly\n" - " -t | --timing print timing information\n" - " -v short version\n" - " -V long version\n" - " --version[=TYPE] version, TYPE is short or long (default short)\n" - " commands (non interactive mode):\n\n"), progname, - progname); + GString *str = g_string_new("Commands (non interactive mode):\n\n"); for (grp = cmdGroups; grp->name; grp++) { - fprintf(stdout, _(" %s (help keyword '%s')\n"), - grp->name, grp->keyword); + g_string_append_printf(str, + _(" %s (help keyword '%s')\n"), + grp->name, grp->keyword); for (cmd = grp->commands; cmd->name; cmd++) { if (cmd->flags & VSH_CMD_FLAG_ALIAS) continue; - fprintf(stdout, - " %-30s %s\n", cmd->name, - _(vshCmddefGetInfo(cmd, "help"))); + g_string_append_printf(str, + " %-30s %s\n", + cmd->name, + _(vshCmddefGetInfo(cmd, "help"))); } - fprintf(stdout, "\n"); + g_string_append_printf(str, "\n"); } - fprintf(stdout, "%s", - _("\n (specify help <group> for details about the commands in the group)\n")); - fprintf(stdout, "%s", - _("\n (specify help <command> for details about the command)\n\n")); - return; + g_string_append_printf(str, + _("Specify help <group> for details about the commands in the group)\n")); + g_string_append_printf(str, + _("Specify help <command> for details about the command)\n")); + + return g_string_free(str, FALSE); } /* @@ -647,6 +629,22 @@ virshAllowedEscapeChar(char c) ('@' <= c && c <= '_'); } +static gboolean +virshVersion(const gchar *option_name G_GNUC_UNUSED, + const gchar *value, + gpointer data, + GError **error G_GNUC_UNUSED) +{ + vshControl *ctl = data; + + if (value == NULL || STRNEQ(value, "long")) + puts(VERSION); + else + virshShowVersion(ctl); + + exit(EXIT_SUCCESS); +} + /* * argv[]: virsh [options] [command] * @@ -654,152 +652,121 @@ virshAllowedEscapeChar(char c) static bool virshParseArgv(vshControl *ctl, int argc, char **argv) { - int arg, len, debug, keepalive; - size_t i; - int longindex = -1; + int debug = 0; + bool version = false; + size_t len; virshControlPtr priv = ctl->privData; - struct option opt[] = { - {"connect", required_argument, NULL, 'c'}, - {"debug", required_argument, NULL, 'd'}, - {"escape", required_argument, NULL, 'e'}, - {"help", no_argument, NULL, 'h'}, - {"keepalive-interval", required_argument, NULL, 'k'}, - {"keepalive-count", required_argument, NULL, 'K'}, - {"log", required_argument, NULL, 'l'}, - {"quiet", no_argument, NULL, 'q'}, - {"readonly", no_argument, NULL, 'r'}, - {"timing", no_argument, NULL, 't'}, - {"version", optional_argument, NULL, 'v'}, - {NULL, 0, NULL, 0} + char *logfile = NULL; + int keepalive_interval = INT_MAX; + int keepalive_count = INT_MAX; + GOptionEntry opt[] = { + { "connect", 'c', 0, + G_OPTION_ARG_STRING, &ctl->connname, + _("hypervisor connection URI"), "URI" }, + { "debug", 'd', 0, + G_OPTION_ARG_INT, &debug, + _("debug level [0-4]\n"), "LEVEL" }, + { "escape", 'e', 0, + G_OPTION_ARG_STRING, &priv->escapeChar, + _("set escape sequence for console"), "ESCAPE" }, + { "keepalive-interval", 'k', 0, + G_OPTION_ARG_INT, &keepalive_interval, + _("keepalive interval in seconds, 0 for disable"), "SECS" }, + { "keepalive-count", 'K', 0, + G_OPTION_ARG_INT, &keepalive_count, + _("number of possible missed keepalive messages"), "NUM" }, + { "log", 'l', 0, + G_OPTION_ARG_STRING, &logfile, + _("output logging to file"), "FILENAME" }, + { "quiet", 'q', 0, + G_OPTION_ARG_NONE, &ctl->quiet, + _("quite mode"), NULL }, + { "readonly", 'r', 0, + G_OPTION_ARG_NONE, &priv->readonly, + _("connect readonly"), NULL }, + { "timing", 't', 0, + G_OPTION_ARG_NONE, &ctl->timing, + _("print timing information"), NULL }, + { "version", 'v', G_OPTION_FLAG_OPTIONAL_ARG, + G_OPTION_ARG_CALLBACK, virshVersion, + _("print short version"), "[short]" }, + { "version", 'V', 0, + G_OPTION_ARG_NONE, &version, + _("print long version"), "long" }, + { NULL, 0, 0, 0, NULL, NULL, NULL }, }; + g_autoptr(GOptionContext) optctx = NULL; + GOptionGroup *optgrp; + g_autoptr(GError) error = NULL; + + optctx = g_option_context_new(_("[COMMAND [OPTION…] - libvirt shell")); + optgrp = g_option_group_new(NULL, NULL, NULL, ctl, NULL); + g_option_group_set_translation_domain(optgrp, PACKAGE); + g_option_group_add_entries(optgrp, opt); + g_option_context_set_main_group(optctx, optgrp); + g_option_context_set_strict_posix(optctx, true); + g_option_context_set_description(optctx, + virshBuildDescription()); + + if (!g_option_context_parse(optctx, &argc, &argv, &error)) { + vshError(ctl, _("option parsing failed: %s\n"), error->message); + exit(EXIT_FAILURE); + } - /* Standard (non-command) options. The leading + ensures that no - * argument reordering takes place, so that command options are - * not confused with top-level virsh options. */ - while ((arg = getopt_long(argc, argv, "+:c:d:e:hk:K:l:qrtvV", opt, &longindex)) != -1) { - switch (arg) { - case 'c': - VIR_FREE(ctl->connname); - ctl->connname = vshStrdup(ctl, optarg); - break; - case 'd': - if (virStrToLong_i(optarg, NULL, 10, &debug) < 0) { - vshError(ctl, _("option %s takes a numeric argument"), - longindex == -1 ? "-d" : "--debug"); - exit(EXIT_FAILURE); - } - if (debug < VSH_ERR_DEBUG || debug > VSH_ERR_ERROR) - vshError(ctl, _("ignoring debug level %d out of range [%d-%d]"), - debug, VSH_ERR_DEBUG, VSH_ERR_ERROR); - else - ctl->debug = debug; - break; - case 'e': - len = strlen(optarg); - - if ((len == 2 && *optarg == '^' && - virshAllowedEscapeChar(optarg[1])) || - (len == 1 && *optarg != '^')) { - priv->escapeChar = optarg; - } else { - vshError(ctl, _("Invalid string '%s' for escape sequence"), - optarg); - exit(EXIT_FAILURE); - } - break; - case 'h': - virshUsage(); - exit(EXIT_SUCCESS); - break; - case 'k': - if (virStrToLong_i(optarg, NULL, 0, &keepalive) < 0) { - vshError(ctl, - _("Invalid value for option %s"), - longindex == -1 ? "-k" : "--keepalive-interval"); - exit(EXIT_FAILURE); - } - - if (keepalive < 0) { - vshError(ctl, - _("option %s requires a positive integer argument"), - longindex == -1 ? "-k" : "--keepalive-interval"); - exit(EXIT_FAILURE); - } - ctl->keepalive_interval = keepalive; - break; - case 'K': - if (virStrToLong_i(optarg, NULL, 0, &keepalive) < 0) { - vshError(ctl, - _("Invalid value for option %s"), - longindex == -1 ? "-K" : "--keepalive-count"); - exit(EXIT_FAILURE); - } + if (debug < VSH_ERR_DEBUG || debug > VSH_ERR_ERROR) { + vshError(ctl, _("debug level %d out of range [%d-%d]"), + debug, VSH_ERR_DEBUG, VSH_ERR_ERROR); + exit(EXIT_FAILURE); + } - if (keepalive < 0) { - vshError(ctl, - _("option %s requires a positive integer argument"), - longindex == -1 ? "-K" : "--keepalive-count"); - exit(EXIT_FAILURE); - } - ctl->keepalive_count = keepalive; - break; - case 'l': - vshCloseLogFile(ctl); - ctl->logfile = vshStrdup(ctl, optarg); - vshOpenLogFile(ctl); - break; - case 'q': - ctl->quiet = true; - break; - case 't': - ctl->timing = true; - break; - case 'r': - priv->readonly = true; - break; - case 'v': - if (STRNEQ_NULLABLE(optarg, "long")) { - puts(VERSION); - exit(EXIT_SUCCESS); - } - ATTRIBUTE_FALLTHROUGH; - case 'V': - virshShowVersion(ctl); - exit(EXIT_SUCCESS); - case ':': - for (i = 0; opt[i].name != NULL; i++) { - if (opt[i].val == optopt) - break; - } - if (opt[i].name) - vshError(ctl, _("option '-%c'/'--%s' requires an argument"), - optopt, opt[i].name); - else - vshError(ctl, _("option '-%c' requires an argument"), optopt); + if (keepalive_interval != INT_MAX) { + if (keepalive_interval < 0) { + vshError(ctl, + _("keepalive interval requires a positive integer")); exit(EXIT_FAILURE); - case '?': - if (optopt) - vshError(ctl, _("unsupported option '-%c'. See --help."), optopt); - else - vshError(ctl, _("unsupported option '%s'. See --help."), argv[optind - 1]); - exit(EXIT_FAILURE); - default: - vshError(ctl, _("unknown option")); + } + ctl->keepalive_interval = keepalive_interval; + } + + if (keepalive_count != INT_MAX) { + if (keepalive_count < 0) { + vshError(ctl, + _("keepalive count requires a positive integer")); exit(EXIT_FAILURE); } - longindex = -1; + ctl->keepalive_count = keepalive_count; + } + + len = strlen(priv->escapeChar); + if (!((len == 2 && *priv->escapeChar == '^' && + virshAllowedEscapeChar(priv->escapeChar[1])) || + (len == 1 && *priv->escapeChar != '^'))) { + vshError(ctl, _("Invalid string '%s' for escape sequence"), + priv->escapeChar); + exit(EXIT_FAILURE); + } + + if (version) { + virshShowVersion(ctl); + exit(EXIT_SUCCESS); + } + + if (logfile) { + vshCloseLogFile(ctl); + ctl->logfile = logfile; + vshOpenLogFile(ctl); } - if (argc == optind) { + if (argc == 1) { ctl->imode = true; } else { /* parse command */ ctl->imode = false; - if (argc - optind == 1) { - vshDebug(ctl, VSH_ERR_INFO, "commands: \"%s\"\n", argv[optind]); - return vshCommandStringParse(ctl, argv[optind], NULL); + if (argc == 2) { + vshDebug(ctl, VSH_ERR_INFO, "commands: \"%s\"\n", argv[1]); + return vshCommandStringParse(ctl, argv[1], NULL); } else { - return vshCommandArgvParse(ctl, argc - optind, argv + optind); + return vshCommandArgvParse(ctl, argc - 1, argv + 1); } } return true; -- 2.21.0

The GOptionContext API has the benefit over getopt_long that it will automatically handle --help output formatting. Signed-off-by: Daniel P. Berrangé <berrange@redhat.com> --- tools/virt-admin.c | 207 +++++++++++++++++++++------------------------ 1 file changed, 98 insertions(+), 109 deletions(-) diff --git a/tools/virt-admin.c b/tools/virt-admin.c index e549ec1f83..3b48dd549d 100644 --- a/tools/virt-admin.c +++ b/tools/virt-admin.c @@ -21,8 +21,6 @@ #include <config.h> #include "virt-admin.h" -#include <getopt.h> - #if WITH_READLINE # include <readline/readline.h> # include <readline/history.h> @@ -1212,44 +1210,34 @@ vshAdmDeinit(vshControl *ctl) /* * Print usage */ -static void -vshAdmUsage(void) +static char * +vshAdmBuildDescription(void) { const vshCmdGrp *grp; const vshCmdDef *cmd; - - fprintf(stdout, _("\n%s [options]... [<command_string>]" - "\n%s [options]... <command> [args...]\n\n" - " options:\n" - " -c | --connect=URI daemon admin connection URI\n" - " -d | --debug=NUM debug level [0-4]\n" - " -h | --help this help\n" - " -l | --log=FILE output logging to file\n" - " -q | --quiet quiet mode\n" - " -v short version\n" - " -V long version\n" - " --version[=TYPE] version, TYPE is short or long (default short)\n" - " commands (non interactive mode):\n\n"), progname, - progname); + GString *str = g_string_new("Commands (non interactive mode):\n\n"); for (grp = cmdGroups; grp->name; grp++) { - fprintf(stdout, _(" %s (help keyword '%s')\n"), - grp->name, grp->keyword); + g_string_append_printf(str, + _(" %s (help keyword '%s')\n"), + grp->name, grp->keyword); for (cmd = grp->commands; cmd->name; cmd++) { if (cmd->flags & VSH_CMD_FLAG_ALIAS) continue; - fprintf(stdout, - " %-30s %s\n", cmd->name, - _(vshCmddefGetInfo(cmd, "help"))); + g_string_append_printf(str, + " %-30s %s\n", + cmd->name, + _(vshCmddefGetInfo(cmd, "help"))); } - fprintf(stdout, "\n"); + g_string_append_printf(str, "\n"); } - fprintf(stdout, "%s", - _("\n (specify help <group> for details about the commands in the group)\n")); - fprintf(stdout, "%s", - _("\n (specify help <command> for details about the command)\n\n")); - return; + g_string_append_printf(str, + _("Specify help <group> for details about the commands in the group)\n")); + g_string_append_printf(str, + _("Specify help <command> for details about the command)\n")); + + return g_string_free(str, FALSE); } /* @@ -1275,103 +1263,104 @@ vshAdmShowVersion(vshControl *ctl ATTRIBUTE_UNUSED) vshPrint(ctl, "\n"); } +static gboolean +vshAdmVersion(const gchar *option_name G_GNUC_UNUSED, + const gchar *value, + gpointer data, + GError **error G_GNUC_UNUSED) +{ + vshControl *ctl = data; + + if (value == NULL || STRNEQ(value, "long")) + puts(VERSION); + else + vshAdmShowVersion(ctl); + + exit(EXIT_SUCCESS); +} + +/* + * argv[]: virsh [options] [command] + * + */ static bool vshAdmParseArgv(vshControl *ctl, int argc, char **argv) { - int arg, debug; - size_t i; - int longindex = -1; - struct option opt[] = { - {"connect", required_argument, NULL, 'c'}, - {"debug", required_argument, NULL, 'd'}, - {"help", no_argument, NULL, 'h'}, - {"log", required_argument, NULL, 'l'}, - {"quiet", no_argument, NULL, 'q'}, - {"version", optional_argument, NULL, 'v'}, - {NULL, 0, NULL, 0} + int debug = 0; + bool version = false; + char *logfile = NULL; + GOptionEntry opt[] = { + { "connect", 'c', 0, + G_OPTION_ARG_STRING, &ctl->connname, + _("hypervisor connection URI"), "URI" }, + { "debug", 'd', 0, + G_OPTION_ARG_INT, &debug, + _("debug level [0-4]\n"), "LEVEL" }, + { "log", 'l', 0, + G_OPTION_ARG_STRING, &logfile, + _("output logging to file"), "FILENAME" }, + { "quiet", 'q', 0, + G_OPTION_ARG_NONE, &ctl->quiet, + _("quite mode"), NULL }, + { "version", 'v', G_OPTION_FLAG_OPTIONAL_ARG, + G_OPTION_ARG_CALLBACK, vshAdmVersion, + _("print short version"), "[short]" }, + { "version", 'V', 0, + G_OPTION_ARG_NONE, &version, + _("print long version"), "long" }, + { NULL, 0, 0, 0, NULL, NULL, NULL }, }; + g_autoptr(GOptionContext) optctx = NULL; + GOptionGroup *optgrp; + g_autoptr(GError) error = NULL; + + optctx = g_option_context_new(_("[COMMAND [OPTION…] - libvirt admin shell")); + optgrp = g_option_group_new(NULL, NULL, NULL, ctl, NULL); + g_option_group_set_translation_domain(optgrp, PACKAGE); + g_option_group_add_entries(optgrp, opt); + g_option_context_set_main_group(optctx, optgrp); + g_option_context_set_strict_posix(optctx, true); + g_option_context_set_description(optctx, + vshAdmBuildDescription()); + + if (!g_option_context_parse(optctx, &argc, &argv, &error)) { + vshError(ctl, _("option parsing failed: %s\n"), error->message); + exit(EXIT_FAILURE); + } - /* Standard (non-command) options. The leading + ensures that no - * argument reordering takes place, so that command options are - * not confused with top-level virt-admin options. */ - while ((arg = getopt_long(argc, argv, "+:c:d:hl:qvV", opt, &longindex)) != -1) { - switch (arg) { - case 'c': - VIR_FREE(ctl->connname); - ctl->connname = vshStrdup(ctl, optarg); - break; - case 'd': - if (virStrToLong_i(optarg, NULL, 10, &debug) < 0) { - vshError(ctl, _("option %s takes a numeric argument"), - longindex == -1 ? "-d" : "--debug"); - exit(EXIT_FAILURE); - } - if (debug < VSH_ERR_DEBUG || debug > VSH_ERR_ERROR) - vshError(ctl, _("ignoring debug level %d out of range [%d-%d]"), - debug, VSH_ERR_DEBUG, VSH_ERR_ERROR); - else - ctl->debug = debug; - break; - case 'h': - vshAdmUsage(); - exit(EXIT_SUCCESS); - break; - case 'l': - vshCloseLogFile(ctl); - ctl->logfile = vshStrdup(ctl, optarg); - vshOpenLogFile(ctl); - break; - case 'q': - ctl->quiet = true; - break; - case 'v': - if (STRNEQ_NULLABLE(optarg, "long")) { - puts(VERSION); - exit(EXIT_SUCCESS); - } - ATTRIBUTE_FALLTHROUGH; - case 'V': - vshAdmShowVersion(ctl); - exit(EXIT_SUCCESS); - case ':': - for (i = 0; opt[i].name != NULL; i++) { - if (opt[i].val == optopt) - break; - } - if (opt[i].name) - vshError(ctl, _("option '-%c'/'--%s' requires an argument"), - optopt, opt[i].name); - else - vshError(ctl, _("option '-%c' requires an argument"), optopt); - exit(EXIT_FAILURE); - case '?': - if (optopt) - vshError(ctl, _("unsupported option '-%c'. See --help."), optopt); - else - vshError(ctl, _("unsupported option '%s'. See --help."), argv[optind - 1]); - exit(EXIT_FAILURE); - default: - vshError(ctl, _("unknown option")); - exit(EXIT_FAILURE); - } - longindex = -1; + if (debug < VSH_ERR_DEBUG || debug > VSH_ERR_ERROR) { + vshError(ctl, _("debug level %d out of range [%d-%d]"), + debug, VSH_ERR_DEBUG, VSH_ERR_ERROR); + exit(EXIT_FAILURE); + } + + if (version) { + vshAdmShowVersion(ctl); + exit(EXIT_SUCCESS); } - if (argc == optind) { + if (logfile) { + vshCloseLogFile(ctl); + ctl->logfile = logfile; + vshOpenLogFile(ctl); + } + + if (argc == 1) { ctl->imode = true; } else { /* parse command */ ctl->imode = false; - if (argc - optind == 1) { - vshDebug(ctl, VSH_ERR_INFO, "commands: \"%s\"\n", argv[optind]); - return vshCommandStringParse(ctl, argv[optind], NULL); + if (argc == 2) { + vshDebug(ctl, VSH_ERR_INFO, "commands: \"%s\"\n", argv[1]); + return vshCommandStringParse(ctl, argv[1], NULL); } else { - return vshCommandArgvParse(ctl, argc - optind, argv + optind); + return vshCommandArgvParse(ctl, argc - 1, argv + 1); } } return true; } + static const vshCmdDef vshAdmCmds[] = { VSH_CMD_CD, VSH_CMD_ECHO, -- 2.21.0