[libvirt] [PATCH v3 00/19] Integrate usage of glib into libvirt

This is a followup to a previous patch series: v0: https://www.redhat.com/archives/libvir-list/2019-August/msg01374.html v1: https://www.redhat.com/archives/libvir-list/2019-September/msg01331.html v2: https://www.redhat.com/archives/libvir-list/2019-October/msg00271.html The focus in this glib series is - Wire up pieces to facilitate interoperability around memory allocation/cleanup. - Deprecate & document existing libvirt APIs that are targetted for removal/conversion. - Illustrate some conversions and/or usage of new APIs. - Convert code that allows us to eliminate more gnulib modules There's obviously alot of conversion work that could be done, especially around memory allocation, auto cleanup and virObject stuff. I'm not intending to do that myself in the short term, as my immediate focus is on eliminating gnulib modules. Once this series is merged though, anyone is able to take on conversion jobs, so this allows the work to be spread out across the contributors. changed in v2: - First patch gets an addition to .travis.yml to add glib2 on macOS - First patch blacklists -Wbad-function-cast warning flag - Second patch updates many makefiles to refer to $(GLIB_LIBS) to fix build on platforms with stricter linkers (Ubuntu) - virIdentity conversion uses _ consistently in methods - GOptionContext conversions are dropped Daniel P. Berrangé (19): build: probe for glib-2 library in configure build: link to glib library util: use glib memory allocation functions util: use glib string allocation/formatting functions util: convert virSystemdActivation to use VIR_DEFINE_AUTOPTR_FUNC util: rewrite auto cleanup macros to use glib's equivalent src: add support for g_autoptr with virObject instances conf: convert virSecretObj APIs to use autofree util: use glib base64 encoding/decoding APIs util: convert virIdentity implementation and test suite to g_autoptr access: convert polkit driver to auto free memory admin: convert admin server code to use auto free macros rpc: convert methods using virIdentityPtr to auto free macros remote: convert methods using virIdentityPtr to auto free macros util: convert virIdentity class to use GObject libxl: convert over to use GRegex for regular expressions conf: convert over to use GRegex for regular expressions util: replace strerror/strerror_r with g_strerror build: remove use of usleep gnulib module in favour of g_usleep .travis.yml | 1 + bootstrap.conf | 8 -- build-aux/syntax-check.mk | 2 +- configure.ac | 7 +- docs/hacking.html.in | 144 ++++++++------------ libvirt.spec.in | 1 + m4/virt-compile-warnings.m4 | 23 ++++ m4/virt-glib.m4 | 36 +++++ mingw-libvirt.spec.in | 2 + src/Makefile.am | 3 + src/access/Makefile.inc.am | 4 +- src/access/viraccessdriverpolkit.c | 38 ++---- src/admin/admin_server.c | 204 ++++++++++++---------------- src/bhyve/Makefile.inc.am | 1 + src/conf/capabilities.h | 3 + src/conf/domain_capabilities.h | 3 + src/conf/domain_conf.h | 3 + src/conf/domain_event.c | 25 ++-- src/conf/snapshot_conf.h | 3 + src/conf/storage_capabilities.h | 3 + src/conf/virsecretobj.c | 68 +++------- src/datatypes.h | 15 ++ src/hyperv/hyperv_driver.c | 2 +- src/hyperv/hyperv_wmi.c | 4 +- src/interface/Makefile.inc.am | 1 + src/internal.h | 1 + src/libvirt_private.syms | 1 - src/libxl/Makefile.inc.am | 1 + src/libxl/libxl_capabilities.c | 42 +++--- src/libxl/libxl_conf.c | 3 +- src/libxl/libxl_conf.h | 2 + src/locking/Makefile.inc.am | 9 +- src/locking/lock_daemon.c | 2 +- src/locking/lock_driver_sanlock.c | 2 +- src/logging/Makefile.inc.am | 1 + src/lxc/Makefile.inc.am | 4 + src/lxc/lxc_controller.c | 2 +- src/lxc/lxc_driver.c | 2 +- src/lxc/lxc_process.c | 2 +- src/network/Makefile.inc.am | 2 + src/network/bridge_driver.c | 2 +- src/node_device/Makefile.inc.am | 5 +- src/nwfilter/Makefile.inc.am | 1 + src/nwfilter/nwfilter_dhcpsnoop.c | 4 +- src/nwfilter/nwfilter_learnipaddr.c | 2 +- src/qemu/Makefile.inc.am | 1 + src/qemu/qemu_agent.c | 6 +- src/qemu/qemu_blockjob.h | 1 + src/qemu/qemu_capabilities.h | 2 + src/qemu/qemu_command.c | 5 +- src/qemu/qemu_conf.h | 3 + src/qemu/qemu_domain.c | 8 +- src/qemu/qemu_monitor_json.c | 2 +- src/qemu/qemu_process.c | 6 +- src/qemu/qemu_tpm.c | 2 +- src/remote/Makefile.inc.am | 2 + src/remote/remote_daemon.c | 3 +- src/remote/remote_daemon_dispatch.c | 35 ++--- src/rpc/virnetserverclient.c | 57 ++++---- src/rpc/virnetserverprogram.c | 13 +- src/rpc/virnetsocket.c | 2 +- src/secret/Makefile.inc.am | 1 + src/secret/secret_driver.c | 1 - src/security/Makefile.inc.am | 1 + src/security/security_manager.c | 2 +- src/storage/Makefile.inc.am | 16 +++ src/storage/storage_backend_rbd.c | 4 +- src/storage/storage_util.c | 4 +- src/util/viralloc.c | 29 +--- src/util/viralloc.h | 14 +- src/util/virautoclean.h | 38 +++--- src/util/vircgroup.c | 2 +- src/util/virerror.c | 9 +- src/util/virerror.h | 1 + src/util/virfile.c | 2 +- src/util/virhostdev.h | 3 + src/util/viridentity.c | 92 +++++++------ src/util/viridentity.h | 7 +- src/util/virmdev.h | 3 + src/util/virnetdev.c | 2 +- src/util/virnetdevip.c | 2 +- src/util/virnetdevmacvlan.c | 2 +- src/util/virnetdevvportprofile.c | 2 +- src/util/virobject.h | 4 + src/util/virpci.c | 8 +- src/util/virpci.h | 3 + src/util/virprocess.c | 4 +- src/util/virresctrl.h | 4 + src/util/virscsi.h | 3 + src/util/virscsivhost.h | 3 + src/util/virstoragefile.h | 2 + src/util/virstring.c | 49 +++---- src/util/virstring.h | 10 +- src/util/virsystemd.c | 10 +- src/util/virsystemd.h | 5 +- src/util/virtime.c | 2 +- src/util/virusb.h | 3 + src/vbox/Makefile.inc.am | 1 + src/vbox/vbox_common.c | 2 +- src/vz/Makefile.inc.am | 1 + tests/Makefile.am | 7 +- tests/commandtest.c | 16 +-- tests/eventtest.c | 4 +- tests/fdstreamtest.c | 4 +- tests/qemumonitortestutils.c | 2 +- tests/seclabeltest.c | 4 +- tests/testutils.c | 6 +- tests/virhostcputest.c | 4 +- tests/viridentitytest.c | 45 +++--- tests/virnetserverclienttest.c | 3 +- tests/virsystemdtest.c | 2 +- tests/virtestmock.c | 4 +- tools/Makefile.am | 4 + tools/virsh-domain.c | 2 +- tools/virsh-secret.c | 17 +-- 115 files changed, 667 insertions(+), 650 deletions(-) create mode 100644 m4/virt-glib.m4 -- 2.21.0

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. We must disable the bad-function-cast warning as various GLib APIs and macros will trigger this. Reviewed-by: Ján Tomko <jtomko@redhat.com> Reviewed-by: Pavel Hrdina <phrdina@redhat.com> Signed-off-by: Daniel P. Berrangé <berrange@redhat.com> --- .travis.yml | 1 + configure.ac | 2 ++ libvirt.spec.in | 1 + m4/virt-compile-warnings.m4 | 2 ++ m4/virt-glib.m4 | 36 ++++++++++++++++++++++++++++++++++++ mingw-libvirt.spec.in | 2 ++ 6 files changed, 44 insertions(+) create mode 100644 m4/virt-glib.m4 diff --git a/.travis.yml b/.travis.yml index e475af34cf..478909d3bb 100644 --- a/.travis.yml +++ b/.travis.yml @@ -13,6 +13,7 @@ addons: - rpcgen - xz - yajl + - glib matrix: include: diff --git a/configure.ac b/configure.ac index f6bf4fb60a..9b4e6fdd6d 100644 --- a/configure.ac +++ b/configure.ac @@ -311,6 +311,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-compile-warnings.m4 b/m4/virt-compile-warnings.m4 index 4f9eee121c..1dbe1abe27 100644 --- a/m4/virt-compile-warnings.m4 +++ b/m4/virt-compile-warnings.m4 @@ -67,6 +67,8 @@ AC_DEFUN([LIBVIRT_COMPILE_WARNINGS],[ # > to handle the code effectively. # Source: https://gcc.gnu.org/onlinedocs/gcc/Warning-Options.html dontwarn="$dontwarn -Wdisabled-optimization" + # Various valid glib APIs/macros trigger this warning + dontwarn="$dontwarn -Wbad-function-cast" # Broken in 6.0 and later # https://gcc.gnu.org/bugzilla/show_bug.cgi?id=69602 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

On Thu, Oct 10, 2019 at 11:53:55AM +0100, Daniel P. Berrangé wrote:
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.
We must disable the bad-function-cast warning as various GLib APIs and macros will trigger this.
Reviewed-by: Ján Tomko <jtomko@redhat.com> Reviewed-by: Pavel Hrdina <phrdina@redhat.com> Signed-off-by: Daniel P. Berrangé <berrange@redhat.com> --- .travis.yml | 1 + configure.ac | 2 ++ libvirt.spec.in | 1 + m4/virt-compile-warnings.m4 | 2 ++ m4/virt-glib.m4 | 36 ++++++++++++++++++++++++++++++++++++ mingw-libvirt.spec.in | 2 ++ 6 files changed, 44 insertions(+) create mode 100644 m4/virt-glib.m4
Reviewed-by: Ján Tomko <jtomko@redhat.com> Jano

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, which provides the foundation layer with a collection of data structures, helper APIs, and platform portability logic. Later patches will introduce linkage to gobject which provides the object type system, built on glib, and gio which 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> --- docs/hacking.html.in | 21 +++++++++++++++++++++ src/Makefile.am | 3 +++ src/access/Makefile.inc.am | 4 +++- src/bhyve/Makefile.inc.am | 1 + src/interface/Makefile.inc.am | 1 + src/internal.h | 1 + src/libxl/Makefile.inc.am | 1 + src/locking/Makefile.inc.am | 9 ++++++++- src/logging/Makefile.inc.am | 1 + src/lxc/Makefile.inc.am | 4 ++++ src/network/Makefile.inc.am | 2 ++ src/node_device/Makefile.inc.am | 5 ++++- src/nwfilter/Makefile.inc.am | 1 + src/qemu/Makefile.inc.am | 1 + src/remote/Makefile.inc.am | 2 ++ src/secret/Makefile.inc.am | 1 + src/security/Makefile.inc.am | 1 + src/storage/Makefile.inc.am | 16 ++++++++++++++++ src/vbox/Makefile.inc.am | 1 + src/vz/Makefile.inc.am | 1 + tests/Makefile.am | 7 +++++-- tools/Makefile.am | 4 ++++ 22 files changed, 83 insertions(+), 5 deletions(-) diff --git a/docs/hacking.html.in b/docs/hacking.html.in index edf2f54ce3..2e064ced5e 100644 --- a/docs/hacking.html.in +++ b/docs/hacking.html.in @@ -989,6 +989,27 @@ BAD: it points to, or it is aliased to another pointer that is. </p> + <h2><a id="glib">Adoption of GLib APIs</a></h2> + + <p> + Libvirt has adopted use of the + <a href="https://developer.gnome.org/glib/stable/">GLib library</a>. + Due to libvirt's long history of development, there are many APIs + in libvirt, for which GLib provides an alternative solution. The + general rule to follow is that the standard GLib solution will be + preferred over historical libvirt APIs. Existing code will be + ported over to use GLib APIs over time, but new code should use + the GLib APIs straight away where possible. + </p> + + <p> + The following is a list of libvirt APIs that should no longer be + used in new code, and their suggested GLib replacements: + </p> + + <dl> + </dl> + <h2><a id="memalloc">Low level memory management</a></h2> <p> diff --git a/src/Makefile.am b/src/Makefile.am index bd03b09cb2..e646e954a4 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) \ @@ -558,6 +559,7 @@ libvirt_admin_la_LIBADD += \ $(YAJL_LIBS) \ $(DEVMAPPER_LIBS) \ $(LIBXML_LIBS) \ + $(GLIB_LIBS) \ $(SSH2_LIBS) \ $(SASL_LIBS) \ $(GNUTLS_LIBS) \ @@ -773,6 +775,7 @@ libvirt_iohelper_LDFLAGS = \ $(NULL) libvirt_iohelper_LDADD = \ libvirt.la \ + $(GLIB_LIBS) \ ../gnulib/lib/libgnu.la if WITH_DTRACE_PROBES libvirt_iohelper_LDADD += libvirt_probes.lo diff --git a/src/access/Makefile.inc.am b/src/access/Makefile.inc.am index 4dc742f4e5..ea27adbe0b 100644 --- a/src/access/Makefile.inc.am +++ b/src/access/Makefile.inc.am @@ -59,7 +59,9 @@ libvirt_driver_access_la_CFLAGS = \ $(AM_CFLAGS) \ $(NULL) libvirt_driver_access_la_LDFLAGS = $(AM_LDFLAGS) -libvirt_driver_access_la_LIBADD = +libvirt_driver_access_la_LIBADD = \ + $(GLIB_LIBS) \ + $(NULL) $(ACCESS_DRIVER_POLKIT_POLICY): $(srcdir)/access/viraccessperm.h \ diff --git a/src/bhyve/Makefile.inc.am b/src/bhyve/Makefile.inc.am index 195069872a..a881a83c56 100644 --- a/src/bhyve/Makefile.inc.am +++ b/src/bhyve/Makefile.inc.am @@ -34,6 +34,7 @@ libvirt_driver_bhyve_la_SOURCES = libvirt_driver_bhyve_la_LIBADD = \ libvirt_driver_bhyve_impl.la \ libvirt.la \ + $(GLIB_LIBS) \ ../gnulib/lib/libgnu.la \ $(NULL) mod_LTLIBRARIES += libvirt_driver_bhyve.la diff --git a/src/interface/Makefile.inc.am b/src/interface/Makefile.inc.am index baa85b4ba9..643e041232 100644 --- a/src/interface/Makefile.inc.am +++ b/src/interface/Makefile.inc.am @@ -28,6 +28,7 @@ libvirt_driver_interface_la_CFLAGS = \ libvirt_driver_interface_la_LDFLAGS = $(AM_LDFLAGS_MOD_NOUNDEF) libvirt_driver_interface_la_LIBADD = \ libvirt.la \ + $(GLIB_LIBS) \ $(NULL) libvirt_driver_interface_la_SOURCES = $(INTERFACE_DRIVER_SOURCES) if WITH_NETCF diff --git a/src/internal.h b/src/internal.h index e1a69be9f2..56e99241b0 100644 --- a/src/internal.h +++ b/src/internal.h @@ -28,6 +28,7 @@ #include <stdio.h> #include <string.h> #include <stdlib.h> +#include <glib.h> #if STATIC_ANALYSIS # undef NDEBUG /* Don't let a prior NDEBUG definition cause trouble. */ diff --git a/src/libxl/Makefile.inc.am b/src/libxl/Makefile.inc.am index c53396b7f3..49c6b4b12f 100644 --- a/src/libxl/Makefile.inc.am +++ b/src/libxl/Makefile.inc.am @@ -34,6 +34,7 @@ libvirt_driver_libxl_la_SOURCES = libvirt_driver_libxl_la_LIBADD = \ libvirt_driver_libxl_impl.la \ libvirt.la \ + $(GLIB_LIBS) \ ../gnulib/lib/libgnu.la \ $(NULL) mod_LTLIBRARIES += libvirt_driver_libxl.la diff --git a/src/locking/Makefile.inc.am b/src/locking/Makefile.inc.am index fae92a6e45..207aa9d7ef 100644 --- a/src/locking/Makefile.inc.am +++ b/src/locking/Makefile.inc.am @@ -103,6 +103,7 @@ lockd_la_CFLAGS = \ lockd_la_LDFLAGS = $(AM_LDFLAGS_MOD_NOUNDEF) lockd_la_LIBADD = \ libvirt.la \ + $(GLIB_LIBS) \ ../gnulib/lib/libgnu.la \ $(NULL) augeas_DATA += locking/libvirt_lockd.aug @@ -145,6 +146,7 @@ virtlockd_LDFLAGS = \ virtlockd_LDADD = \ libvirt.la \ libvirt_driver_admin.la \ + $(GLIB_LIBS) \ ../gnulib/lib/libgnu.la \ $(CYGWIN_EXTRA_LIBADD) \ $(NULL) @@ -163,7 +165,12 @@ lockdriver_LTLIBRARIES += sanlock.la sanlock_la_SOURCES = $(LOCK_DRIVER_SANLOCK_SOURCES) sanlock_la_CFLAGS = -I$(srcdir)/conf $(AM_CFLAGS) sanlock_la_LDFLAGS = $(AM_LDFLAGS_MOD_NOUNDEF) -sanlock_la_LIBADD = -lsanlock_client libvirt.la ../gnulib/lib/libgnu.la +sanlock_la_LIBADD = \ + -lsanlock_client \ + libvirt.la \ + $(GLIB_LIBS) \ + ../gnulib/lib/libgnu.la \ + $(NULL) augeas_DATA += locking/libvirt_sanlock.aug diff --git a/src/logging/Makefile.inc.am b/src/logging/Makefile.inc.am index 7e441dbffb..d7cb22f8bc 100644 --- a/src/logging/Makefile.inc.am +++ b/src/logging/Makefile.inc.am @@ -82,6 +82,7 @@ virtlogd_LDFLAGS = \ virtlogd_LDADD = \ libvirt_driver_admin.la \ libvirt.la \ + $(GLIB_LIBS) \ ../gnulib/lib/libgnu.la \ $(CYGWIN_EXTRA_LIBADD) \ $(NULL) diff --git a/src/lxc/Makefile.inc.am b/src/lxc/Makefile.inc.am index b4d560702c..0b8d4e5273 100644 --- a/src/lxc/Makefile.inc.am +++ b/src/lxc/Makefile.inc.am @@ -83,6 +83,7 @@ libvirt_driver_lxc_la_SOURCES = libvirt_driver_lxc_la_LIBADD = \ libvirt_driver_lxc_impl.la \ libvirt.la \ + $(GLIB_LIBS) \ ../gnulib/lib/libgnu.la \ $(NULL) mod_LTLIBRARIES += libvirt_driver_lxc.la @@ -184,10 +185,12 @@ libvirt_lxc_LDFLAGS = \ $(PIE_LDFLAGS) \ $(CAPNG_LIBS) \ $(LIBXML_LIBS) \ + $(GLIB_LIBS) \ $(NULL) libvirt_lxc_LDADD = \ libvirt.la \ $(FUSE_LIBS) \ + $(GLIB_LIBS) \ ../gnulib/lib/libgnu.la \ $(NULL) if WITH_DTRACE_PROBES @@ -200,6 +203,7 @@ libvirt_lxc_CFLAGS = \ $(PIE_CFLAGS) \ $(CAPNG_CFLAGS) \ $(LIBXML_CFLAGS) \ + $(GLIB_CFLAGS) \ $(LIBNL_CFLAGS) \ $(FUSE_CFLAGS) \ $(DBUS_CFLAGS) \ diff --git a/src/network/Makefile.inc.am b/src/network/Makefile.inc.am index 17467a65ad..6a1bea7aed 100644 --- a/src/network/Makefile.inc.am +++ b/src/network/Makefile.inc.am @@ -32,6 +32,7 @@ libvirt_driver_network_la_SOURCES = libvirt_driver_network_la_LIBADD = \ libvirt_driver_network_impl.la \ libvirt.la \ + $(GLIB_LIBS) \ ../gnulib/lib/libgnu.la \ $(LIBNL_LIBS) \ $(DBUS_LIBS) \ @@ -121,6 +122,7 @@ libvirt_leaseshelper_LDFLAGS = \ $(NULL) libvirt_leaseshelper_LDADD = \ libvirt.la \ + $(GLIB_LIBS) \ ../gnulib/lib/libgnu.la if WITH_DTRACE_PROBES libvirt_leaseshelper_LDADD += libvirt_probes.lo diff --git a/src/node_device/Makefile.inc.am b/src/node_device/Makefile.inc.am index eac7f92e88..9e07bae839 100644 --- a/src/node_device/Makefile.inc.am +++ b/src/node_device/Makefile.inc.am @@ -44,7 +44,10 @@ libvirt_driver_nodedev_la_CFLAGS = \ $(LIBNL_CFLAGS) \ $(NULL) libvirt_driver_nodedev_la_LDFLAGS = $(AM_LDFLAGS_MOD_NOUNDEF) -libvirt_driver_nodedev_la_LIBADD = libvirt.la +libvirt_driver_nodedev_la_LIBADD = \ + libvirt.la \ + $(GLIB_LIBS) \ + $(NULL) if WITH_HAL libvirt_driver_nodedev_la_SOURCES += $(NODE_DEVICE_DRIVER_HAL_SOURCES) diff --git a/src/nwfilter/Makefile.inc.am b/src/nwfilter/Makefile.inc.am index 6acb45705c..ceba8558fb 100644 --- a/src/nwfilter/Makefile.inc.am +++ b/src/nwfilter/Makefile.inc.am @@ -46,6 +46,7 @@ libvirt_driver_nwfilter_impl_la_LIBADD = \ $(LIBPCAP_LIBS) \ $(LIBNL_LIBS) \ $(DBUS_LIBS) \ + $(GLIB_LIBS) \ ../gnulib/lib/libgnu.la \ $(NULL) libvirt_driver_nwfilter_impl_la_SOURCES = $(NWFILTER_DRIVER_SOURCES) diff --git a/src/qemu/Makefile.inc.am b/src/qemu/Makefile.inc.am index e0e13fb1c3..e66da76c0a 100644 --- a/src/qemu/Makefile.inc.am +++ b/src/qemu/Makefile.inc.am @@ -83,6 +83,7 @@ libvirt_driver_qemu_la_SOURCES = libvirt_driver_qemu_la_LIBADD = \ libvirt_driver_qemu_impl.la \ libvirt.la \ + $(GLIB_LIBS) \ ../gnulib/lib/libgnu.la \ $(NULL) mod_LTLIBRARIES += libvirt_driver_qemu.la diff --git a/src/remote/Makefile.inc.am b/src/remote/Makefile.inc.am index 5a5c90a922..ae11e6592d 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) \ @@ -76,6 +77,7 @@ REMOTE_DAEMON_LD_ADD += ../src/libvirt_probes.lo endif WITH_DTRACE_PROBES REMOTE_DAEMON_LD_ADD += \ + $(GLIB_LIBS) \ ../gnulib/lib/libgnu.la \ $(NULL) diff --git a/src/secret/Makefile.inc.am b/src/secret/Makefile.inc.am index 76bc67418c..41a5e23da2 100644 --- a/src/secret/Makefile.inc.am +++ b/src/secret/Makefile.inc.am @@ -33,6 +33,7 @@ libvirt_driver_secret_la_CFLAGS = \ $(NULL) libvirt_driver_secret_la_LIBADD = \ libvirt.la \ + $(GLIB_LIBS) \ ../gnulib/lib/libgnu.la \ $(NULL) libvirt_driver_secret_la_LDFLAGS = $(AM_LDFLAGS_MOD_NOUNDEF) diff --git a/src/security/Makefile.inc.am b/src/security/Makefile.inc.am index 64e0f46857..6fe9d50f29 100644 --- a/src/security/Makefile.inc.am +++ b/src/security/Makefile.inc.am @@ -73,6 +73,7 @@ virt_aa_helper_LDFLAGS = \ virt_aa_helper_LDADD = \ libvirt.la \ libvirt_driver_storage_impl.la \ + $(GLIB_LIBS) \ ../gnulib/lib/libgnu.la \ $(NULL) if WITH_DTRACE_PROBES diff --git a/src/storage/Makefile.inc.am b/src/storage/Makefile.inc.am index 4dccb14ac1..6d5093b6a0 100644 --- a/src/storage/Makefile.inc.am +++ b/src/storage/Makefile.inc.am @@ -136,6 +136,7 @@ libvirt_driver_storage_la_SOURCES = libvirt_driver_storage_la_LIBADD = \ libvirt_driver_storage_impl.la \ libvirt.la \ + $(GLIB_LIBS) \ ../gnulib/lib/libgnu.la \ $(NULL) mod_LTLIBRARIES += libvirt_driver_storage.la @@ -217,6 +218,7 @@ storagebackend_LTLIBRARIES += libvirt_storage_backend_fs.la libvirt_storage_backend_fs_la_LDFLAGS = $(AM_LDFLAGS_MOD) libvirt_storage_backend_fs_la_LIBADD = \ libvirt.la \ + $(GLIB_LIBS) \ ../gnulib/lib/libgnu.la \ $(NULL) @@ -230,6 +232,7 @@ storagefile_LTLIBRARIES += libvirt_storage_file_fs.la libvirt_storage_file_fs_la_LDFLAGS = $(AM_LDFLAGS_MOD) libvirt_storage_file_fs_la_LIBADD = \ libvirt.la \ + $(GLIB_LIBS) \ ../gnulib/lib/libgnu.la \ $(NULL) endif WITH_STORAGE @@ -245,6 +248,7 @@ storagebackend_LTLIBRARIES += libvirt_storage_backend_logical.la libvirt_storage_backend_logical_la_LDFLAGS = $(AM_LDFLAGS_MOD) libvirt_storage_backend_logical_la_LIBADD = \ libvirt.la \ + $(GLIB_LIBS) \ ../gnulib/lib/libgnu.la \ $(NULL) endif WITH_STORAGE_LVM @@ -261,6 +265,7 @@ storagebackend_LTLIBRARIES += libvirt_storage_backend_iscsi.la libvirt_storage_backend_iscsi_la_LDFLAGS = $(AM_LDFLAGS_MOD) libvirt_storage_backend_iscsi_la_LIBADD = \ libvirt.la \ + $(GLIB_LIBS) \ ../gnulib/lib/libgnu.la \ $(NULL) endif WITH_STORAGE_ISCSI @@ -279,6 +284,7 @@ storagebackend_LTLIBRARIES += libvirt_storage_backend_iscsi-direct.la libvirt_storage_backend_iscsi_direct_la_LDFLAGS = $(AM_LDFLAGS_MOD) libvirt_storage_backend_iscsi_direct_la_LIBADD = \ libvirt.la \ + $(GLIB_LIBS) \ ../gnulib/lib/libgnu.la \ $(LIBISCSI_LIBS) \ $(NULL) @@ -295,6 +301,7 @@ storagebackend_LTLIBRARIES += libvirt_storage_backend_scsi.la libvirt_storage_backend_scsi_la_LDFLAGS = $(AM_LDFLAGS_MOD) libvirt_storage_backend_scsi_la_LIBADD = \ libvirt.la \ + $(GLIB_LIBS) \ ../gnulib/lib/libgnu.la \ $(NULL) endif WITH_STORAGE_SCSI @@ -304,6 +311,7 @@ libvirt_storage_backend_mpath_la_SOURCES = $(STORAGE_DRIVER_MPATH_SOURCES) libvirt_storage_backend_mpath_la_LIBADD = \ libvirt.la \ $(DEVMAPPER_LIBS) \ + $(GLIB_LIBS) \ ../gnulib/lib/libgnu.la \ $(NULL) libvirt_storage_backend_mpath_la_CFLAGS = \ @@ -327,6 +335,7 @@ storagebackend_LTLIBRARIES += libvirt_storage_backend_disk.la libvirt_storage_backend_disk_la_LDFLAGS = $(AM_LDFLAGS_MOD) libvirt_storage_backend_disk_la_LIBADD = \ libvirt.la \ + $(GLIB_LIBS) \ ../gnulib/lib/libgnu.la \ $(NULL) endif WITH_STORAGE_DISK @@ -336,6 +345,7 @@ libvirt_storage_backend_rbd_la_SOURCES = $(STORAGE_DRIVER_RBD_SOURCES) libvirt_storage_backend_rbd_la_LIBADD = \ libvirt.la \ $(LIBRBD_LIBS) \ + $(GLIB_LIBS) \ ../gnulib/lib/libgnu.la \ $(NULL) libvirt_storage_backend_rbd_la_CFLAGS = \ @@ -368,6 +378,7 @@ storagebackend_LTLIBRARIES += libvirt_storage_backend_sheepdog.la libvirt_storage_backend_sheepdog_la_LDFLAGS = $(AM_LDFLAGS_MOD) libvirt_storage_backend_sheepdog_la_LIBADD = \ libvirt.la \ + $(GLIB_LIBS) \ ../gnulib/lib/libgnu.la \ $(NULL) endif WITH_STORAGE_SHEEPDOG @@ -378,6 +389,7 @@ libvirt_storage_backend_gluster_la_SOURCES = \ libvirt_storage_backend_gluster_la_LIBADD = \ libvirt.la \ $(GLUSTERFS_LIBS) \ + $(GLIB_LIBS) \ ../gnulib/lib/libgnu.la \ $(NULL) libvirt_storage_backend_gluster_la_CFLAGS = \ @@ -395,6 +407,7 @@ libvirt_storage_file_gluster_la_SOURCES = \ libvirt_storage_file_gluster_la_LIBADD = \ libvirt.la \ $(GLUSTERFS_LIBS) \ + $(GLIB_LIBS) \ ../gnulib/lib/libgnu.la \ $(NULL) libvirt_storage_file_gluster_la_CFLAGS = \ @@ -419,6 +432,7 @@ storagebackend_LTLIBRARIES += libvirt_storage_backend_zfs.la libvirt_storage_backend_zfs_la_LDFLAGS = $(AM_LDFLAGS_MOD) libvirt_storage_backend_zfs_la_LIBADD = \ libvirt.la \ + $(GLIB_LIBS) \ ../gnulib/lib/libgnu.la \ $(NULL) endif WITH_STORAGE_ZFS @@ -435,6 +449,7 @@ storagebackend_LTLIBRARIES += libvirt_storage_backend_vstorage.la libvirt_storage_backend_vstorage_la_LDFLAGS = $(AM_LDFLAGS_MOD) libvirt_storage_backend_vstorage_la_LIBADD = \ libvirt.la \ + $(GLIB_LIBS) \ ../gnulib/lib/libgnu.la \ $(NULL) endif WITH_STORAGE_VSTORAGE @@ -450,6 +465,7 @@ libvirt_parthelper_LDFLAGS = \ libvirt_parthelper_LDADD = \ $(LIBPARTED_LIBS) \ libvirt.la \ + $(GLIB_LIBS) \ ../gnulib/lib/libgnu.la \ $(NULL) if WITH_DTRACE_PROBES diff --git a/src/vbox/Makefile.inc.am b/src/vbox/Makefile.inc.am index 178c360b99..35e370ac32 100644 --- a/src/vbox/Makefile.inc.am +++ b/src/vbox/Makefile.inc.am @@ -46,6 +46,7 @@ libvirt_driver_vbox_la_SOURCES = libvirt_driver_vbox_la_LIBADD = \ libvirt_driver_vbox_impl.la \ libvirt.la \ + $(GLIB_LIBS) \ ../gnulib/lib/libgnu.la \ $(NULL) mod_LTLIBRARIES += libvirt_driver_vbox.la diff --git a/src/vz/Makefile.inc.am b/src/vz/Makefile.inc.am index f56fceb8f7..63fe28be37 100644 --- a/src/vz/Makefile.inc.am +++ b/src/vz/Makefile.inc.am @@ -21,6 +21,7 @@ libvirt_driver_vz_la_SOURCES = libvirt_driver_vz_la_LIBADD = \ libvirt_driver_vz_impl.la \ libvirt.la \ + $(GLIB_LIBS) \ ../gnulib/lib/libgnu.la \ $(NULL) mod_LTLIBRARIES += libvirt_driver_vz.la diff --git a/tests/Makefile.am b/tests/Makefile.am index d88ad7f686..44fe51b83d 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) \ @@ -72,7 +73,9 @@ LDADDS = \ $(NO_INDIRECT_LDFLAGS) \ $(PROBES_O) \ $(GNULIB_LIBS) \ - ../src/libvirt.la + ../src/libvirt.la \ + $(GLIB_LIBS) \ + $(NULL) MOCKLIBS_LIBS = \ $(GNULIB_LIBS) \ @@ -525,7 +528,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 ece70384e6..68320c7246 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 = \ @@ -150,6 +151,7 @@ libvirt_shell_la_LIBADD = \ ../src/libvirt.la \ $(LIBXML_LIBS) \ $(READLINE_LIBS) \ + $(GLIB_LIBS) \ ../gnulib/lib/libgnu.la \ $(NULL) libvirt_shell_la_SOURCES = \ @@ -195,6 +197,7 @@ virt_host_validate_LDFLAGS = \ virt_host_validate_LDADD = \ ../src/libvirt.la \ + $(GLIB_LIBS) \ ../gnulib/lib/libgnu.la \ $(NULL) @@ -221,6 +224,7 @@ virt_login_shell_helper_LDFLAGS = \ virt_login_shell_helper_LDADD = \ ../src/libvirt.la \ ../src/libvirt-lxc.la \ + $(GLIB_LIBS) \ ../gnulib/lib/libgnu.la virt_login_shell_helper_CFLAGS = \ -- 2.21.0

On Thu, Oct 10, 2019 at 11:53:56AM +0100, Daniel P. Berrangé wrote:
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, which provides the foundation layer with a collection of data structures, helper APIs, and platform portability logic.
Later patches will introduce linkage to gobject which provides the object type system, built on glib, and gio which 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> --- docs/hacking.html.in | 21 +++++++++++++++++++++ src/Makefile.am | 3 +++ src/access/Makefile.inc.am | 4 +++- src/bhyve/Makefile.inc.am | 1 + src/interface/Makefile.inc.am | 1 + src/internal.h | 1 + src/libxl/Makefile.inc.am | 1 + src/locking/Makefile.inc.am | 9 ++++++++- src/logging/Makefile.inc.am | 1 + src/lxc/Makefile.inc.am | 4 ++++ src/network/Makefile.inc.am | 2 ++ src/node_device/Makefile.inc.am | 5 ++++- src/nwfilter/Makefile.inc.am | 1 + src/qemu/Makefile.inc.am | 1 + src/remote/Makefile.inc.am | 2 ++ src/secret/Makefile.inc.am | 1 + src/security/Makefile.inc.am | 1 + src/storage/Makefile.inc.am | 16 ++++++++++++++++ src/vbox/Makefile.inc.am | 1 + src/vz/Makefile.inc.am | 1 + tests/Makefile.am | 7 +++++-- tools/Makefile.am | 4 ++++ 22 files changed, 83 insertions(+), 5 deletions(-)
Reviewed-by: Ján Tomko <jtomko@redhat.com> Jano

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. Reviewed-by: Ján Tomko <jtomko@redhat.com> Signed-off-by: Daniel P. Berrangé <berrange@redhat.com> --- bootstrap.conf | 1 - docs/hacking.html.in | 106 +++++-------------------------------------- src/util/viralloc.c | 29 +++--------- src/util/viralloc.h | 9 ++++ 4 files changed, 27 insertions(+), 118 deletions(-) diff --git a/bootstrap.conf b/bootstrap.conf index 358d783a6b..7d73584809 100644 --- a/bootstrap.conf +++ b/bootstrap.conf @@ -26,7 +26,6 @@ byteswap c-ctype c-strcase c-strcasestr -calloc-posix canonicalize-lgpl chown clock-time diff --git a/docs/hacking.html.in b/docs/hacking.html.in index 2e064ced5e..8072796312 100644 --- a/docs/hacking.html.in +++ b/docs/hacking.html.in @@ -1008,102 +1008,20 @@ BAD: </p> <dl> + <dt>VIR_ALLOC, VIR_REALLOC, VIR_RESIZE_N, VIR_EXPAND_N, + VIR_SHRINK_N, VIR_FREE, VIR_APPEND_ELEMENT, VIR_INSERT_ELEMENT, + VIR_DELETE_ELEMENT</dt> + <dd>Prefer the GLib APIs g_new0/g_renew/g_free in most cases. + There should rarely be a need to use g_malloc/g_realloc. + Instead of using plain C arrays, it is preferrable to use + one of the GLib types, GArray, GPtrArray or GByteArray. These + all use a struct to track the array memory and size together + and efficiently resize. <strong>NEVER MIX</strong> use of the + classic libvirt memory allocation APIs and GLib APIs within + a single method. Keep the style consistent, converting existing + code to GLib style in a separate, prior commit.</dd> </dl> - <h2><a id="memalloc">Low level memory management</a></h2> - - <p> - Use of the malloc/free/realloc/calloc APIs is deprecated in the libvirt - codebase, because they encourage a number of serious coding bugs and do - not enable compile time verification of checks for NULL. Instead of these - routines, use the macros from viralloc.h. - </p> - - <ul> - <li><p>To allocate a single object:</p> - -<pre> - virDomainPtr domain; - - if (VIR_ALLOC(domain) < 0) - return NULL; -</pre> - </li> - - <li><p>To allocate an array of objects:</p> -<pre> - virDomainPtr domains; - size_t ndomains = 10; - - if (VIR_ALLOC_N(domains, ndomains) < 0) - return NULL; -</pre> - </li> - - <li><p>To allocate an array of object pointers:</p> -<pre> - virDomainPtr *domains; - size_t ndomains = 10; - - if (VIR_ALLOC_N(domains, ndomains) < 0) - return NULL; -</pre> - </li> - - <li><p>To re-allocate the array of domains to be 1 element - longer (however, note that repeatedly expanding an array by 1 - scales quadratically, so this is recommended only for smaller - arrays):</p> -<pre> - virDomainPtr domains; - size_t ndomains = 0; - - if (VIR_EXPAND_N(domains, ndomains, 1) < 0) - return NULL; - domains[ndomains - 1] = domain; -</pre></li> - - <li><p>To ensure an array has room to hold at least one more - element (this approach scales better, but requires tracking - allocation separately from usage)</p> - -<pre> - virDomainPtr domains; - size_t ndomains = 0; - size_t ndomains_max = 0; - - if (VIR_RESIZE_N(domains, ndomains_max, ndomains, 1) < 0) - return NULL; - domains[ndomains++] = domain; -</pre> - </li> - - <li><p>To trim an array of domains from its allocated size down - to the actual used size:</p> - -<pre> - virDomainPtr domains; - size_t ndomains = x; - size_t ndomains_max = y; - - VIR_SHRINK_N(domains, ndomains_max, ndomains_max - ndomains); -</pre></li> - - <li><p>To free an array of domains:</p> -<pre> - virDomainPtr domains; - size_t ndomains = x; - size_t ndomains_max = y; - size_t i; - - for (i = 0; i < ndomains; i++) - VIR_FREE(domains[i]); - VIR_FREE(domains); - ndomains_max = ndomains = 0; -</pre> - </li> - </ul> - <h2><a id="file_handling">File handling</a></h2> <p> 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) diff --git a/src/util/viralloc.h b/src/util/viralloc.h index 3e72e40bc9..517f9aada6 100644 --- a/src/util/viralloc.h +++ b/src/util/viralloc.h @@ -24,6 +24,15 @@ #include "internal.h" +/** + * DEPRECATION WARNING + * + * APIs in this file should only be used when modifying existing code. + * Consider converting existing code to use the new APIs when touching + * it. All new code must use the GLib memory allocation APIs and/or + * GLib array data types. See the hacking file for more guidance. + */ + /* Return 1 if an array of N objects, each of size S, cannot exist due to size arithmetic overflow. S must be positive and N must be nonnegative. This is a macro, not an inline function, so that it -- 2.21.0

On Thu, Oct 10, 2019 at 6:54 PM Daniel P. Berrangé <berrange@redhat.com> wrote:
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.
Reviewed-by: Ján Tomko <jtomko@redhat.com> Signed-off-by: Daniel P. Berrangé <berrange@redhat.com> --- bootstrap.conf | 1 - docs/hacking.html.in | 106 +++++-------------------------------------- src/util/viralloc.c | 29 +++--------- src/util/viralloc.h | 9 ++++ 4 files changed, 27 insertions(+), 118 deletions(-)
diff --git a/bootstrap.conf b/bootstrap.conf index 358d783a6b..7d73584809 100644 --- a/bootstrap.conf +++ b/bootstrap.conf @@ -26,7 +26,6 @@ byteswap c-ctype c-strcase c-strcasestr -calloc-posix canonicalize-lgpl chown clock-time diff --git a/docs/hacking.html.in b/docs/hacking.html.in index 2e064ced5e..8072796312 100644 --- a/docs/hacking.html.in +++ b/docs/hacking.html.in @@ -1008,102 +1008,20 @@ BAD: </p>
<dl> + <dt>VIR_ALLOC, VIR_REALLOC, VIR_RESIZE_N, VIR_EXPAND_N, + VIR_SHRINK_N, VIR_FREE, VIR_APPEND_ELEMENT, VIR_INSERT_ELEMENT, + VIR_DELETE_ELEMENT</dt> + <dd>Prefer the GLib APIs g_new0/g_renew/g_free in most cases. + There should rarely be a need to use g_malloc/g_realloc. + Instead of using plain C arrays, it is preferrable to use + one of the GLib types, GArray, GPtrArray or GByteArray. These + all use a struct to track the array memory and size together + and efficiently resize. <strong>NEVER MIX</strong> use of the + classic libvirt memory allocation APIs and GLib APIs within + a single method. Keep the style consistent, converting existing + code to GLib style in a separate, prior commit.</dd> </dl>
- <h2><a id="memalloc">Low level memory management</a></h2> - - <p> - Use of the malloc/free/realloc/calloc APIs is deprecated in the libvirt - codebase, because they encourage a number of serious coding bugs and do - not enable compile time verification of checks for NULL. Instead of these - routines, use the macros from viralloc.h. - </p> - - <ul> - <li><p>To allocate a single object:</p> - -<pre> - virDomainPtr domain; - - if (VIR_ALLOC(domain) < 0) - return NULL; -</pre> - </li> - - <li><p>To allocate an array of objects:</p> -<pre> - virDomainPtr domains; - size_t ndomains = 10; - - if (VIR_ALLOC_N(domains, ndomains) < 0) - return NULL; -</pre> - </li> - - <li><p>To allocate an array of object pointers:</p> -<pre> - virDomainPtr *domains; - size_t ndomains = 10; - - if (VIR_ALLOC_N(domains, ndomains) < 0) - return NULL; -</pre> - </li> - - <li><p>To re-allocate the array of domains to be 1 element - longer (however, note that repeatedly expanding an array by 1 - scales quadratically, so this is recommended only for smaller - arrays):</p> -<pre> - virDomainPtr domains; - size_t ndomains = 0; - - if (VIR_EXPAND_N(domains, ndomains, 1) < 0) - return NULL; - domains[ndomains - 1] = domain; -</pre></li> - - <li><p>To ensure an array has room to hold at least one more - element (this approach scales better, but requires tracking - allocation separately from usage)</p> - -<pre> - virDomainPtr domains; - size_t ndomains = 0; - size_t ndomains_max = 0; - - if (VIR_RESIZE_N(domains, ndomains_max, ndomains, 1) < 0) - return NULL; - domains[ndomains++] = domain; -</pre> - </li> - - <li><p>To trim an array of domains from its allocated size down - to the actual used size:</p> - -<pre> - virDomainPtr domains; - size_t ndomains = x; - size_t ndomains_max = y; - - VIR_SHRINK_N(domains, ndomains_max, ndomains_max - ndomains); -</pre></li> - - <li><p>To free an array of domains:</p> -<pre> - virDomainPtr domains; - size_t ndomains = x; - size_t ndomains_max = y; - size_t i; - - for (i = 0; i < ndomains; i++) - VIR_FREE(domains[i]); - VIR_FREE(domains); - ndomains_max = ndomains = 0; -</pre> - </li> - </ul> - <h2><a id="file_handling">File handling</a></h2>
<p> 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);
Hello, Daniel, a SIGABRT was found here: Version: libvirt v5.8.0-134-g9d03e9adf1 glib2-devel-2.56.4-7.el8.x86_64 Build: # CC=/usr/lib64/ccache/cc ./configure --without-libssh --build=x86_64-redhat-linux-gnu --host=x86_64-redhat-linux-gnu --program-prefix= --disable-dependency-tracking --prefix=/usr --exec-prefix=/usr --bindir=/usr/bin --sbindir=/usr/sbin --sysconfdir=/etc --datadir=/usr/share --includedir=/usr/include --libdir=/usr/lib64 --libexecdir=/usr/libexec --localstatedir=/var --sharedstatedir=/var/lib --mandir=/usr/share/man --infodir=/usr/share/info --with-qemu --without-openvz --without-lxc --without-vbox --without-libxl --with-sasl --with-polkit --with-libvirtd --without-phyp --with-esx --without-hyperv --without-vmware --without-xenapi --without-vz --without-bhyve --with-interface --with-network --with-storage-fs --with-storage-lvm --with-storage-iscsi --with-storage-iscsi-direct --with-storage-scsi --with-storage-disk --with-storage-mpath --with-storage-rbd --without-storage-sheepdog --with-storage-gluster --without-storage-zfs --without-storage-vstorage --with-numactl --with-numad --with-capng --without-fuse --with-netcf --with-selinux --with-selinux-mount=/sys/fs/selinux --without-apparmor --without-hal --with-udev --with-yajl --with-sanlock --with-libpcap --with-macvtap --with-audit --with-dtrace --with-driver-modules --with-firewalld --with-firewalld-zone --without-wireshark-dissector --without-pm-utils --with-nss-plugin '--with-packager=Unknown, 2019-08-19-12:13:01, lab.rhel8.me' --with-packager-version=1.el8 --with-qemu-user=qemu --with-qemu-group=qemu --with-tls-priority=@LIBVIRT,SYSTEM --enable-werror --enable-expensive-tests --with-init-script=systemd --without-login-shell && make -j8 Test steps: # LD_PRELOAD="$(find src -name '*.so.*'|tr '\n' ' ')" src/.libs/virtlogd free(): invalid pointer [1] 22488 abort (core dumped) LD_PRELOAD="$(find src -name '*.so.*'|tr '\n' ' ')" src/.libs/virtlogd (gdb) bt #0 0x00007fc3be4c68df in __GI_raise (sig=sig@entry=6) at ../sysdeps/unix/sysv/linux/raise.c:50 #1 0x00007fc3be4b0cf5 in __GI_abort () at abort.c:79 #2 0x00007fc3be509c17 in __libc_message (action=action@entry=do_abort, fmt=fmt@entry=0x7fc3be61670c "%s\n") at ../sysdeps/posix/libc_fatal.c:181 #3 0x00007fc3be51053c in malloc_printerr (str=str@entry=0x7fc3be6148bb "free(): invalid pointer") at malloc.c:5356 #4 0x00007fc3be511c64 in _int_free (av=<optimized out>, p=<optimized out>, have_lock=<optimized out>) at malloc.c:4185 #5 0x00007fc3bf3182e2 in g_free (mem=0x7ffe56ef7e58) at gmem.c:194 #6 0x00007fc3c261f63b in virFree (ptrptr=ptrptr@entry=0x7ffe56ef7d88) at util/viralloc.c:348 #7 0x00007fc3c26a1270 in virSystemdActivationFree (act=<optimized out>, act@entry=0x7ffe56ef7e58) at util/virsystemd.c:1056 #8 0x000055767eed7f4c in main (argc=<optimized out>, argv=0x7ffe56ef84c8) at logging/log_daemon.c:1119 See the full backtrace in attachment *(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) diff --git a/src/util/viralloc.h b/src/util/viralloc.h index 3e72e40bc9..517f9aada6 100644 --- a/src/util/viralloc.h +++ b/src/util/viralloc.h @@ -24,6 +24,15 @@
#include "internal.h"
+/** + * DEPRECATION WARNING + * + * APIs in this file should only be used when modifying existing code. + * Consider converting existing code to use the new APIs when touching + * it. All new code must use the GLib memory allocation APIs and/or + * GLib array data types. See the hacking file for more guidance. + */ + /* Return 1 if an array of N objects, each of size S, cannot exist due to size arithmetic overflow. S must be positive and N must be nonnegative. This is a macro, not an inline function, so that it -- 2.21.0
-- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
-- Best regards, ----------------------------------- Han Han Quality Engineer Redhat. Email: hhan@redhat.com Phone: +861065339333

Not reproduced when build source before `make clean`. Please ignore that issue On Tue, Oct 15, 2019 at 10:20 AM Han Han <hhan@redhat.com> wrote:
On Thu, Oct 10, 2019 at 6:54 PM Daniel P. Berrangé <berrange@redhat.com> wrote:
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.
Reviewed-by: Ján Tomko <jtomko@redhat.com> Signed-off-by: Daniel P. Berrangé <berrange@redhat.com> --- bootstrap.conf | 1 - docs/hacking.html.in | 106 +++++-------------------------------------- src/util/viralloc.c | 29 +++--------- src/util/viralloc.h | 9 ++++ 4 files changed, 27 insertions(+), 118 deletions(-)
diff --git a/bootstrap.conf b/bootstrap.conf index 358d783a6b..7d73584809 100644 --- a/bootstrap.conf +++ b/bootstrap.conf @@ -26,7 +26,6 @@ byteswap c-ctype c-strcase c-strcasestr -calloc-posix canonicalize-lgpl chown clock-time diff --git a/docs/hacking.html.in b/docs/hacking.html.in index 2e064ced5e..8072796312 100644 --- a/docs/hacking.html.in +++ b/docs/hacking.html.in @@ -1008,102 +1008,20 @@ BAD: </p>
<dl> + <dt>VIR_ALLOC, VIR_REALLOC, VIR_RESIZE_N, VIR_EXPAND_N, + VIR_SHRINK_N, VIR_FREE, VIR_APPEND_ELEMENT, VIR_INSERT_ELEMENT, + VIR_DELETE_ELEMENT</dt> + <dd>Prefer the GLib APIs g_new0/g_renew/g_free in most cases. + There should rarely be a need to use g_malloc/g_realloc. + Instead of using plain C arrays, it is preferrable to use + one of the GLib types, GArray, GPtrArray or GByteArray. These + all use a struct to track the array memory and size together + and efficiently resize. <strong>NEVER MIX</strong> use of the + classic libvirt memory allocation APIs and GLib APIs within + a single method. Keep the style consistent, converting existing + code to GLib style in a separate, prior commit.</dd> </dl>
- <h2><a id="memalloc">Low level memory management</a></h2> - - <p> - Use of the malloc/free/realloc/calloc APIs is deprecated in the libvirt - codebase, because they encourage a number of serious coding bugs and do - not enable compile time verification of checks for NULL. Instead of these - routines, use the macros from viralloc.h. - </p> - - <ul> - <li><p>To allocate a single object:</p> - -<pre> - virDomainPtr domain; - - if (VIR_ALLOC(domain) < 0) - return NULL; -</pre> - </li> - - <li><p>To allocate an array of objects:</p> -<pre> - virDomainPtr domains; - size_t ndomains = 10; - - if (VIR_ALLOC_N(domains, ndomains) < 0) - return NULL; -</pre> - </li> - - <li><p>To allocate an array of object pointers:</p> -<pre> - virDomainPtr *domains; - size_t ndomains = 10; - - if (VIR_ALLOC_N(domains, ndomains) < 0) - return NULL; -</pre> - </li> - - <li><p>To re-allocate the array of domains to be 1 element - longer (however, note that repeatedly expanding an array by 1 - scales quadratically, so this is recommended only for smaller - arrays):</p> -<pre> - virDomainPtr domains; - size_t ndomains = 0; - - if (VIR_EXPAND_N(domains, ndomains, 1) < 0) - return NULL; - domains[ndomains - 1] = domain; -</pre></li> - - <li><p>To ensure an array has room to hold at least one more - element (this approach scales better, but requires tracking - allocation separately from usage)</p> - -<pre> - virDomainPtr domains; - size_t ndomains = 0; - size_t ndomains_max = 0; - - if (VIR_RESIZE_N(domains, ndomains_max, ndomains, 1) < 0) - return NULL; - domains[ndomains++] = domain; -</pre> - </li> - - <li><p>To trim an array of domains from its allocated size down - to the actual used size:</p> - -<pre> - virDomainPtr domains; - size_t ndomains = x; - size_t ndomains_max = y; - - VIR_SHRINK_N(domains, ndomains_max, ndomains_max - ndomains); -</pre></li> - - <li><p>To free an array of domains:</p> -<pre> - virDomainPtr domains; - size_t ndomains = x; - size_t ndomains_max = y; - size_t i; - - for (i = 0; i < ndomains; i++) - VIR_FREE(domains[i]); - VIR_FREE(domains); - ndomains_max = ndomains = 0; -</pre> - </li> - </ul> - <h2><a id="file_handling">File handling</a></h2>
<p> 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);
Hello, Daniel, a SIGABRT was found here: Version: libvirt v5.8.0-134-g9d03e9adf1 glib2-devel-2.56.4-7.el8.x86_64
Build: # CC=/usr/lib64/ccache/cc ./configure --without-libssh --build=x86_64-redhat-linux-gnu --host=x86_64-redhat-linux-gnu --program-prefix= --disable-dependency-tracking --prefix=/usr --exec-prefix=/usr --bindir=/usr/bin --sbindir=/usr/sbin --sysconfdir=/etc --datadir=/usr/share --includedir=/usr/include --libdir=/usr/lib64 --libexecdir=/usr/libexec --localstatedir=/var --sharedstatedir=/var/lib --mandir=/usr/share/man --infodir=/usr/share/info --with-qemu --without-openvz --without-lxc --without-vbox --without-libxl --with-sasl --with-polkit --with-libvirtd --without-phyp --with-esx --without-hyperv --without-vmware --without-xenapi --without-vz --without-bhyve --with-interface --with-network --with-storage-fs --with-storage-lvm --with-storage-iscsi --with-storage-iscsi-direct --with-storage-scsi --with-storage-disk --with-storage-mpath --with-storage-rbd --without-storage-sheepdog --with-storage-gluster --without-storage-zfs --without-storage-vstorage --with-numactl --with-numad --with-capng --without-fuse --with-netcf --with-selinux --with-selinux-mount=/sys/fs/selinux --without-apparmor --without-hal --with-udev --with-yajl --with-sanlock --with-libpcap --with-macvtap --with-audit --with-dtrace --with-driver-modules --with-firewalld --with-firewalld-zone --without-wireshark-dissector --without-pm-utils --with-nss-plugin '--with-packager=Unknown, 2019-08-19-12:13:01, lab.rhel8.me' --with-packager-version=1.el8 --with-qemu-user=qemu --with-qemu-group=qemu --with-tls-priority=@LIBVIRT,SYSTEM --enable-werror --enable-expensive-tests --with-init-script=systemd --without-login-shell && make -j8
Test steps: # LD_PRELOAD="$(find src -name '*.so.*'|tr '\n' ' ')" src/.libs/virtlogd free(): invalid pointer [1] 22488 abort (core dumped) LD_PRELOAD="$(find src -name '*.so.*'|tr '\n' ' ')" src/.libs/virtlogd (gdb) bt #0 0x00007fc3be4c68df in __GI_raise (sig=sig@entry=6) at ../sysdeps/unix/sysv/linux/raise.c:50 #1 0x00007fc3be4b0cf5 in __GI_abort () at abort.c:79 #2 0x00007fc3be509c17 in __libc_message (action=action@entry=do_abort, fmt=fmt@entry=0x7fc3be61670c "%s\n") at ../sysdeps/posix/libc_fatal.c:181 #3 0x00007fc3be51053c in malloc_printerr (str=str@entry=0x7fc3be6148bb "free(): invalid pointer") at malloc.c:5356 #4 0x00007fc3be511c64 in _int_free (av=<optimized out>, p=<optimized out>, have_lock=<optimized out>) at malloc.c:4185 #5 0x00007fc3bf3182e2 in g_free (mem=0x7ffe56ef7e58) at gmem.c:194 #6 0x00007fc3c261f63b in virFree (ptrptr=ptrptr@entry=0x7ffe56ef7d88) at util/viralloc.c:348 #7 0x00007fc3c26a1270 in virSystemdActivationFree (act=<optimized out>, act@entry=0x7ffe56ef7e58) at util/virsystemd.c:1056 #8 0x000055767eed7f4c in main (argc=<optimized out>, argv=0x7ffe56ef84c8) at logging/log_daemon.c:1119
See the full backtrace in attachment
*(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) diff --git a/src/util/viralloc.h b/src/util/viralloc.h index 3e72e40bc9..517f9aada6 100644 --- a/src/util/viralloc.h +++ b/src/util/viralloc.h @@ -24,6 +24,15 @@
#include "internal.h"
+/** + * DEPRECATION WARNING + * + * APIs in this file should only be used when modifying existing code. + * Consider converting existing code to use the new APIs when touching + * it. All new code must use the GLib memory allocation APIs and/or + * GLib array data types. See the hacking file for more guidance. + */ + /* Return 1 if an array of N objects, each of size S, cannot exist due to size arithmetic overflow. S must be positive and N must be nonnegative. This is a macro, not an inline function, so that it -- 2.21.0
-- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
-- Best regards, ----------------------------------- Han Han Quality Engineer Redhat.
Email: hhan@redhat.com Phone: +861065339333
-- Best regards, ----------------------------------- Han Han Quality Engineer Redhat. Email: hhan@redhat.com Phone: +861065339333

Convert the string duplication APIs to use the g_strdup family of APIs. We previously used the 'strdup-posix' gnulib module because mingw does not set errno to ENOMEM on failure We previously used the 'strndup' gnulib module because this function does not exist on mingw. We previously used the 'vasprintf' gnulib module because of many GNU supported format specifiers not working on non-Linux platforms. glib's own equivalent standardizes on GNU format specifiers too. Reviewed-by: Ján Tomko <jtomko@redhat.com> Signed-off-by: Daniel P. Berrangé <berrange@redhat.com> --- bootstrap.conf | 3 --- docs/hacking.html.in | 8 ++++++++ src/util/virstring.c | 28 ++++++++++++++++++++++------ src/util/virstring.h | 8 ++++++++ 4 files changed, 38 insertions(+), 9 deletions(-) diff --git a/bootstrap.conf b/bootstrap.conf index 7d73584809..7e264b63ad 100644 --- a/bootstrap.conf +++ b/bootstrap.conf @@ -80,8 +80,6 @@ snprintf socket stat-time strchrnul -strdup-posix -strndup strerror strerror_r-posix strptime @@ -96,7 +94,6 @@ ttyname_r uname unsetenv usleep -vasprintf verify vsnprintf waitpid diff --git a/docs/hacking.html.in b/docs/hacking.html.in index 8072796312..3f1542b6de 100644 --- a/docs/hacking.html.in +++ b/docs/hacking.html.in @@ -1020,6 +1020,14 @@ BAD: classic libvirt memory allocation APIs and GLib APIs within a single method. Keep the style consistent, converting existing code to GLib style in a separate, prior commit.</dd> + + <dt>VIR_STRDUP, VIR_STRNDUP</dt> + <dd>Prefer the GLib APIs g_strdup and g_strndup.</dd> + + <dt>virAsprintf, virVasprintf</dt> + <dd>The GLib APIs g_strdup_printf / g_strdup_vprint should be used + instead. Don't use g_vasprintf unless having the string length + returned is unavoidable.</dd> </dl> <h2><a id="file_handling">File handling</a></h2> diff --git a/src/util/virstring.c b/src/util/virstring.c index a4cc7e9c0a..6b2b6ed24e 100644 --- a/src/util/virstring.c +++ b/src/util/virstring.c @@ -18,6 +18,7 @@ #include <config.h> +#include <glib/gprintf.h> #include <regex.h> #include <locale.h> @@ -730,10 +731,21 @@ virVasprintfInternal(char **strp, const char *fmt, va_list list) { + char *str = NULL; int ret; - if ((ret = vasprintf(strp, fmt, list)) == -1) + ret = g_vasprintf(&str, fmt, list); + + /* GLib is supposed to abort() on OOM, but a mistake meant + * it did not. Delete this once our min glib is at 2.64.0 + * which includes the fix: + * https://gitlab.gnome.org/GNOME/glib/merge_requests/1145 + */ +#if !GLIB_CHECK_VERSION(2, 64, 0) + if (!str) abort(); +#endif + *strp = str; return ret; } @@ -743,11 +755,17 @@ virAsprintfInternal(char **strp, const char *fmt, ...) { va_list ap; + char *str = NULL; int ret; va_start(ap, fmt); - ret = virVasprintfInternal(strp, fmt, ap); + ret = g_vasprintf(&str, fmt, ap); va_end(ap); + + if (!*str) + abort(); + *strp = str; + return ret; } @@ -936,8 +954,7 @@ virStrdup(char **dest, *dest = NULL; if (!src) return 0; - if (!(*dest = strdup(src))) - abort(); + *dest = g_strdup(src); return 1; } @@ -965,8 +982,7 @@ virStrndup(char **dest, return 0; if (n < 0) n = strlen(src); - if (!(*dest = strndup(src, n))) - abort(); + *dest = g_strndup(src, n); return 1; } diff --git a/src/util/virstring.h b/src/util/virstring.h index f537f3472e..3ffe51f7b8 100644 --- a/src/util/virstring.h +++ b/src/util/virstring.h @@ -145,6 +145,8 @@ int virVasprintfInternal(char **strp, const char *fmt, va_list list) * @dst: variable to hold result (char*, not char**) * @src: string to duplicate * + * DEPRECATED: use g_strdup instead + * * Duplicate @src string and store it into @dst. * * This macro is safe to use on arguments with side effects. @@ -158,6 +160,8 @@ int virVasprintfInternal(char **strp, const char *fmt, va_list list) * @dst: variable to hold result (char*, not char**) * @src: string to duplicate * + * DEPRECATED: use g_strdup instead + * * Duplicate @src string and store it into @dst. * * This macro is safe to use on arguments with side effects. @@ -172,6 +176,8 @@ int virVasprintfInternal(char **strp, const char *fmt, va_list list) * @src: string to duplicate * @n: the maximum number of bytes to copy * + * DEPRECATED: use g_strndup instead + * * Duplicate @src string and store it into @dst. If @src is longer than @n, * only @n bytes are copied and terminating null byte '\0' is added. If @n * is a negative number, then the whole @src string is copied. That is, @@ -189,6 +195,8 @@ int virVasprintfInternal(char **strp, const char *fmt, va_list list) * @src: string to duplicate * @n: the maximum number of bytes to copy * + * DEPRECATED: use g_strndup instead + * * Duplicate @src string and store it into @dst. If @src is longer than @n, * only @n bytes are copied and terminating null byte '\0' is added. If @n * is a negative number, then the whole @src string is copied. That is, -- 2.21.0

Using the standard macro will facilitate the conversion to glib's auto cleanup macros. Reviewed-by: Ján Tomko <jtomko@redhat.com> Signed-off-by: Daniel P. Berrangé <berrange@redhat.com> --- src/util/virsystemd.c | 10 +++++----- src/util/virsystemd.h | 5 +++-- tests/virsystemdtest.c | 2 +- 3 files changed, 9 insertions(+), 8 deletions(-) diff --git a/src/util/virsystemd.c b/src/util/virsystemd.c index 2efc0dd72c..c2e4c3df51 100644 --- a/src/util/virsystemd.c +++ b/src/util/virsystemd.c @@ -917,7 +917,7 @@ virSystemdActivationNew(virSystemdActivationMap *map, return act; error: - virSystemdActivationFree(&act); + virSystemdActivationFree(act); return NULL; } @@ -1046,12 +1046,12 @@ virSystemdActivationClaimFDs(virSystemdActivationPtr act, * associated with the activation object */ void -virSystemdActivationFree(virSystemdActivationPtr *act) +virSystemdActivationFree(virSystemdActivationPtr act) { - if (!*act) + if (!act) return; - virHashFree((*act)->fds); + virHashFree(act->fds); - VIR_FREE(*act); + VIR_FREE(act); } diff --git a/src/util/virsystemd.h b/src/util/virsystemd.h index 5f1a4413fe..2c0a0d8dc0 100644 --- a/src/util/virsystemd.h +++ b/src/util/virsystemd.h @@ -22,6 +22,7 @@ #pragma once #include "internal.h" +#include "virautoclean.h" typedef struct _virSystemdActivation virSystemdActivation; typedef virSystemdActivation *virSystemdActivationPtr; @@ -81,6 +82,6 @@ void virSystemdActivationClaimFDs(virSystemdActivationPtr act, int **fds, size_t *nfds); -void virSystemdActivationFree(virSystemdActivationPtr *act); +void virSystemdActivationFree(virSystemdActivationPtr act); -#define virSystemdActivationAutoPtrFree virSystemdActivationFree +VIR_DEFINE_AUTOPTR_FUNC(virSystemdActivation, virSystemdActivationFree); diff --git a/tests/virsystemdtest.c b/tests/virsystemdtest.c index 3add1ab56f..d33a7c192f 100644 --- a/tests/virsystemdtest.c +++ b/tests/virsystemdtest.c @@ -650,7 +650,7 @@ testActivationEmpty(const void *opaque ATTRIBUTE_UNUSED) if (act != NULL) { fprintf(stderr, "Unexpectedly got activation object"); - virSystemdActivationFree(&act); + virSystemdActivationFree(act); return -1; } -- 2.21.0

To facilitate porting over to glib, this rewrites the auto cleanup macros to use glib's equivalent. As a result it is now possible to use g_autoptr/VIR_AUTOPTR, and g_auto/VIR_AUTOCLEAN, g_autofree/VIR_AUTOFREE interchangably, regardless of which macros were used to declare the cleanup types. Within the scope of any single method, code must remain consistent using either GLib or Libvirt macros, never mixing both. New code must preferentially use the GLib macros, and old code will be converted incrementally. Reviewed-by: Ján Tomko <jtomko@redhat.com> Signed-off-by: Daniel P. Berrangé <berrange@redhat.com> --- build-aux/syntax-check.mk | 2 +- docs/hacking.html.in | 18 ++++++++++++++++++ m4/virt-compile-warnings.m4 | 21 ++++++++++++++++++++ src/util/viralloc.h | 5 ++++- src/util/virautoclean.h | 38 +++++++++++++++++++------------------ 5 files changed, 64 insertions(+), 20 deletions(-) diff --git a/build-aux/syntax-check.mk b/build-aux/syntax-check.mk index 8345703b3e..f4712c24c3 100644 --- a/build-aux/syntax-check.mk +++ b/build-aux/syntax-check.mk @@ -1128,7 +1128,7 @@ sc_prohibit_backslash_alignment: # Rule to ensure that variables declared using a cleanup macro are # always initialized. sc_require_attribute_cleanup_initialization: - @prohibit='VIR_AUTO((FREE|PTR|UNREF|CLEAN)\(.+\)|CLOSE|STRINGLIST) *[^=]+;' \ + @prohibit='((g_auto(ptr|free)?)|(VIR_AUTO((FREE|PTR|UNREF|CLEAN)\(.+\)|CLOSE|STRINGLIST))) *[^=]+;' \ in_vc_files='\.[chx]$$' \ halt='variable declared with a cleanup macro must be initialized' \ $(_sc_search_regexp) diff --git a/docs/hacking.html.in b/docs/hacking.html.in index 3f1542b6de..6e62b2d4ff 100644 --- a/docs/hacking.html.in +++ b/docs/hacking.html.in @@ -1028,6 +1028,24 @@ BAD: <dd>The GLib APIs g_strdup_printf / g_strdup_vprint should be used instead. Don't use g_vasprintf unless having the string length returned is unavoidable.</dd> + + <dt>VIR_AUTOPTR, VIR_AUTOCLEAN, VIR_AUTOFREE</dt> + <dd>The GLib macros g_autoptr, g_auto and g_autofree must be used + instead in all new code. In existing code, the GLib macros must + never be mixed with libvirt macros within a method, nor should + they be mixed with VIR_FREE. If introducing GLib macros to an + existing method, any use of libvirt macros must be converted + in an independent commit. + </dd> + + <dt>VIR_DEFINE_AUTOPTR_FUNC, VIR_DEFINE_AUTOCLEAN_FUNC</dt> + <dd>The Gib macros G_DEFINE_AUTOPTR_CLEANUP_FUNC and + G_DEFINE_AUTO_CLEANUP_CLEAR_FUNC must be used in all + new code. Existing code should be converted to the + new macros where relevant. It is permissible to use + g_autoptr, g_auto on an object whose cleanup function + is declared with the libvirt macros and vice-verca. + </dd> </dl> <h2><a id="file_handling">File handling</a></h2> diff --git a/m4/virt-compile-warnings.m4 b/m4/virt-compile-warnings.m4 index 1dbe1abe27..7c86fdd3c6 100644 --- a/m4/virt-compile-warnings.m4 +++ b/m4/virt-compile-warnings.m4 @@ -104,6 +104,20 @@ AC_DEFUN([LIBVIRT_COMPILE_WARNINGS],[ dontwarn="$dontwarn -Wdouble-promotion" fi + # Clang complains about unused static inline functions + # which are common with G_DEFINE_AUTOPTR_CLEANUP_FUNC + AC_CACHE_CHECK([whether clang gives bogus warnings for -Wunused-function], + [lv_cv_clang_unused_function_broken], [ + save_CFLAGS="$CFLAGS" + CFLAGS="-Wunused-function -Werror" + AC_COMPILE_IFELSE([AC_LANG_PROGRAM([[ + static inline void foo(void) {} + ]], [[ + return 0]])], + [lv_cv_clang_unused_function_broken=no], + [lv_cv_clang_unused_function_broken=yes]) + CFLAGS="$save_CFLAGS"]) + # We might fundamentally need some of these disabled forever, but # ideally we'd turn many of them on dontwarn="$dontwarn -Wfloat-equal" @@ -119,6 +133,13 @@ AC_DEFUN([LIBVIRT_COMPILE_WARNINGS],[ # Remove the ones we don't want, blacklisted earlier gl_MANYWARN_COMPLEMENT([wantwarn], [$maybewarn], [$dontwarn]) + # -Wunused-functin is implied by -Wall we must turn it + # off explicitly. + if test "$lv_cv_clang_unused_function_broken" = "yes"; + then + wantwarn="$wantwarn -Wno-unused-function" + fi + # GNULIB uses '-W' (aka -Wextra) which includes a bunch of stuff. # Unfortunately, this means you can't simply use '-Wsign-compare' # with gl_MANYWARN_COMPLEMENT diff --git a/src/util/viralloc.h b/src/util/viralloc.h index 517f9aada6..49bf2b86e7 100644 --- a/src/util/viralloc.h +++ b/src/util/viralloc.h @@ -494,8 +494,11 @@ void virDisposeString(char **strptr) * VIR_AUTOFREE: * @type: type of the variable to be freed automatically * + * DEPRECATED: use g_autofree for new code. See HACKING + * for further guidance. + * * Macro to automatically free the memory allocated to * the variable declared with it by calling virFree * when the variable goes out of scope. */ -#define VIR_AUTOFREE(type) __attribute__((cleanup(virFree))) type +#define VIR_AUTOFREE(type) g_autofree type diff --git a/src/util/virautoclean.h b/src/util/virautoclean.h index 6da288e67d..71312a2782 100644 --- a/src/util/virautoclean.h +++ b/src/util/virautoclean.h @@ -20,7 +20,21 @@ #pragma once -#define VIR_AUTOPTR_FUNC_NAME(type) type##AutoPtrFree +/** + * DEPRECATION WARNING + * + * The macros in this file should not be used in newly written code. + * Use the equivalent GLib macros instead. + * + * For existing code, use of the libvirt and GLib macros must NEVER + * be mixed within a single method. + * + * The use of the libvirt VIR_FREE macros should also not be mixed + * with GLib auto-free macros and vice-verca. + * + * Existing code should be converted to the new GLib macros and + * g_free APIs as needed. + */ /** * VIR_DEFINE_AUTOPTR_FUNC: @@ -31,15 +45,8 @@ * resources allocated to a variable of type @type. This newly * defined function works as a necessary wrapper around @func. */ -#define VIR_DEFINE_AUTOPTR_FUNC(type, func) \ - static inline void VIR_AUTOPTR_FUNC_NAME(type)(type **_ptr) \ - { \ - if (*_ptr) \ - (func)(*_ptr); \ - *_ptr = NULL; \ - } - -#define VIR_AUTOCLEAN_FUNC_NAME(type) type##AutoClean +#define VIR_DEFINE_AUTOPTR_FUNC(t, f) \ + G_DEFINE_AUTOPTR_CLEANUP_FUNC(t, f) /** * VIR_DEFINE_AUTOCLEAN_FUNC: @@ -51,10 +58,7 @@ * take pointer to @type. */ #define VIR_DEFINE_AUTOCLEAN_FUNC(type, func) \ - static inline void VIR_AUTOCLEAN_FUNC_NAME(type)(type *_ptr) \ - { \ - (func)(_ptr); \ - } + G_DEFINE_AUTO_CLEANUP_CLEAR_FUNC(type, func) /** * VIR_AUTOPTR: @@ -68,8 +72,7 @@ * Note that this macro must NOT be used with vectors! The freeing function * will not free any elements beyond the first. */ -#define VIR_AUTOPTR(type) \ - __attribute__((cleanup(VIR_AUTOPTR_FUNC_NAME(type)))) type * +#define VIR_AUTOPTR(type) g_autoptr(type) /** * VIR_AUTOCLEAN: @@ -83,5 +86,4 @@ * Note that this macro must NOT be used with vectors! The cleaning function * will not clean any elements beyond the first. */ -#define VIR_AUTOCLEAN(type) \ - __attribute__((cleanup(VIR_AUTOCLEAN_FUNC_NAME(type)))) type +#define VIR_AUTOCLEAN(type) g_auto(type) -- 2.21.0

Daniel P. Berrangé <berrange@redhat.com> [2019-10-10, 11:54AM +0100]:
diff --git a/docs/hacking.html.in b/docs/hacking.html.in index 3f1542b6de..6e62b2d4ff 100644 --- a/docs/hacking.html.in +++ b/docs/hacking.html.in @@ -1028,6 +1028,24 @@ BAD: <dd>The GLib APIs g_strdup_printf / g_strdup_vprint should be used instead. Don't use g_vasprintf unless having the string length returned is unavoidable.</dd> + + <dt>VIR_AUTOPTR, VIR_AUTOCLEAN, VIR_AUTOFREE</dt> + <dd>The GLib macros g_autoptr, g_auto and g_autofree must be used + instead in all new code. In existing code, the GLib macros must + never be mixed with libvirt macros within a method, nor should + they be mixed with VIR_FREE. If introducing GLib macros to an + existing method, any use of libvirt macros must be converted + in an independent commit. + </dd> + + <dt>VIR_DEFINE_AUTOPTR_FUNC, VIR_DEFINE_AUTOCLEAN_FUNC</dt> + <dd>The Gib macros G_DEFINE_AUTOPTR_CLEANUP_FUNC and ^ GLib
-- IBM Systems Linux on Z & Virtualization Development -------------------------------------------------- IBM Deutschland Research & Development GmbH Schönaicher Str. 220, 71032 Böblingen Phone: +49 7031 16 1819 -------------------------------------------------- Vorsitzende des Aufsichtsrats: Matthias Hartmann Geschäftsführung: Dirk Wittkopp Sitz der Gesellschaft: Böblingen Registergericht: Amtsgericht Stuttgart, HRB 243294

Libvirt currently uses the VIR_AUTOUNREF macro for auto cleanup of virObject instances. GLib approaches things differently with GObject, reusing their g_autoptr() concept. This introduces support for g_autoptr() with virObject, to facilitate the conversion to GObject. Only virObject classes which are currently used with VIR_AUTOREF are updated. Any others should be converted to GObject before introducing use of autocleanup. Reviewed-by: Ján Tomko <jtomko@redhat.com> Signed-off-by: Daniel P. Berrangé <berrange@redhat.com> --- docs/hacking.html.in | 5 +++++ src/conf/capabilities.h | 3 +++ src/conf/domain_capabilities.h | 3 +++ src/conf/domain_conf.h | 3 +++ src/conf/snapshot_conf.h | 3 +++ src/conf/storage_capabilities.h | 3 +++ src/datatypes.h | 15 +++++++++++++++ src/libxl/libxl_conf.h | 2 ++ src/qemu/qemu_blockjob.h | 1 + src/qemu/qemu_capabilities.h | 2 ++ src/qemu/qemu_conf.h | 3 +++ src/util/virhostdev.h | 3 +++ src/util/viridentity.h | 2 ++ src/util/virmdev.h | 3 +++ src/util/virobject.h | 4 ++++ src/util/virpci.h | 3 +++ src/util/virresctrl.h | 4 ++++ src/util/virscsi.h | 3 +++ src/util/virscsivhost.h | 3 +++ src/util/virstoragefile.h | 2 ++ src/util/virusb.h | 3 +++ 21 files changed, 73 insertions(+) diff --git a/docs/hacking.html.in b/docs/hacking.html.in index 6e62b2d4ff..a92608cd3c 100644 --- a/docs/hacking.html.in +++ b/docs/hacking.html.in @@ -1046,6 +1046,11 @@ BAD: g_autoptr, g_auto on an object whose cleanup function is declared with the libvirt macros and vice-verca. </dd> + + <dt>VIR_AUTOUNREF</dt> + <dd>The GLib macros g_autoptr and G_DEFINE_AUTOPTR_CLEANUP_FUNC + should be used to manage autoclean of virObject classes. + This matches usage with GObject classes.</dd> </dl> <h2><a id="file_handling">File handling</a></h2> diff --git a/src/conf/capabilities.h b/src/conf/capabilities.h index d6a4e79d77..4abd3dcabd 100644 --- a/src/conf/capabilities.h +++ b/src/conf/capabilities.h @@ -195,6 +195,9 @@ struct _virCaps { virCapsStoragePoolPtr *pools; }; +G_DEFINE_AUTOPTR_CLEANUP_FUNC(virCaps, virObjectUnref); + + struct _virCapsDomainData { int ostype; int arch; diff --git a/src/conf/domain_capabilities.h b/src/conf/domain_capabilities.h index 4756af38e9..f5571b2188 100644 --- a/src/conf/domain_capabilities.h +++ b/src/conf/domain_capabilities.h @@ -185,6 +185,9 @@ struct _virDomainCaps { /* add new domain features here */ }; +G_DEFINE_AUTOPTR_CLEANUP_FUNC(virDomainCaps, virObjectUnref); + + virDomainCapsPtr virDomainCapsNew(const char *path, const char *machine, virArch arch, diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h index b7ae57aa9d..f7404b814f 100644 --- a/src/conf/domain_conf.h +++ b/src/conf/domain_conf.h @@ -2594,6 +2594,9 @@ struct _virDomainObj { * restore will be required later */ }; +G_DEFINE_AUTOPTR_CLEANUP_FUNC(virDomainObj, virObjectUnref); + + typedef bool (*virDomainObjListACLFilter)(virConnectPtr conn, virDomainDefPtr def); diff --git a/src/conf/snapshot_conf.h b/src/conf/snapshot_conf.h index 17d614a7e1..7e2ffa9d60 100644 --- a/src/conf/snapshot_conf.h +++ b/src/conf/snapshot_conf.h @@ -87,6 +87,9 @@ struct _virDomainSnapshotDef { virObjectPtr cookie; }; +G_DEFINE_AUTOPTR_CLEANUP_FUNC(virDomainSnapshotDef, virObjectUnref); + + typedef enum { VIR_DOMAIN_SNAPSHOT_PARSE_REDEFINE = 1 << 0, VIR_DOMAIN_SNAPSHOT_PARSE_DISKS = 1 << 1, diff --git a/src/conf/storage_capabilities.h b/src/conf/storage_capabilities.h index 948e5bed5b..788ea227ea 100644 --- a/src/conf/storage_capabilities.h +++ b/src/conf/storage_capabilities.h @@ -30,6 +30,9 @@ struct _virStoragePoolCaps { virCapsPtr driverCaps; }; +G_DEFINE_AUTOPTR_CLEANUP_FUNC(virStoragePoolCaps, virObjectUnref); + + virStoragePoolCapsPtr virStoragePoolCapsNew(virCapsPtr driverCaps); diff --git a/src/datatypes.h b/src/datatypes.h index 87e77fff79..16ab5b7282 100644 --- a/src/datatypes.h +++ b/src/datatypes.h @@ -548,6 +548,9 @@ struct _virConnect { void *userData; /* the user data */ }; +G_DEFINE_AUTOPTR_CLEANUP_FUNC(virConnect, virObjectUnref); + + /** * _virAdmConnect: * @@ -616,6 +619,9 @@ struct _virNetwork { unsigned char uuid[VIR_UUID_BUFLEN]; /* the network unique identifier */ }; +G_DEFINE_AUTOPTR_CLEANUP_FUNC(virNetwork, virObjectUnref); + + /** * _virNetworkPort: * @@ -627,6 +633,9 @@ struct _virNetworkPort { unsigned char uuid[VIR_UUID_BUFLEN]; /* the network unique identifier */ }; +G_DEFINE_AUTOPTR_CLEANUP_FUNC(virNetworkPort, virObjectUnref); + + /** * _virInterface: * @@ -658,6 +667,9 @@ struct _virStoragePool { virFreeCallback privateDataFreeFunc; }; +G_DEFINE_AUTOPTR_CLEANUP_FUNC(virStoragePool, virObjectUnref); + + /** * _virStorageVol: * @@ -678,6 +690,9 @@ struct _virStorageVol { virFreeCallback privateDataFreeFunc; }; +G_DEFINE_AUTOPTR_CLEANUP_FUNC(virStorageVol, virObjectUnref); + + /** * _virNodeDevice: * diff --git a/src/libxl/libxl_conf.h b/src/libxl/libxl_conf.h index 27badfb292..80be791b7c 100644 --- a/src/libxl/libxl_conf.h +++ b/src/libxl/libxl_conf.h @@ -102,6 +102,8 @@ struct _libxlDriverConfig { size_t nfirmwares; }; +G_DEFINE_AUTOPTR_CLEANUP_FUNC(libxlDriverConfig, virObjectUnref); + struct _libxlDriverPrivate { virMutex lock; diff --git a/src/qemu/qemu_blockjob.h b/src/qemu/qemu_blockjob.h index 41a5cd91f8..417f253e31 100644 --- a/src/qemu/qemu_blockjob.h +++ b/src/qemu/qemu_blockjob.h @@ -134,6 +134,7 @@ struct _qemuBlockJobData { bool invalidData; /* the job data (except name) is not valid */ bool reconnected; /* internal field for tracking whether job is live after reconnect to qemu */ }; +G_DEFINE_AUTOPTR_CLEANUP_FUNC(qemuBlockJobData, virObjectUnref); int qemuBlockJobRegister(qemuBlockJobDataPtr job, diff --git a/src/qemu/qemu_capabilities.h b/src/qemu/qemu_capabilities.h index ea45edb9a4..10f0ce2654 100644 --- a/src/qemu/qemu_capabilities.h +++ b/src/qemu/qemu_capabilities.h @@ -535,6 +535,8 @@ typedef enum { /* virQEMUCapsFlags grouping marker for syntax-check */ typedef struct _virQEMUCaps virQEMUCaps; typedef virQEMUCaps *virQEMUCapsPtr; +G_DEFINE_AUTOPTR_CLEANUP_FUNC(virQEMUCaps, virObjectUnref); + virQEMUCapsPtr virQEMUCapsNew(void); void virQEMUCapsSet(virQEMUCapsPtr qemuCaps, diff --git a/src/qemu/qemu_conf.h b/src/qemu/qemu_conf.h index 8473d6d4ca..7247199d3e 100644 --- a/src/qemu/qemu_conf.h +++ b/src/qemu/qemu_conf.h @@ -218,6 +218,9 @@ struct _virQEMUDriverConfig { char **capabilityfilters; }; +G_DEFINE_AUTOPTR_CLEANUP_FUNC(virQEMUDriverConfig, virObjectUnref); + + /* Main driver state */ struct _virQEMUDriver { virMutex lock; diff --git a/src/util/virhostdev.h b/src/util/virhostdev.h index 88501e2743..b19a9c3f45 100644 --- a/src/util/virhostdev.h +++ b/src/util/virhostdev.h @@ -55,6 +55,9 @@ struct _virHostdevManager { virMediatedDeviceListPtr activeMediatedHostdevs; }; +G_DEFINE_AUTOPTR_CLEANUP_FUNC(virHostdevManager, virObjectUnref); + + virHostdevManagerPtr virHostdevManagerGetDefault(void); int virHostdevPreparePCIDevices(virHostdevManagerPtr hostdev_mgr, diff --git a/src/util/viridentity.h b/src/util/viridentity.h index 861ecca736..7513dd4e35 100644 --- a/src/util/viridentity.h +++ b/src/util/viridentity.h @@ -26,6 +26,8 @@ typedef struct _virIdentity virIdentity; typedef virIdentity *virIdentityPtr; +G_DEFINE_AUTOPTR_CLEANUP_FUNC(virIdentity, virObjectUnref); + virIdentityPtr virIdentityGetCurrent(void); int virIdentitySetCurrent(virIdentityPtr ident); diff --git a/src/util/virmdev.h b/src/util/virmdev.h index fb125e7056..7f442b571f 100644 --- a/src/util/virmdev.h +++ b/src/util/virmdev.h @@ -40,6 +40,9 @@ typedef virMediatedDevice *virMediatedDevicePtr; typedef struct _virMediatedDeviceList virMediatedDeviceList; typedef virMediatedDeviceList *virMediatedDeviceListPtr; +G_DEFINE_AUTOPTR_CLEANUP_FUNC(virMediatedDeviceList, virObjectUnref); + + typedef struct _virMediatedDeviceType virMediatedDeviceType; typedef virMediatedDeviceType *virMediatedDeviceTypePtr; struct _virMediatedDeviceType { diff --git a/src/util/virobject.h b/src/util/virobject.h index fe5dbe7326..773a009f5e 100644 --- a/src/util/virobject.h +++ b/src/util/virobject.h @@ -116,12 +116,16 @@ virObjectAutoUnref(void *objptr); * VIR_AUTOUNREF: * @type: type of an virObject subclass to be unref'd automatically * + * DEPRECATED: Use g_autoptr(type) instead + * * Declares a variable of @type which will be automatically unref'd when * control goes out of the scope. */ #define VIR_AUTOUNREF(type) \ __attribute__((cleanup(virObjectAutoUnref))) type +G_DEFINE_AUTOPTR_CLEANUP_FUNC(virObject, virObjectUnref); + void * virObjectRef(void *obj); diff --git a/src/util/virpci.h b/src/util/virpci.h index dc20f91710..f3226f1f1b 100644 --- a/src/util/virpci.h +++ b/src/util/virpci.h @@ -34,6 +34,9 @@ typedef virPCIDeviceAddress *virPCIDeviceAddressPtr; typedef struct _virPCIDeviceList virPCIDeviceList; typedef virPCIDeviceList *virPCIDeviceListPtr; +G_DEFINE_AUTOPTR_CLEANUP_FUNC(virPCIDeviceList, virObjectUnref); + + #define VIR_DOMAIN_DEVICE_ZPCI_MAX_UID UINT16_MAX #define VIR_DOMAIN_DEVICE_ZPCI_MAX_FID UINT32_MAX diff --git a/src/util/virresctrl.h b/src/util/virresctrl.h index 216a7302cd..3dd7c96348 100644 --- a/src/util/virresctrl.h +++ b/src/util/virresctrl.h @@ -20,6 +20,7 @@ #include "internal.h" +#include "virobject.h" #include "virbitmap.h" #include "virutil.h" #include "virenum.h" @@ -114,6 +115,9 @@ virResctrlInfoGetMemoryBandwidth(virResctrlInfoPtr resctrl, typedef struct _virResctrlAlloc virResctrlAlloc; typedef virResctrlAlloc *virResctrlAllocPtr; +G_DEFINE_AUTOPTR_CLEANUP_FUNC(virResctrlAlloc, virObjectUnref); + + typedef int virResctrlAllocForeachCacheCallback(unsigned int level, virCacheType type, unsigned int cache, diff --git a/src/util/virscsi.h b/src/util/virscsi.h index 6cc68835b7..8c2c84e07b 100644 --- a/src/util/virscsi.h +++ b/src/util/virscsi.h @@ -30,6 +30,9 @@ typedef virSCSIDevice *virSCSIDevicePtr; typedef struct _virSCSIDeviceList virSCSIDeviceList; typedef virSCSIDeviceList *virSCSIDeviceListPtr; +G_DEFINE_AUTOPTR_CLEANUP_FUNC(virSCSIDeviceList, virObjectUnref); + + char *virSCSIDeviceGetSgName(const char *sysfs_prefix, const char *adapter, unsigned int bus, diff --git a/src/util/virscsivhost.h b/src/util/virscsivhost.h index a1a0ea5618..334eb81af6 100644 --- a/src/util/virscsivhost.h +++ b/src/util/virscsivhost.h @@ -30,6 +30,9 @@ typedef virSCSIVHostDevice *virSCSIVHostDevicePtr; typedef struct _virSCSIVHostDeviceList virSCSIVHostDeviceList; typedef virSCSIVHostDeviceList *virSCSIVHostDeviceListPtr; +G_DEFINE_AUTOPTR_CLEANUP_FUNC(virSCSIVHostDeviceList, virObjectUnref); + + typedef int (*virSCSIVHostDeviceFileActor)(virSCSIVHostDevicePtr dev, const char *name, void *opaque); diff --git a/src/util/virstoragefile.h b/src/util/virstoragefile.h index 81b83a53ef..5b01f9303b 100644 --- a/src/util/virstoragefile.h +++ b/src/util/virstoragefile.h @@ -339,6 +339,8 @@ struct _virStorageSource { bool hostcdrom; /* backing device is a cdrom */ }; +G_DEFINE_AUTOPTR_CLEANUP_FUNC(virStorageSource, virObjectUnref); + #ifndef DEV_BSIZE # define DEV_BSIZE 512 diff --git a/src/util/virusb.h b/src/util/virusb.h index c95514ed3d..33ddb6c84e 100644 --- a/src/util/virusb.h +++ b/src/util/virusb.h @@ -31,6 +31,9 @@ typedef virUSBDevice *virUSBDevicePtr; typedef struct _virUSBDeviceList virUSBDeviceList; typedef virUSBDeviceList *virUSBDeviceListPtr; +G_DEFINE_AUTOPTR_CLEANUP_FUNC(virUSBDeviceList, virObjectUnref); + + virUSBDevicePtr virUSBDeviceNew(unsigned int bus, unsigned int devno, const char *vroot); -- 2.21.0

Reviewed-by: Ján Tomko <jtomko@redhat.com> Signed-off-by: Daniel P. Berrangé <berrange@redhat.com> --- src/conf/virsecretobj.c | 46 +++++++++++++++-------------------------- 1 file changed, 17 insertions(+), 29 deletions(-) diff --git a/src/conf/virsecretobj.c b/src/conf/virsecretobj.c index 7800912bff..aeae82332b 100644 --- a/src/conf/virsecretobj.c +++ b/src/conf/virsecretobj.c @@ -678,43 +678,33 @@ virSecretObjDeleteData(virSecretObjPtr obj) int virSecretObjSaveConfig(virSecretObjPtr obj) { - char *xml = NULL; - int ret = -1; + g_autofree char *xml = NULL; if (!(xml = virSecretDefFormat(obj->def))) - goto cleanup; + return -1; if (virFileRewriteStr(obj->configFile, S_IRUSR | S_IWUSR, xml) < 0) - goto cleanup; - - ret = 0; + return -1; - cleanup: - VIR_FREE(xml); - return ret; + return 0; } int virSecretObjSaveData(virSecretObjPtr obj) { - char *base64 = NULL; - int ret = -1; + g_autofree char *base64 = NULL; if (!obj->value) return 0; if (!(base64 = virStringEncodeBase64(obj->value, obj->value_size))) - goto cleanup; + return -1; if (virFileRewriteStr(obj->base64File, S_IRUSR | S_IWUSR, base64) < 0) - goto cleanup; - - ret = 0; + return -1; - cleanup: - VIR_FREE(base64); - return ret; + return 0; } @@ -762,7 +752,8 @@ virSecretObjSetValue(virSecretObjPtr obj, size_t value_size) { virSecretDefPtr def = obj->def; - unsigned char *old_value, *new_value; + g_autofree unsigned char *old_value = NULL; + g_autofree unsigned char *new_value = NULL; size_t old_value_size; if (VIR_ALLOC_N(new_value, value_size) < 0) @@ -772,26 +763,24 @@ virSecretObjSetValue(virSecretObjPtr obj, old_value_size = obj->value_size; memcpy(new_value, value, value_size); - obj->value = new_value; + obj->value = g_steal_pointer(&new_value); obj->value_size = value_size; if (!def->isephemeral && virSecretObjSaveData(obj) < 0) goto error; /* Saved successfully - drop old value */ - if (old_value) { + if (old_value) memset(old_value, 0, old_value_size); - VIR_FREE(old_value); - } return 0; error: /* Error - restore previous state and free new value */ - obj->value = old_value; + new_value = g_steal_pointer(&obj->value); + obj->value = g_steal_pointer(&old_value); obj->value_size = old_value_size; memset(new_value, 0, value_size); - VIR_FREE(new_value); return -1; } @@ -835,7 +824,8 @@ virSecretLoadValue(virSecretObjPtr obj) { int ret = -1, fd = -1; struct stat st; - char *contents = NULL, *value = NULL; + g_autofree char *contents = NULL; + char *value = NULL; size_t value_size; if ((fd = open(obj->base64File, O_RDONLY)) == -1) { @@ -892,10 +882,8 @@ virSecretLoadValue(virSecretObjPtr obj) memset(value, 0, value_size); VIR_FREE(value); } - if (contents != NULL) { + if (contents != NULL) memset(contents, 0, st.st_size); - VIR_FREE(contents); - } VIR_FORCE_CLOSE(fd); return ret; } -- 2.21.0

Replace use of the gnulib base64 module with glib's own base64 API family. Reviewed-by: Ján Tomko <jtomko@redhat.com> Signed-off-by: Daniel P. Berrangé <berrange@redhat.com> --- bootstrap.conf | 1 - configure.ac | 5 ----- src/conf/virsecretobj.c | 26 ++++---------------------- src/libvirt_private.syms | 1 - src/libxl/libxl_conf.c | 3 +-- src/qemu/qemu_agent.c | 6 ++---- src/qemu/qemu_command.c | 5 ++--- src/qemu/qemu_domain.c | 8 +++----- src/secret/secret_driver.c | 1 - src/storage/storage_backend_rbd.c | 4 +--- src/util/virstring.c | 21 --------------------- src/util/virstring.h | 2 -- tools/virsh-secret.c | 17 ++++------------- 13 files changed, 17 insertions(+), 83 deletions(-) diff --git a/bootstrap.conf b/bootstrap.conf index 7e264b63ad..bb40e978aa 100644 --- a/bootstrap.conf +++ b/bootstrap.conf @@ -20,7 +20,6 @@ gnulib_modules=' accept areadlink -base64 bind byteswap c-ctype diff --git a/configure.ac b/configure.ac index 9b4e6fdd6d..acac1bed41 100644 --- a/configure.ac +++ b/configure.ac @@ -911,11 +911,6 @@ test "x$lv_cv_static_analysis" = xyes && t=1 AC_DEFINE_UNQUOTED([STATIC_ANALYSIS], [$t], [Define to 1 when performing static analysis.]) -# Some GNULIB base64 symbols clash with a kerberos library -AC_DEFINE_UNQUOTED([isbase64],[libvirt_gl_isbase64],[Hack to avoid symbol clash]) -AC_DEFINE_UNQUOTED([base64_encode],[libvirt_gl_base64_encode],[Hack to avoid symbol clash]) -AC_DEFINE_UNQUOTED([base64_encode_alloc],[libvirt_gl_base64_encode_alloc],[Hack to avoid symbol clash]) - GNUmakefile=GNUmakefile m4_if(m4_version_compare([2.61a.100], m4_defn([m4_PACKAGE_VERSION])), [1], [], diff --git a/src/conf/virsecretobj.c b/src/conf/virsecretobj.c index aeae82332b..5bd84d82ed 100644 --- a/src/conf/virsecretobj.c +++ b/src/conf/virsecretobj.c @@ -31,7 +31,6 @@ #include "virhash.h" #include "virlog.h" #include "virstring.h" -#include "base64.h" #define VIR_FROM_THIS VIR_FROM_SECRET @@ -698,8 +697,7 @@ virSecretObjSaveData(virSecretObjPtr obj) if (!obj->value) return 0; - if (!(base64 = virStringEncodeBase64(obj->value, obj->value_size))) - return -1; + base64 = g_base64_encode(obj->value, obj->value_size); if (virFileRewriteStr(obj->base64File, S_IRUSR | S_IWUSR, base64) < 0) return -1; @@ -825,8 +823,6 @@ virSecretLoadValue(virSecretObjPtr obj) int ret = -1, fd = -1; struct stat st; g_autofree char *contents = NULL; - char *value = NULL; - size_t value_size; if ((fd = open(obj->base64File, O_RDONLY)) == -1) { if (errno == ENOENT) { @@ -851,7 +847,7 @@ virSecretLoadValue(virSecretObjPtr obj) goto cleanup; } - if (VIR_ALLOC_N(contents, st.st_size) < 0) + if (VIR_ALLOC_N(contents, st.st_size + 1) < 0) goto cleanup; if (saferead(fd, contents, st.st_size) != st.st_size) { @@ -859,29 +855,15 @@ virSecretLoadValue(virSecretObjPtr obj) obj->base64File); goto cleanup; } + contents[st.st_size] = '\0'; VIR_FORCE_CLOSE(fd); - if (!base64_decode_alloc(contents, st.st_size, &value, &value_size)) { - virReportError(VIR_ERR_INTERNAL_ERROR, - _("invalid base64 in '%s'"), - obj->base64File); - goto cleanup; - } - if (value == NULL) - goto cleanup; - - obj->value = (unsigned char *)value; - value = NULL; - obj->value_size = value_size; + obj->value = g_base64_decode(contents, &obj->value_size); ret = 0; cleanup: - if (value != NULL) { - memset(value, 0, value_size); - VIR_FREE(value); - } if (contents != NULL) memset(contents, 0, st.st_size); VIR_FORCE_CLOSE(fd); diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index 5949cba08d..e88518a1ce 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -3069,7 +3069,6 @@ virSkipSpacesBackwards; virStrcpy; virStrdup; virStringBufferIsPrintable; -virStringEncodeBase64; virStringFilterChars; virStringHasCaseSuffix; virStringHasChars; diff --git a/src/libxl/libxl_conf.c b/src/libxl/libxl_conf.c index c76704a11d..de56567cf0 100644 --- a/src/libxl/libxl_conf.c +++ b/src/libxl/libxl_conf.c @@ -999,8 +999,7 @@ libxlMakeNetworkDiskSrc(virStorageSourcePtr src, char **srcstr) goto cleanup; /* RBD expects an encoded secret */ - if (!(base64secret = virStringEncodeBase64(secret, secretlen))) - goto cleanup; + base64secret = g_base64_encode(secret, secretlen); } if (!(*srcstr = libxlMakeNetworkDiskSrcStr(src, username, base64secret))) diff --git a/src/qemu/qemu_agent.c b/src/qemu/qemu_agent.c index 34e1a85d64..0ef8b563f5 100644 --- a/src/qemu/qemu_agent.c +++ b/src/qemu/qemu_agent.c @@ -39,7 +39,6 @@ #include "virtime.h" #include "virobject.h" #include "virstring.h" -#include "base64.h" #include "virenum.h" #define VIR_FROM_THIS VIR_FROM_QEMU @@ -2518,9 +2517,8 @@ qemuAgentSetUserPassword(qemuAgentPtr mon, virJSONValuePtr reply = NULL; char *password64 = NULL; - if (!(password64 = virStringEncodeBase64((unsigned char *)password, - strlen(password)))) - goto cleanup; + password64 = g_base64_encode((unsigned char *)password, + strlen(password)); if (!(cmd = qemuAgentMakeCommand("guest-set-user-password", "b:crypted", crypted, diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index 50cc3bdf7c..0f1625d401 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -837,9 +837,8 @@ qemuBuildRBDSecinfoURI(virBufferPtr buf, switch ((qemuDomainSecretInfoType) secinfo->type) { case VIR_DOMAIN_SECRET_INFO_TYPE_PLAIN: - if (!(base64secret = virStringEncodeBase64(secinfo->s.plain.secret, - secinfo->s.plain.secretlen))) - return -1; + base64secret = g_base64_encode(secinfo->s.plain.secret, + secinfo->s.plain.secretlen); virBufferEscape(buf, '\\', ":", ":id=%s", secinfo->s.plain.username); virBufferEscape(buf, '\\', ":", ":key=%s:auth_supported=cephx\\;none", diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c index dc7568fe18..35067c851f 100644 --- a/src/qemu/qemu_domain.c +++ b/src/qemu/qemu_domain.c @@ -1470,8 +1470,7 @@ qemuDomainSecretAESSetup(qemuDomainObjPrivatePtr priv, goto cleanup; /* Encode the IV and save that since qemu will need it */ - if (!(secinfo->s.aes.iv = virStringEncodeBase64(raw_iv, ivlen))) - goto cleanup; + secinfo->s.aes.iv = g_base64_encode(raw_iv, ivlen); /* Grab the unencoded secret */ if (virSecretGetSecretString(conn, seclookupdef, usageType, @@ -1488,9 +1487,8 @@ qemuDomainSecretAESSetup(qemuDomainObjPrivatePtr priv, memset(secret, 0, secretlen); /* Now encode the ciphertext and store to be passed to qemu */ - if (!(secinfo->s.aes.ciphertext = virStringEncodeBase64(ciphertext, - ciphertextlen))) - goto cleanup; + secinfo->s.aes.ciphertext = g_base64_encode(ciphertext, + ciphertextlen); ret = 0; diff --git a/src/secret/secret_driver.c b/src/secret/secret_driver.c index ed3bd3c751..13f75ee4fa 100644 --- a/src/secret/secret_driver.c +++ b/src/secret/secret_driver.c @@ -25,7 +25,6 @@ #include <unistd.h> #include "internal.h" -#include "base64.h" #include "datatypes.h" #include "driver.h" #include "virlog.h" diff --git a/src/storage/storage_backend_rbd.c b/src/storage/storage_backend_rbd.c index c4781debd8..b10ca1503d 100644 --- a/src/storage/storage_backend_rbd.c +++ b/src/storage/storage_backend_rbd.c @@ -28,7 +28,6 @@ #include "storage_conf.h" #include "viralloc.h" #include "virlog.h" -#include "base64.h" #include "viruuid.h" #include "virstring.h" #include "virrandom.h" @@ -218,8 +217,7 @@ virStorageBackendRBDOpenRADOSConn(virStorageBackendRBDStatePtr ptr, &secret_value, &secret_value_size) < 0) goto cleanup; - if (!(rados_key = virStringEncodeBase64(secret_value, secret_value_size))) - goto cleanup; + rados_key = g_base64_encode(secret_value, secret_value_size); if (virStorageBackendRBDRADOSConfSet(ptr->cluster, "key", rados_key) < 0) diff --git a/src/util/virstring.c b/src/util/virstring.c index 6b2b6ed24e..0abdcd8b04 100644 --- a/src/util/virstring.c +++ b/src/util/virstring.c @@ -22,7 +22,6 @@ #include <regex.h> #include <locale.h> -#include "base64.h" #include "c-ctype.h" #include "virstring.h" #include "virthread.h" @@ -1438,26 +1437,6 @@ virStringBufferIsPrintable(const uint8_t *buf, } -/** - * virStringEncodeBase64: - * @buf: buffer of bytes to encode - * @buflen: number of bytes to encode - * - * Encodes @buf to base 64 and returns the resulting string. The caller is - * responsible for freeing the result. - */ -char * -virStringEncodeBase64(const uint8_t *buf, size_t buflen) -{ - char *ret; - - base64_encode_alloc((const char *) buf, buflen, &ret); - if (!ret) - abort(); - - return ret; -} - /** * virStringTrimOptionalNewline: * @str: the string to modify in-place diff --git a/src/util/virstring.h b/src/util/virstring.h index 3ffe51f7b8..af6e234d83 100644 --- a/src/util/virstring.h +++ b/src/util/virstring.h @@ -291,8 +291,6 @@ void virStringFilterChars(char *str, const char *valid); bool virStringIsPrintable(const char *str); bool virStringBufferIsPrintable(const uint8_t *buf, size_t buflen); -char *virStringEncodeBase64(const uint8_t *buf, size_t buflen); - void virStringTrimOptionalNewline(char *str); int virStringParsePort(const char *str, diff --git a/tools/virsh-secret.c b/tools/virsh-secret.c index b34ae12bbe..48058bea05 100644 --- a/tools/virsh-secret.c +++ b/tools/virsh-secret.c @@ -22,7 +22,6 @@ #include "virsh-secret.h" #include "internal.h" -#include "base64.h" #include "virbuffer.h" #include "viralloc.h" #include "virfile.h" @@ -192,7 +191,7 @@ cmdSecretSetValue(vshControl *ctl, const vshCmd *cmd) virSecretPtr secret; size_t value_size; const char *base64 = NULL; - char *value; + unsigned char *value; int res; bool ret = false; @@ -202,16 +201,9 @@ cmdSecretSetValue(vshControl *ctl, const vshCmd *cmd) if (vshCommandOptStringReq(ctl, cmd, "base64", &base64) < 0) goto cleanup; - if (!base64_decode_alloc(base64, strlen(base64), &value, &value_size)) { - vshError(ctl, "%s", _("Invalid base64 data")); - goto cleanup; - } - if (value == NULL) { - vshError(ctl, "%s", _("Failed to allocate memory")); - goto cleanup; - } + value = g_base64_decode(base64, &value_size); - res = virSecretSetValue(secret, (unsigned char *)value, value_size, 0); + res = virSecretSetValue(secret, value, value_size, 0); memset(value, 0, value_size); VIR_FREE(value); @@ -267,8 +259,7 @@ cmdSecretGetValue(vshControl *ctl, const vshCmd *cmd) if (value == NULL) goto cleanup; - if (!(base64 = virStringEncodeBase64(value, value_size))) - goto cleanup; + base64 = g_base64_encode(value, value_size); vshPrint(ctl, "%s", base64); ret = true; -- 2.21.0

To simplify the later conversion from virObject to GObject, introduce the use of g_autoptr to the virIdentity implementnation and test suite. Reviewed-by: Ján Tomko <jtomko@redhat.com> Signed-off-by: Daniel P. Berrangé <berrange@redhat.com> --- src/util/viridentity.c | 36 ++++++++++++++------------------- tests/viridentitytest.c | 44 +++++++++++++++++------------------------ 2 files changed, 33 insertions(+), 47 deletions(-) diff --git a/src/util/viridentity.c b/src/util/viridentity.c index 22e2644c19..6636077161 100644 --- a/src/util/viridentity.c +++ b/src/util/viridentity.c @@ -105,7 +105,7 @@ virIdentityPtr virIdentityGetCurrent(void) */ int virIdentitySetCurrent(virIdentityPtr ident) { - virIdentityPtr old; + g_autoptr(virIdentity) old = NULL; if (virIdentityInitialize() < 0) return -1; @@ -120,8 +120,6 @@ int virIdentitySetCurrent(virIdentityPtr ident) return -1; } - virObjectUnref(old); - return 0; } @@ -136,60 +134,56 @@ int virIdentitySetCurrent(virIdentityPtr ident) */ virIdentityPtr virIdentityGetSystem(void) { - VIR_AUTOFREE(char *) username = NULL; - VIR_AUTOFREE(char *) groupname = NULL; + g_autofree char *username = NULL; + g_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) { if (getcon(&con) < 0) { virReportSystemError(errno, "%s", _("Unable to lookup SELinux process context")); - return ret; + return NULL; } 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); } diff --git a/tests/viridentitytest.c b/tests/viridentitytest.c index 1eadd6173a..db041a98a8 100644 --- a/tests/viridentitytest.c +++ b/tests/viridentitytest.c @@ -38,58 +38,53 @@ 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; + return -1; 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 +92,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) -- 2.21.0

Reviewed-by: Ján Tomko <jtomko@redhat.com> Signed-off-by: Daniel P. Berrangé <berrange@redhat.com> --- src/access/viraccessdriverpolkit.c | 38 +++++++++++------------------- 1 file changed, 14 insertions(+), 24 deletions(-) diff --git a/src/access/viraccessdriverpolkit.c b/src/access/viraccessdriverpolkit.c index e61ac6fa19..fbe7789ccd 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; } @@ -130,21 +125,20 @@ virAccessDriverPolkitCheck(virAccessManagerPtr manager ATTRIBUTE_UNUSED, const char *permname, const char **attrs) { - char *actionid = NULL; - int ret = -1; + g_autofree char *actionid = NULL; pid_t pid; uid_t uid; unsigned long long startTime; int rv; if (!(actionid = virAccessDriverPolkitFormatAction(typename, permname))) - goto cleanup; + return -1; if (virAccessDriverPolkitGetCaller(actionid, &pid, &startTime, &uid) < 0) - goto cleanup; + return -1; VIR_DEBUG("Check action '%s' for process '%lld' time %lld uid %d", actionid, (long long)pid, startTime, uid); @@ -157,18 +151,14 @@ virAccessDriverPolkitCheck(virAccessManagerPtr manager ATTRIBUTE_UNUSED, false); if (rv == 0) { - ret = 1; /* Allowed */ + return 1; /* Allowed */ } else { if (rv == -2) { - ret = 0; /* Denied */ + return 0; /* Denied */ } else { - ret = -1; /* Error */ + return -1; /* Error */ } } - - cleanup: - VIR_FREE(actionid); - return ret; } -- 2.21.0

Reviewed-by: Ján Tomko <jtomko@redhat.com> Signed-off-by: Daniel P. Berrangé <berrange@redhat.com> --- src/admin/admin_server.c | 204 ++++++++++++++++----------------------- 1 file changed, 85 insertions(+), 119 deletions(-) diff --git a/src/admin/admin_server.c b/src/admin/admin_server.c index 0d6091937d..ba87f701c3 100644 --- a/src/admin/admin_server.c +++ b/src/admin/admin_server.c @@ -75,15 +75,13 @@ adminServerGetThreadPoolParameters(virNetServerPtr srv, int *nparams, unsigned int flags) { - int ret = -1; - int maxparams = 0; size_t minWorkers; size_t maxWorkers; size_t nWorkers; size_t freeWorkers; size_t nPrioWorkers; size_t jobQueueDepth; - virTypedParameterPtr tmpparams = NULL; + g_autoptr(virTypedParamList) paramlist = g_new0(virTypedParamList, 1); virCheckFlags(0, -1); @@ -93,46 +91,36 @@ adminServerGetThreadPoolParameters(virNetServerPtr srv, &jobQueueDepth) < 0) { virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("Unable to retrieve threadpool parameters")); - goto cleanup; + return -1; } - if (virTypedParamsAddUInt(&tmpparams, nparams, - &maxparams, VIR_THREADPOOL_WORKERS_MIN, - minWorkers) < 0) - goto cleanup; + if (virTypedParamListAddUInt(paramlist, minWorkers, + "%s", VIR_THREADPOOL_WORKERS_MIN) < 0) + return -1; - if (virTypedParamsAddUInt(&tmpparams, nparams, - &maxparams, VIR_THREADPOOL_WORKERS_MAX, - maxWorkers) < 0) - goto cleanup; + if (virTypedParamListAddUInt(paramlist, maxWorkers, + "%s", VIR_THREADPOOL_WORKERS_MAX) < 0) + return -1; - if (virTypedParamsAddUInt(&tmpparams, nparams, - &maxparams, VIR_THREADPOOL_WORKERS_CURRENT, - nWorkers) < 0) - goto cleanup; + if (virTypedParamListAddUInt(paramlist, nWorkers, + "%s", VIR_THREADPOOL_WORKERS_CURRENT) < 0) + return -1; - if (virTypedParamsAddUInt(&tmpparams, nparams, - &maxparams, VIR_THREADPOOL_WORKERS_FREE, - freeWorkers) < 0) - goto cleanup; + if (virTypedParamListAddUInt(paramlist, freeWorkers, + "%s", VIR_THREADPOOL_WORKERS_FREE) < 0) + return -1; - if (virTypedParamsAddUInt(&tmpparams, nparams, - &maxparams, VIR_THREADPOOL_WORKERS_PRIORITY, - nPrioWorkers) < 0) - goto cleanup; + if (virTypedParamListAddUInt(paramlist, nPrioWorkers, + "%s", VIR_THREADPOOL_WORKERS_PRIORITY) < 0) + return -1; - if (virTypedParamsAddUInt(&tmpparams, nparams, - &maxparams, VIR_THREADPOOL_JOB_QUEUE_DEPTH, - jobQueueDepth) < 0) - goto cleanup; + if (virTypedParamListAddUInt(paramlist, jobQueueDepth, + "%s", VIR_THREADPOOL_JOB_QUEUE_DEPTH) < 0) + return -1; - *params = tmpparams; - tmpparams = NULL; - ret = 0; + *nparams = virTypedParamListStealParams(paramlist, params); - cleanup: - virTypedParamsFree(tmpparams, *nparams); - return ret; + return 0; } int @@ -215,106 +203,90 @@ adminClientGetInfo(virNetServerClientPtr client, int *nparams, unsigned int flags) { - int ret = -1; - int maxparams = 0; bool readonly; - char *sock_addr = NULL; + g_autofree char *sock_addr = NULL; const char *attr = NULL; - virTypedParameterPtr tmpparams = NULL; - virIdentityPtr identity = NULL; + g_autoptr(virTypedParamList) paramlist = g_new0(virTypedParamList, 1); + g_autoptr(virIdentity) identity = NULL; int rc; virCheckFlags(0, -1); if (virNetServerClientGetInfo(client, &readonly, &sock_addr, &identity) < 0) - goto cleanup; + return -1; - if (virTypedParamsAddBoolean(&tmpparams, nparams, &maxparams, - VIR_CLIENT_INFO_READONLY, - readonly) < 0) - goto cleanup; + if (virTypedParamListAddBoolean(paramlist, readonly, + "%s", VIR_CLIENT_INFO_READONLY) < 0) + return -1; if ((rc = virIdentityGetSASLUserName(identity, &attr)) < 0) - goto cleanup; + return -1; if (rc == 1 && - virTypedParamsAddString(&tmpparams, nparams, &maxparams, - VIR_CLIENT_INFO_SASL_USER_NAME, - attr) < 0) - goto cleanup; + virTypedParamListAddString(paramlist, attr, + "%s", VIR_CLIENT_INFO_SASL_USER_NAME) < 0) + return -1; if (!virNetServerClientIsLocal(client)) { - if (virTypedParamsAddString(&tmpparams, nparams, &maxparams, - VIR_CLIENT_INFO_SOCKET_ADDR, - sock_addr) < 0) - goto cleanup; + if (virTypedParamListAddString(paramlist, sock_addr, + "%s", VIR_CLIENT_INFO_SOCKET_ADDR) < 0) + return -1; if ((rc = virIdentityGetX509DName(identity, &attr)) < 0) - goto cleanup; + return -1; if (rc == 1 && - virTypedParamsAddString(&tmpparams, nparams, &maxparams, - VIR_CLIENT_INFO_X509_DISTINGUISHED_NAME, - attr) < 0) - goto cleanup; + virTypedParamListAddString(paramlist, attr, + "%s", VIR_CLIENT_INFO_X509_DISTINGUISHED_NAME) < 0) + return -1; } else { pid_t pid; uid_t uid; gid_t gid; if ((rc = virIdentityGetUNIXUserID(identity, &uid)) < 0) - goto cleanup; + return -1; if (rc == 1 && - virTypedParamsAddInt(&tmpparams, nparams, &maxparams, - VIR_CLIENT_INFO_UNIX_USER_ID, uid) < 0) - goto cleanup; + virTypedParamListAddInt(paramlist, uid, + "%s", VIR_CLIENT_INFO_UNIX_USER_ID) < 0) + return -1; if ((rc = virIdentityGetUserName(identity, &attr)) < 0) - goto cleanup; + return -1; if (rc == 1 && - virTypedParamsAddString(&tmpparams, nparams, &maxparams, - VIR_CLIENT_INFO_UNIX_USER_NAME, - attr) < 0) - goto cleanup; + virTypedParamListAddString(paramlist, attr, + "%s", VIR_CLIENT_INFO_UNIX_USER_NAME) < 0) + return -1; if ((rc = virIdentityGetUNIXGroupID(identity, &gid)) < 0) - goto cleanup; + return -1; if (rc == 1 && - virTypedParamsAddInt(&tmpparams, nparams, &maxparams, - VIR_CLIENT_INFO_UNIX_GROUP_ID, gid) < 0) - goto cleanup; + virTypedParamListAddInt(paramlist, gid, + "%s", VIR_CLIENT_INFO_UNIX_GROUP_ID) < 0) + return -1; if ((rc = virIdentityGetGroupName(identity, &attr)) < 0) - goto cleanup; + return -1; if (rc == 1 && - virTypedParamsAddString(&tmpparams, nparams, &maxparams, - VIR_CLIENT_INFO_UNIX_GROUP_NAME, - attr) < 0) - goto cleanup; + virTypedParamListAddString(paramlist, attr, + "%s", VIR_CLIENT_INFO_UNIX_GROUP_NAME) < 0) + return -1; if ((rc = virIdentityGetProcessID(identity, &pid)) < 0) - goto cleanup; + return -1; if (rc == 1 && - virTypedParamsAddInt(&tmpparams, nparams, &maxparams, - VIR_CLIENT_INFO_UNIX_PROCESS_ID, pid) < 0) - goto cleanup; + virTypedParamListAddInt(paramlist, pid, + "%s", VIR_CLIENT_INFO_UNIX_PROCESS_ID) < 0) + return -1; } if ((rc = virIdentityGetSELinuxContext(identity, &attr)) < 0) - goto cleanup; + return -1; if (rc == 1 && - virTypedParamsAddString(&tmpparams, nparams, &maxparams, - VIR_CLIENT_INFO_SELINUX_CONTEXT, attr) < 0) - goto cleanup; - - *params = tmpparams; - tmpparams = NULL; - ret = 0; + virTypedParamListAddString(paramlist, attr, + "%s", VIR_CLIENT_INFO_SELINUX_CONTEXT) < 0) + return -1; - cleanup: - if (tmpparams) - virTypedParamsFree(tmpparams, *nparams); - virObjectUnref(identity); - VIR_FREE(sock_addr); - return ret; + *nparams = virTypedParamListStealParams(paramlist, params); + return 0; } int adminClientClose(virNetServerClientPtr client, @@ -332,39 +304,33 @@ adminServerGetClientLimits(virNetServerPtr srv, int *nparams, unsigned int flags) { - int ret = -1; - int maxparams = 0; - virTypedParameterPtr tmpparams = NULL; + g_autoptr(virTypedParamList) paramlist = g_new0(virTypedParamList, 1); virCheckFlags(0, -1); - if (virTypedParamsAddUInt(&tmpparams, nparams, &maxparams, - VIR_SERVER_CLIENTS_MAX, - virNetServerGetMaxClients(srv)) < 0) - goto cleanup; + if (virTypedParamListAddUInt(paramlist, + virNetServerGetMaxClients(srv), + "%s", VIR_SERVER_CLIENTS_MAX) < 0) + return -1; - if (virTypedParamsAddUInt(&tmpparams, nparams, &maxparams, - VIR_SERVER_CLIENTS_CURRENT, - virNetServerGetCurrentClients(srv)) < 0) - goto cleanup; + if (virTypedParamListAddUInt(paramlist, + virNetServerGetCurrentClients(srv), + "%s", VIR_SERVER_CLIENTS_CURRENT) < 0) + return -1; - if (virTypedParamsAddUInt(&tmpparams, nparams, &maxparams, - VIR_SERVER_CLIENTS_UNAUTH_MAX, - virNetServerGetMaxUnauthClients(srv)) < 0) - goto cleanup; + if (virTypedParamListAddUInt(paramlist, + virNetServerGetMaxUnauthClients(srv), + "%s", VIR_SERVER_CLIENTS_UNAUTH_MAX) < 0) + return -1; - if (virTypedParamsAddUInt(&tmpparams, nparams, &maxparams, - VIR_SERVER_CLIENTS_UNAUTH_CURRENT, - virNetServerGetCurrentUnauthClients(srv)) < 0) - goto cleanup; + if (virTypedParamListAddUInt(paramlist, + virNetServerGetCurrentUnauthClients(srv), + "%s", VIR_SERVER_CLIENTS_UNAUTH_CURRENT) < 0) + return -1; - *params = tmpparams; - tmpparams = NULL; - ret = 0; + *nparams = virTypedParamListStealParams(paramlist, params); - cleanup: - virTypedParamsFree(tmpparams, *nparams); - return ret; + return 0; } int -- 2.21.0

Reviewed-by: Ján Tomko <jtomko@redhat.com> Signed-off-by: Daniel P. Berrangé <berrange@redhat.com> --- src/rpc/virnetserverclient.c | 47 ++++++++++++++-------------------- src/rpc/virnetserverprogram.c | 13 +++------- tests/virnetserverclienttest.c | 3 +-- 3 files changed, 23 insertions(+), 40 deletions(-) diff --git a/src/rpc/virnetserverclient.c b/src/rpc/virnetserverclient.c index 171ee636dd..79287572b6 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); } 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/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

Reviewed-by: Ján Tomko <jtomko@redhat.com> Signed-off-by: Daniel P. Berrangé <berrange@redhat.com> --- src/remote/remote_daemon.c | 3 +-- src/remote/remote_daemon_dispatch.c | 35 ++++++++++------------------- 2 files changed, 13 insertions(+), 25 deletions(-) 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; } /* -- 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, vir_identity, 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, vir_identity, 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. Reviewed-by: Ján Tomko <jtomko@redhat.com> Signed-off-by: Daniel P. Berrangé <berrange@redhat.com> --- m4/virt-glib.m4 | 4 +-- src/qemu/qemu_process.c | 4 +-- src/rpc/virnetserverclient.c | 10 +++---- src/util/viridentity.c | 56 ++++++++++++++++++++++-------------- src/util/viridentity.h | 9 +++--- tests/viridentitytest.c | 5 +--- 6 files changed, 49 insertions(+), 39 deletions(-) diff --git a/m4/virt-glib.m4 b/m4/virt-glib.m4 index 5a5bc19660..eb2c77b25b 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], [$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 >= $GLIB_REQUIRED are required for libvirt]) fi ]) diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c index c14c09da11..3b45b2f641 100644 --- a/src/qemu/qemu_process.c +++ b/src/qemu/qemu_process.c @@ -8040,7 +8040,7 @@ qemuProcessReconnect(void *opaque) bool tryMonReconn = false; virIdentitySetCurrent(data->identity); - virObjectUnref(data->identity); + g_clear_object(&data->identity); VIR_FREE(data); qemuDomainObjRestoreJob(obj, &oldjob); @@ -8353,7 +8353,7 @@ qemuProcessReconnectHelper(virDomainObjPtr obj, virDomainObjEndAPI(&obj); virNWFilterUnlockFilterUpdates(); - virObjectUnref(data->identity); + g_clear_object(&data->identity); VIR_FREE(data); return -1; } diff --git a/src/rpc/virnetserverclient.c b/src/rpc/virnetserverclient.c index 79287572b6..8482c5c29c 100644 --- a/src/rpc/virnetserverclient.c +++ b/src/rpc/virnetserverclient.c @@ -829,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; } @@ -839,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); } @@ -979,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); @@ -1674,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/util/viridentity.c b/src/util/viridentity.c index 6636077161..8cc2db2568 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, vir_identity, G_TYPE_OBJECT) + static virThreadLocal virIdentityCurrent; -static void virIdentityDispose(void *obj); +static void virIdentityFinalize(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 vir_identity_init(virIdentity *ident G_GNUC_UNUSED) +{ +} + +static void vir_identity_class_init(virIdentityClass *klass) +{ + GObjectClass *obj = G_OBJECT_CLASS(klass); + + obj->finalize = virIdentityFinalize; +} + /** * 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; } @@ -113,10 +130,11 @@ 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; } @@ -197,23 +215,17 @@ 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 virIdentityFinalize(GObject *object) { - virIdentityPtr ident = object; + virIdentityPtr ident = VIR_IDENTITY(object); virTypedParamsFree(ident->params, ident->nparams); + + G_OBJECT_CLASS(vir_identity_parent_class)->finalize(object); } diff --git a/src/util/viridentity.h b/src/util/viridentity.h index 7513dd4e35..2940e02054 100644 --- a/src/util/viridentity.h +++ b/src/util/viridentity.h @@ -21,12 +21,13 @@ #pragma once -#include "virobject.h" +#include "internal.h" +#include <glib-object.h> -typedef struct _virIdentity virIdentity; -typedef virIdentity *virIdentityPtr; +#define VIR_TYPE_IDENTITY vir_identity_get_type() +G_DECLARE_FINAL_TYPE(virIdentity, vir_identity, VIR, IDENTITY, GObject); -G_DEFINE_AUTOPTR_CLEANUP_FUNC(virIdentity, virObjectUnref); +typedef virIdentity *virIdentityPtr; virIdentityPtr virIdentityGetCurrent(void); int virIdentitySetCurrent(virIdentityPtr ident); diff --git a/tests/viridentitytest.c b/tests/viridentitytest.c index db041a98a8..90e7247817 100644 --- a/tests/viridentitytest.c +++ b/tests/viridentitytest.c @@ -38,13 +38,10 @@ VIR_LOG_INIT("tests.identitytest"); static int testIdentityAttrs(const void *data ATTRIBUTE_UNUSED) { - g_autoptr(virIdentity) ident = NULL; + g_autoptr(virIdentity) ident = virIdentityNew(); const char *val; int rc; - if (!(ident = virIdentityNew())) - return -1; - if (virIdentitySetUserName(ident, "fred") < 0) return -1; -- 2.21.0

Reviewed-by: Ján Tomko <jtomko@redhat.com> Reviewed-by: Pavel Hrdina <phrdina@redhat.com> Signed-off-by: Daniel P. Berrangé <berrange@redhat.com> --- src/libxl/libxl_capabilities.c | 42 +++++++++++++++------------------- 1 file changed, 19 insertions(+), 23 deletions(-) diff --git a/src/libxl/libxl_capabilities.c b/src/libxl/libxl_capabilities.c index 73ae0b3fa1..65c68ffb52 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,31 @@ 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 +512,6 @@ libxlCapsInitGuests(libxl_ctx *ctx, virCapsPtr caps) #endif } } - regfree(®ex); for (i = 0; i < nr_guest_archs; ++i) { virCapsGuestPtr guest; -- 2.21.0

Reviewed-by: Ján Tomko <jtomko@redhat.com> Reviewed-by: Pavel Hrdina <phrdina@redhat.com> Signed-off-by: Daniel P. Berrangé <berrange@redhat.com> --- src/conf/domain_event.c | 25 ++++++++++--------------- 1 file changed, 10 insertions(+), 15 deletions(-) diff --git a/src/conf/domain_event.c b/src/conf/domain_event.c index b33589f472..40031a46c8 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 = G_REGEX_OPTIMIZE; + 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; } -- 2.21.0

g_strerror is offers the safety/correctness benefits of strerror_r, with the API design convenience of strerror. Use of virStrerror should be eliminated through the codebase in favour of g_strerror. commandhelper.c is a special case as its a tiny single threaded test program, not linked to glib, so it just uses traditional strerror(). Signed-off-by: Daniel P. Berrangé <berrange@redhat.com> --- bootstrap.conf | 2 -- docs/hacking.html.in | 4 ++++ src/util/virerror.c | 9 +++++---- src/util/virerror.h | 1 + tests/commandtest.c | 10 +++++----- tests/qemumonitortestutils.c | 2 +- tests/seclabeltest.c | 4 ++-- tests/testutils.c | 6 +++--- tests/virhostcputest.c | 4 ++-- tests/virtestmock.c | 4 ++-- 10 files changed, 25 insertions(+), 21 deletions(-) diff --git a/bootstrap.conf b/bootstrap.conf index bb40e978aa..241dce50c2 100644 --- a/bootstrap.conf +++ b/bootstrap.conf @@ -79,8 +79,6 @@ snprintf socket stat-time strchrnul -strerror -strerror_r-posix strptime strsep strtok_r diff --git a/docs/hacking.html.in b/docs/hacking.html.in index a92608cd3c..6cf796a175 100644 --- a/docs/hacking.html.in +++ b/docs/hacking.html.in @@ -1051,6 +1051,10 @@ BAD: <dd>The GLib macros g_autoptr and G_DEFINE_AUTOPTR_CLEANUP_FUNC should be used to manage autoclean of virObject classes. This matches usage with GObject classes.</dd> + + <dt>virStrerror</dt> + <dd>The GLib g_strerror() function should be used instead, + which has a simpler calling convention as an added benefit.</dd> </dl> <h2><a id="file_handling">File handling</a></h2> diff --git a/src/util/virerror.c b/src/util/virerror.c index 3bb9d1d32c..5d69e4e972 100644 --- a/src/util/virerror.c +++ b/src/util/virerror.c @@ -1322,8 +1322,11 @@ const char *virStrerror(int theerrno, char *errBuf, size_t errBufLen) { int save_errno = errno; const char *ret; + const char *str = g_strerror(theerrno); + size_t len = strlen(str); - strerror_r(theerrno, errBuf, errBufLen); + memcpy(errBuf, str, MIN(len, errBufLen)); + errBuf[errBufLen-1] = '\0'; ret = errBuf; errno = save_errno; return ret; @@ -1349,11 +1352,9 @@ void virReportSystemErrorFull(int domcode, const char *fmt, ...) { int save_errno = errno; - char strerror_buf[VIR_ERROR_MAX_LENGTH]; char msgDetailBuf[VIR_ERROR_MAX_LENGTH]; - const char *errnoDetail = virStrerror(theerrno, strerror_buf, - sizeof(strerror_buf)); + const char *errnoDetail = g_strerror(theerrno); const char *msg = virErrorMsg(VIR_ERR_SYSTEM_ERROR, fmt); const char *msgDetail = NULL; diff --git a/src/util/virerror.h b/src/util/virerror.h index fa88217b27..201195d660 100644 --- a/src/util/virerror.h +++ b/src/util/virerror.h @@ -193,6 +193,7 @@ void virReportOOMErrorFull(int domcode, int virSetError(virErrorPtr newerr); virErrorPtr virErrorCopyNew(virErrorPtr err); void virDispatchError(virConnectPtr conn); +/* DEPRECATED: use g_strerror() directly */ const char *virStrerror(int theerrno, char *errBuf, size_t errBufLen); typedef int (*virErrorLogPriorityFunc)(virErrorPtr, int); diff --git a/tests/commandtest.c b/tests/commandtest.c index c6fd826003..2aaddef3d1 100644 --- a/tests/commandtest.c +++ b/tests/commandtest.c @@ -636,12 +636,12 @@ static int test16(const void *unused ATTRIBUTE_UNUSED) } if ((fd = open(abs_builddir "/commandhelper.log", O_CREAT | O_TRUNC | O_WRONLY, 0600)) < 0) { - printf("Cannot open log file: %s\n", strerror(errno)); + printf("Cannot open log file: %s\n", g_strerror(errno)); goto cleanup; } virCommandWriteArgLog(cmd, fd); if (VIR_CLOSE(fd) < 0) { - printf("Cannot close log file: %s\n", strerror(errno)); + printf("Cannot close log file: %s\n", g_strerror(errno)); goto cleanup; } @@ -1116,12 +1116,12 @@ static int test26(const void *unused ATTRIBUTE_UNUSED) } if ((fd = open(abs_builddir "/commandhelper.log", O_CREAT | O_TRUNC | O_WRONLY, 0600)) < 0) { - printf("Cannot open log file: %s\n", strerror(errno)); + printf("Cannot open log file: %s\n", g_strerror(errno)); goto cleanup; } virCommandWriteArgLog(cmd, fd); if (VIR_CLOSE(fd) < 0) { - printf("Cannot close log file: %s\n", strerror(errno)); + printf("Cannot close log file: %s\n", g_strerror(errno)); goto cleanup; } @@ -1186,7 +1186,7 @@ static int test27(const void *unused ATTRIBUTE_UNUSED) } if (pipe(pipe1) < 0 || pipe(pipe2) < 0) { - printf("Could not create pipe: %s\n", strerror(errno)); + printf("Could not create pipe: %s\n", g_strerror(errno)); goto cleanup; } diff --git a/tests/qemumonitortestutils.c b/tests/qemumonitortestutils.c index c7580c5f28..9f2594b09a 100644 --- a/tests/qemumonitortestutils.c +++ b/tests/qemumonitortestutils.c @@ -417,7 +417,7 @@ qemuMonitorTestFree(qemuMonitorTestPtr test) VIR_FREE(test->items); if (test->tmpdir && rmdir(test->tmpdir) < 0) - VIR_WARN("Failed to remove tempdir: %s", strerror(errno)); + VIR_WARN("Failed to remove tempdir: %s", g_strerror(errno)); VIR_FREE(test->tmpdir); diff --git a/tests/seclabeltest.c b/tests/seclabeltest.c index 42dcb8c97f..105c25ea2d 100644 --- a/tests/seclabeltest.c +++ b/tests/seclabeltest.c @@ -20,14 +20,14 @@ mymain(void) model = virSecurityManagerGetModel(mgr); if (!model) { fprintf(stderr, "Failed to copy secModel model: %s", - strerror(errno)); + g_strerror(errno)); return EXIT_FAILURE; } doi = virSecurityManagerGetDOI(mgr); if (!doi) { fprintf(stderr, "Failed to copy secModel DOI: %s", - strerror(errno)); + g_strerror(errno)); return EXIT_FAILURE; } diff --git a/tests/testutils.c b/tests/testutils.c index 1b663f9d5d..1f3718b5cd 100644 --- a/tests/testutils.c +++ b/tests/testutils.c @@ -172,12 +172,12 @@ virTestLoadFile(const char *file, char **buf) int len, tmplen, buflen; if (!fp) { - fprintf(stderr, "%s: failed to open: %s\n", file, strerror(errno)); + fprintf(stderr, "%s: failed to open: %s\n", file, g_strerror(errno)); return -1; } if (fstat(fileno(fp), &st) < 0) { - fprintf(stderr, "%s: failed to fstat: %s\n", file, strerror(errno)); + fprintf(stderr, "%s: failed to fstat: %s\n", file, g_strerror(errno)); VIR_FORCE_FCLOSE(fp); return -1; } @@ -208,7 +208,7 @@ virTestLoadFile(const char *file, char **buf) tmplen -= len; } if (ferror(fp)) { - fprintf(stderr, "%s: read failed: %s\n", file, strerror(errno)); + fprintf(stderr, "%s: read failed: %s\n", file, g_strerror(errno)); VIR_FORCE_FCLOSE(fp); VIR_FREE(*buf); return -1; diff --git a/tests/virhostcputest.c b/tests/virhostcputest.c index 03ed3b1609..d9bdef701d 100644 --- a/tests/virhostcputest.c +++ b/tests/virhostcputest.c @@ -37,7 +37,7 @@ linuxTestCompareFiles(const char *cpuinfofile, cpuinfo = fopen(cpuinfofile, "r"); if (!cpuinfo) { fprintf(stderr, "unable to open: %s : %s\n", - cpuinfofile, strerror(errno)); + cpuinfofile, g_strerror(errno)); goto fail; } @@ -86,7 +86,7 @@ linuxCPUStatsToBuf(virBufferPtr buf, if ((sc_clk_tck = sysconf(_SC_CLK_TCK)) < 0) { fprintf(stderr, "sysconf(_SC_CLK_TCK) fails : %s\n", - strerror(errno)); + g_strerror(errno)); return -1; } tick_to_nsec = (1000ull * 1000ull * 1000ull) / sc_clk_tck; diff --git a/tests/virtestmock.c b/tests/virtestmock.c index df8cac6441..fa52667a2b 100644 --- a/tests/virtestmock.c +++ b/tests/virtestmock.c @@ -76,12 +76,12 @@ printFile(const char *file, } if (!(fp = real_fopen(output, "a"))) { - fprintf(stderr, "Unable to open %s: %s\n", output, strerror(errno)); + fprintf(stderr, "Unable to open %s: %s\n", output, g_strerror(errno)); abort(); } if (flock(fileno(fp), LOCK_EX) < 0) { - fprintf(stderr, "Unable to lock %s: %s\n", output, strerror(errno)); + fprintf(stderr, "Unable to lock %s: %s\n", output, g_strerror(errno)); fclose(fp); abort(); } -- 2.21.0

On Thu, Oct 10, 2019 at 11:54:12AM +0100, Daniel P. Berrangé wrote:
g_strerror is offers the safety/correctness benefits of strerror_r, with the API design convenience of strerror.
Use of virStrerror should be eliminated through the codebase in favour of g_strerror.
commandhelper.c is a special case as its a tiny single threaded test program, not linked to glib, so it just uses traditional strerror().
Signed-off-by: Daniel P. Berrangé <berrange@redhat.com> --- bootstrap.conf | 2 -- docs/hacking.html.in | 4 ++++ src/util/virerror.c | 9 +++++---- src/util/virerror.h | 1 + tests/commandtest.c | 10 +++++----- tests/qemumonitortestutils.c | 2 +- tests/seclabeltest.c | 4 ++-- tests/testutils.c | 6 +++--- tests/virhostcputest.c | 4 ++-- tests/virtestmock.c | 4 ++-- 10 files changed, 25 insertions(+), 21 deletions(-)
Reviewed-by: Ján Tomko <jtomko@redhat.com> Jano

The usleep function was missing on older mingw versions, but we can rely on it existing everywhere these days. It may only support times upto 1 second in duration though, so we'll prefer to use g_usleep instead. The commandhelper program is not changed since that can't link to glib. Fortunately it doesn't need to build on Windows platforms either. Signed-off-by: Daniel P. Berrangé <berrange@redhat.com> --- bootstrap.conf | 1 - src/hyperv/hyperv_driver.c | 2 +- src/hyperv/hyperv_wmi.c | 4 ++-- src/locking/lock_daemon.c | 2 +- src/locking/lock_driver_sanlock.c | 2 +- src/lxc/lxc_controller.c | 2 +- src/lxc/lxc_driver.c | 2 +- src/lxc/lxc_process.c | 2 +- src/network/bridge_driver.c | 2 +- src/nwfilter/nwfilter_dhcpsnoop.c | 4 ++-- src/nwfilter/nwfilter_learnipaddr.c | 2 +- src/qemu/qemu_monitor_json.c | 2 +- src/qemu/qemu_process.c | 2 +- src/qemu/qemu_tpm.c | 2 +- src/rpc/virnetsocket.c | 2 +- src/security/security_manager.c | 2 +- src/storage/storage_util.c | 4 ++-- src/util/vircgroup.c | 2 +- src/util/virfile.c | 2 +- src/util/virnetdev.c | 2 +- src/util/virnetdevip.c | 2 +- src/util/virnetdevmacvlan.c | 2 +- src/util/virnetdevvportprofile.c | 2 +- src/util/virpci.c | 8 ++++---- src/util/virprocess.c | 4 ++-- src/util/virtime.c | 2 +- src/vbox/vbox_common.c | 2 +- tests/commandtest.c | 6 +++--- tests/eventtest.c | 4 ++-- tests/fdstreamtest.c | 4 ++-- tools/virsh-domain.c | 2 +- 31 files changed, 41 insertions(+), 42 deletions(-) diff --git a/bootstrap.conf b/bootstrap.conf index 241dce50c2..1b5a68b873 100644 --- a/bootstrap.conf +++ b/bootstrap.conf @@ -90,7 +90,6 @@ timegm ttyname_r uname unsetenv -usleep verify vsnprintf waitpid diff --git a/src/hyperv/hyperv_driver.c b/src/hyperv/hyperv_driver.c index 0e2c6c55ef..ceaf528dd3 100644 --- a/src/hyperv/hyperv_driver.c +++ b/src/hyperv/hyperv_driver.c @@ -1415,7 +1415,7 @@ hypervDomainSendKey(virDomainPtr domain, unsigned int codeset, /* simulate holdtime by sleeping */ if (holdtime > 0) - usleep(holdtime * 1000); + g_usleep(holdtime * 1000); /* release the keys */ for (i = 0; i < nkeycodes; i++) { diff --git a/src/hyperv/hyperv_wmi.c b/src/hyperv/hyperv_wmi.c index 0f39bd4431..c2c1f082e1 100644 --- a/src/hyperv/hyperv_wmi.c +++ b/src/hyperv/hyperv_wmi.c @@ -909,7 +909,7 @@ hypervInvokeMethod(hypervPrivate *priv, hypervInvokeParamsListPtr params, case MSVM_CONCRETEJOB_JOBSTATE_SHUTTING_DOWN: hypervFreeObject(priv, (hypervObject *)job); job = NULL; - usleep(100 * 1000); /* sleep 100 ms */ + g_usleep(100 * 1000); /* sleep 100 ms */ timeout -= 100; continue; case MSVM_CONCRETEJOB_JOBSTATE_COMPLETED: @@ -1418,7 +1418,7 @@ hypervInvokeMsvmComputerSystemRequestStateChange(virDomainPtr domain, hypervFreeObject(priv, (hypervObject *)concreteJob); concreteJob = NULL; - usleep(100 * 1000); + g_usleep(100 * 1000); continue; case MSVM_CONCRETEJOB_JOBSTATE_COMPLETED: diff --git a/src/locking/lock_daemon.c b/src/locking/lock_daemon.c index a5a3a97e99..c12cb4ea0f 100644 --- a/src/locking/lock_daemon.c +++ b/src/locking/lock_daemon.c @@ -657,7 +657,7 @@ virLockDaemonClientFree(void *opaque) VIR_WARN("Failed to kill off pid %lld", (unsigned long long)priv->clientPid); } - usleep(200 * 1000); + g_usleep(200 * 1000); } } } diff --git a/src/locking/lock_driver_sanlock.c b/src/locking/lock_driver_sanlock.c index 85a23c7642..7ebd63913e 100644 --- a/src/locking/lock_driver_sanlock.c +++ b/src/locking/lock_driver_sanlock.c @@ -376,7 +376,7 @@ virLockManagerSanlockSetupLockspace(virLockManagerSanlockDriverPtr driver) #else /* fall back to polling */ VIR_DEBUG("Sleeping for %dms", LOCKSPACE_SLEEP); - usleep(LOCKSPACE_SLEEP * 1000); + g_usleep(LOCKSPACE_SLEEP * 1000); #endif VIR_DEBUG("Retrying to add lockspace (left %d)", retries); goto retry; diff --git a/src/lxc/lxc_controller.c b/src/lxc/lxc_controller.c index 37851bf284..9097655b4d 100644 --- a/src/lxc/lxc_controller.c +++ b/src/lxc/lxc_controller.c @@ -574,7 +574,7 @@ static int virLXCControllerAppendNBDPids(virLXCControllerPtr ctrl, while (!virFileExists(pidpath)) { /* wait for 100ms before checking again, but don't do it for ever */ if (errno == ENOENT && loops < 10) { - usleep(100 * 1000); + g_usleep(100 * 1000); loops++; } else { virReportSystemError(errno, diff --git a/src/lxc/lxc_driver.c b/src/lxc/lxc_driver.c index a69589e50c..c0bbeb09e8 100644 --- a/src/lxc/lxc_driver.c +++ b/src/lxc/lxc_driver.c @@ -2965,7 +2965,7 @@ static int lxcFreezeContainer(virDomainObjPtr vm) * decide that the freezing has been complete only with * the state actually transit to "FROZEN". */ - usleep(check_interval * 1000); + g_usleep(check_interval * 1000); r = virCgroupGetFreezerState(priv->cgroup, &state); diff --git a/src/lxc/lxc_process.c b/src/lxc/lxc_process.c index cbdc7b1268..318b4c1653 100644 --- a/src/lxc/lxc_process.c +++ b/src/lxc/lxc_process.c @@ -1095,7 +1095,7 @@ virLXCProcessReadLogOutputData(virDomainObjPtr vm, goto cleanup; } - usleep(100*1000); + g_usleep(100*1000); retries--; } diff --git a/src/network/bridge_driver.c b/src/network/bridge_driver.c index c05157c3ca..d10665efa3 100644 --- a/src/network/bridge_driver.c +++ b/src/network/bridge_driver.c @@ -1034,7 +1034,7 @@ networkKillDaemon(pid_t pid, * than modifications to domains, this seems a reasonable * tradeoff in exchange for less code disruption. */ - usleep(20 * 1000); + g_usleep(20 * 1000); } VIR_WARN("Timed out waiting after SIG%s to %s process %d " "(network '%s')", diff --git a/src/nwfilter/nwfilter_dhcpsnoop.c b/src/nwfilter/nwfilter_dhcpsnoop.c index 2af8cfdd32..c82457989e 100644 --- a/src/nwfilter/nwfilter_dhcpsnoop.c +++ b/src/nwfilter/nwfilter_dhcpsnoop.c @@ -1273,7 +1273,7 @@ virNWFilterSnoopRatePenalty(virNWFilterSnoopPcapConfPtr pc, unsigned long long now; if (virTimeMillisNowRaw(&now) < 0) { - usleep(PCAP_FLOOD_TIMEOUT_MS); /* 1 ms */ + g_usleep(PCAP_FLOOD_TIMEOUT_MS); /* 1 ms */ pc->penaltyTimeoutAbs = 0; } else { /* don't listen to the fd for 1 ms */ @@ -2010,7 +2010,7 @@ virNWFilterSnoopJoinThreads(void) while (virAtomicIntGet(&virNWFilterSnoopState.nThreads) != 0) { VIR_WARN("Waiting for snooping threads to terminate: %u", virAtomicIntGet(&virNWFilterSnoopState.nThreads)); - usleep(1000 * 1000); + g_usleep(1000 * 1000); } } diff --git a/src/nwfilter/nwfilter_learnipaddr.c b/src/nwfilter/nwfilter_learnipaddr.c index cd66e3ea7d..34b8d7fcd9 100644 --- a/src/nwfilter/nwfilter_learnipaddr.c +++ b/src/nwfilter/nwfilter_learnipaddr.c @@ -812,7 +812,7 @@ virNWFilterLearnThreadsTerminate(bool allowNewThreads) threadsTerminate = true; while (virHashSize(pendingLearnReq) != 0) - usleep((PKT_TIMEOUT_MS * 1000) / 3); + g_usleep((PKT_TIMEOUT_MS * 1000) / 3); if (allowNewThreads) threadsTerminate = false; diff --git a/src/qemu/qemu_monitor_json.c b/src/qemu/qemu_monitor_json.c index a6facdc09b..dbc32acb5e 100644 --- a/src/qemu/qemu_monitor_json.c +++ b/src/qemu/qemu_monitor_json.c @@ -1650,7 +1650,7 @@ qemuMonitorJSONStartCPUs(qemuMonitorPtr mon) virJSONValueFree(reply); reply = NULL; - usleep(250000); + g_usleep(250000); } while (++i <= timeout); virJSONValueFree(cmd); diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c index 3b45b2f641..c6fac01ada 100644 --- a/src/qemu/qemu_process.c +++ b/src/qemu/qemu_process.c @@ -7611,7 +7611,7 @@ void qemuProcessStop(virQEMUDriverPtr driver, retry: if ((ret = qemuRemoveCgroup(vm)) < 0) { if (ret == -EBUSY && (retries++ < 5)) { - usleep(200*1000); + g_usleep(200*1000); goto retry; } VIR_WARN("Failed to remove cgroup for %s", diff --git a/src/qemu/qemu_tpm.c b/src/qemu/qemu_tpm.c index 1d68803a28..31fdfa995f 100644 --- a/src/qemu/qemu_tpm.c +++ b/src/qemu/qemu_tpm.c @@ -884,7 +884,7 @@ qemuExtTPMStartEmulator(virQEMUDriverPtr driver, rc = qemuTPMEmulatorGetPid(cfg->swtpmStateDir, shortName, &pid); if (rc < 0) { timeout -= 50; - usleep(50 * 1000); + g_usleep(50 * 1000); continue; } if (rc == 0 && pid == (pid_t)-1) diff --git a/src/rpc/virnetsocket.c b/src/rpc/virnetsocket.c index a469907779..bd5fa96751 100644 --- a/src/rpc/virnetsocket.c +++ b/src/rpc/virnetsocket.c @@ -749,7 +749,7 @@ int virNetSocketNewConnectUNIX(const char *path, daemonLaunched = true; } - usleep(10000); + g_usleep(10000); } localAddr.len = sizeof(localAddr.data); diff --git a/src/security/security_manager.c b/src/security/security_manager.c index 7c905f0785..7f187c9068 100644 --- a/src/security/security_manager.c +++ b/src/security/security_manager.c @@ -1349,7 +1349,7 @@ virSecurityManagerMetadataLock(virSecurityManagerPtr mgr ATTRIBUTE_UNUSED, if (retries && (errno == EACCES || errno == EAGAIN)) { /* File is locked. Try again. */ retries--; - usleep(1000); + g_usleep(1000); continue; } else { virReportSystemError(errno, diff --git a/src/storage/storage_util.c b/src/storage/storage_util.c index 6165dd43b2..be084119f4 100644 --- a/src/storage/storage_util.c +++ b/src/storage/storage_util.c @@ -1940,7 +1940,7 @@ virStorageBackendStablePath(virStoragePoolObjPtr pool, if (virDirOpenQuiet(&dh, def->target.path) < 0) { opentries++; if (loop && errno == ENOENT && opentries < 50) { - usleep(100 * 1000); + g_usleep(100 * 1000); goto reopen; } virReportSystemError(errno, @@ -1975,7 +1975,7 @@ virStorageBackendStablePath(virStoragePoolObjPtr pool, } if (!direrr && loop && ++retry < 100) { - usleep(100 * 1000); + g_usleep(100 * 1000); goto retry; } diff --git a/src/util/vircgroup.c b/src/util/vircgroup.c index 4f9d80666d..70d41c4ba5 100644 --- a/src/util/vircgroup.c +++ b/src/util/vircgroup.c @@ -2656,7 +2656,7 @@ virCgroupKillPainfully(virCgroupPtr group) if (ret <= 0) break; - usleep(200 * 1000); + g_usleep(200 * 1000); } VIR_DEBUG("Complete %d", ret); return ret; diff --git a/src/util/virfile.c b/src/util/virfile.c index bb844c64e5..dead335c62 100644 --- a/src/util/virfile.c +++ b/src/util/virfile.c @@ -4387,7 +4387,7 @@ virFileWaitForExists(const char *path, if (tries == 0 || errno != ENOENT) return -1; - usleep(ms * 1000); + g_usleep(ms * 1000); } return 0; diff --git a/src/util/virnetdev.c b/src/util/virnetdev.c index 5c0e9723b9..5fef0b79aa 100644 --- a/src/util/virnetdev.c +++ b/src/util/virnetdev.c @@ -2304,7 +2304,7 @@ virNetDevSetNetConfig(const char *linkdev, int vf, * wait, then upcoming operations on the VF may fail. */ while (retries-- > 0 && !virNetDevExists(linkdev)) - usleep(1000); + g_usleep(1000); } if (pfDevOrig && setMACrc == 0) { diff --git a/src/util/virnetdevip.c b/src/util/virnetdevip.c index e2009fd829..e9d1ce0831 100644 --- a/src/util/virnetdevip.c +++ b/src/util/virnetdevip.c @@ -465,7 +465,7 @@ virNetDevIPWaitDadFinish(virSocketAddrPtr *addrs, size_t count) /* Parse response. */ dad = virNetDevIPParseDadStatus(resp, recvbuflen, addrs, count); if (dad) - usleep(1000 * 10); + g_usleep(1000 * 10); } /* Check timeout. */ if (dad) { diff --git a/src/util/virnetdevmacvlan.c b/src/util/virnetdevmacvlan.c index e8a9b052b6..efd81c3aa0 100644 --- a/src/util/virnetdevmacvlan.c +++ b/src/util/virnetdevmacvlan.c @@ -404,7 +404,7 @@ virNetDevMacVLanTapOpen(const char *ifname, tapfd[i] = fd; } else if (retries-- > 0) { /* may need to wait for udev to be done */ - usleep(20000); + g_usleep(20000); } else { /* However, if haven't succeeded, quit. */ virReportSystemError(errno, diff --git a/src/util/virnetdevvportprofile.c b/src/util/virnetdevvportprofile.c index a5ecd783f2..048f891920 100644 --- a/src/util/virnetdevvportprofile.c +++ b/src/util/virnetdevvportprofile.c @@ -999,7 +999,7 @@ virNetDevVPortProfileOpCommon(const char *ifname, int ifindex, break; } - usleep(STATUS_POLL_INTERVL_USEC); + g_usleep(STATUS_POLL_INTERVL_USEC); } if (status == PORT_PROFILE_RESPONSE_INPROGRESS) { diff --git a/src/util/virpci.c b/src/util/virpci.c index ee78151e74..9b828b7781 100644 --- a/src/util/virpci.c +++ b/src/util/virpci.c @@ -833,11 +833,11 @@ virPCIDeviceTrySecondaryBusReset(virPCIDevicePtr dev, virPCIDeviceWrite16(parent, parentfd, PCI_BRIDGE_CONTROL, ctl | PCI_BRIDGE_CTL_RESET); - usleep(200 * 1000); /* sleep 200ms */ + g_usleep(200 * 1000); /* sleep 200ms */ virPCIDeviceWrite16(parent, parentfd, PCI_BRIDGE_CONTROL, ctl); - usleep(200 * 1000); /* sleep 200ms */ + g_usleep(200 * 1000); /* sleep 200ms */ if (virPCIDeviceWrite(dev, cfgfd, 0, config_space, PCI_CONF_LEN) < 0) { virReportError(VIR_ERR_INTERNAL_ERROR, @@ -881,12 +881,12 @@ virPCIDeviceTryPowerManagementReset(virPCIDevicePtr dev, int cfgfd) virPCIDeviceWrite32(dev, cfgfd, dev->pci_pm_cap_pos + PCI_PM_CTRL, ctl | PCI_PM_CTRL_STATE_D3hot); - usleep(10 * 1000); /* sleep 10ms */ + g_usleep(10 * 1000); /* sleep 10ms */ virPCIDeviceWrite32(dev, cfgfd, dev->pci_pm_cap_pos + PCI_PM_CTRL, ctl | PCI_PM_CTRL_STATE_D0); - usleep(10 * 1000); /* sleep 10ms */ + g_usleep(10 * 1000); /* sleep 10ms */ if (virPCIDeviceWrite(dev, cfgfd, 0, &config_space[0], PCI_CONF_LEN) < 0) { virReportError(VIR_ERR_INTERNAL_ERROR, diff --git a/src/util/virprocess.c b/src/util/virprocess.c index 66834d37d3..b1544af730 100644 --- a/src/util/virprocess.c +++ b/src/util/virprocess.c @@ -177,7 +177,7 @@ virProcessAbort(pid_t pid) } else if (ret == 0) { VIR_DEBUG("trying SIGTERM to child process %d", pid); kill(pid, SIGTERM); - usleep(10 * 1000); + g_usleep(10 * 1000); while ((ret = waitpid(pid, &status, WNOHANG)) == -1 && errno == EINTR); if (ret == pid) { @@ -399,7 +399,7 @@ virProcessKillPainfullyDelay(pid_t pid, bool force, unsigned int extradelay) goto cleanup; /* process is dead */ } - usleep(200 * 1000); + g_usleep(200 * 1000); } virReportSystemError(EBUSY, diff --git a/src/util/virtime.c b/src/util/virtime.c index 86993e4e8d..6f978d7c64 100644 --- a/src/util/virtime.c +++ b/src/util/virtime.c @@ -448,6 +448,6 @@ virTimeBackOffWait(virTimeBackOffVar *var) VIR_DEBUG("sleeping for %llu ms", next); - usleep(next * 1000); + g_usleep(next * 1000); return 1; } diff --git a/src/vbox/vbox_common.c b/src/vbox/vbox_common.c index d3b8fb625f..748133933c 100644 --- a/src/vbox/vbox_common.c +++ b/src/vbox/vbox_common.c @@ -7932,7 +7932,7 @@ vboxDomainSendKey(virDomainPtr dom, /* since VBOX does not support holdtime, simulate it by sleeping and then sending the release key scancodes */ if (holdtime > 0) - usleep(holdtime * 1000); + g_usleep(holdtime * 1000); rc = gVBoxAPI.UIKeyboard.PutScancodes(keyboard, nkeycodes, keyUpCodes, &codesStored); diff --git a/tests/commandtest.c b/tests/commandtest.c index 2aaddef3d1..2ec3434f1e 100644 --- a/tests/commandtest.c +++ b/tests/commandtest.c @@ -262,7 +262,7 @@ static int test4(const void *unused ATTRIBUTE_UNUSED) goto cleanup; } while (kill(pid, 0) != -1) - usleep(100*1000); + g_usleep(100*1000); ret = checkoutput("test4", NULL); @@ -751,7 +751,7 @@ static int test18(const void *unused ATTRIBUTE_UNUSED) } while (kill(pid, SIGINT) != -1) - usleep(100*1000); + g_usleep(100*1000); ret = 0; @@ -1052,7 +1052,7 @@ static int test25(const void *unused ATTRIBUTE_UNUSED) goto cleanup; } - usleep(10 * 1000); + g_usleep(10 * 1000); } else { break; } diff --git a/tests/eventtest.c b/tests/eventtest.c index f426469d21..f04b416ad2 100644 --- a/tests/eventtest.c +++ b/tests/eventtest.c @@ -382,7 +382,7 @@ mymain(void) startJob(); pthread_mutex_unlock(&eventThreadMutex); sched_yield(); - usleep(100 * 1000); + g_usleep(100 * 1000); pthread_mutex_lock(&eventThreadMutex); virEventPollRemoveHandle(handles[1].watch); if (finishJob("Interrupted during poll", -1, -1) != EXIT_SUCCESS) @@ -448,7 +448,7 @@ mymain(void) startJob(); pthread_mutex_unlock(&eventThreadMutex); sched_yield(); - usleep(100 * 1000); + g_usleep(100 * 1000); pthread_mutex_lock(&eventThreadMutex); virEventPollRemoveTimeout(timers[1].timer); if (finishJob("Interrupted during poll", -1, -1) != EXIT_SUCCESS) diff --git a/tests/fdstreamtest.c b/tests/fdstreamtest.c index 054c405cec..98519e6266 100644 --- a/tests/fdstreamtest.c +++ b/tests/fdstreamtest.c @@ -101,7 +101,7 @@ static int testFDStreamReadCommon(const char *scratchdir, bool blocking) got = st->driver->streamRecv(st, buf + offset, want); if (got < 0) { if (got == -2 && !blocking) { - usleep(20 * 1000); + g_usleep(20 * 1000); goto reread; } virFilePrintf(stderr, "Failed to read stream: %s\n", @@ -222,7 +222,7 @@ static int testFDStreamWriteCommon(const char *scratchdir, bool blocking) got = st->driver->streamSend(st, pattern + offset, want); if (got < 0) { if (got == -2 && !blocking) { - usleep(20 * 1000); + g_usleep(20 * 1000); goto rewrite; } if (i == 9 && diff --git a/tools/virsh-domain.c b/tools/virsh-domain.c index fbfdc09c0d..c2e7c2b227 100644 --- a/tools/virsh-domain.c +++ b/tools/virsh-domain.c @@ -1945,7 +1945,7 @@ virshBlockJobWait(virshBlockJobWaitDataPtr data) break; } - usleep(500 * 1000); + g_usleep(500 * 1000); } /* print 100% completed */ -- 2.21.0

On Thu, Oct 10, 2019 at 11:54:13AM +0100, Daniel P. Berrangé wrote:
The usleep function was missing on older mingw versions, but we can rely on it existing everywhere these days. It may only support times upto 1 second in duration though, so we'll prefer to use g_usleep instead.
The commandhelper program is not changed since that can't link to glib. Fortunately it doesn't need to build on Windows platforms either.
Signed-off-by: Daniel P. Berrangé <berrange@redhat.com> --- bootstrap.conf | 1 - src/hyperv/hyperv_driver.c | 2 +- src/hyperv/hyperv_wmi.c | 4 ++-- src/locking/lock_daemon.c | 2 +- src/locking/lock_driver_sanlock.c | 2 +- src/lxc/lxc_controller.c | 2 +- src/lxc/lxc_driver.c | 2 +- src/lxc/lxc_process.c | 2 +- src/network/bridge_driver.c | 2 +- src/nwfilter/nwfilter_dhcpsnoop.c | 4 ++-- src/nwfilter/nwfilter_learnipaddr.c | 2 +- src/qemu/qemu_monitor_json.c | 2 +- src/qemu/qemu_process.c | 2 +- src/qemu/qemu_tpm.c | 2 +- src/rpc/virnetsocket.c | 2 +- src/security/security_manager.c | 2 +- src/storage/storage_util.c | 4 ++-- src/util/vircgroup.c | 2 +- src/util/virfile.c | 2 +- src/util/virnetdev.c | 2 +- src/util/virnetdevip.c | 2 +- src/util/virnetdevmacvlan.c | 2 +- src/util/virnetdevvportprofile.c | 2 +- src/util/virpci.c | 8 ++++---- src/util/virprocess.c | 4 ++-- src/util/virtime.c | 2 +- src/vbox/vbox_common.c | 2 +- tests/commandtest.c | 6 +++--- tests/eventtest.c | 4 ++-- tests/fdstreamtest.c | 4 ++-- tools/virsh-domain.c | 2 +- 31 files changed, 41 insertions(+), 42 deletions(-)
Reviewed-by: Ján Tomko <jtomko@redhat.com> Jano
participants (4)
-
Bjoern Walk
-
Daniel P. Berrangé
-
Han Han
-
Ján Tomko