[libvirt] [PATCH v3 03/19] util: use glib memory allocation functions
Han Han
hhan at redhat.com
Tue Oct 15 06:17:14 UTC 2019
Not reproduced when build source before `make clean`. Please ignore that
issue
On Tue, Oct 15, 2019 at 10:20 AM Han Han <hhan at redhat.com> wrote:
>
>
> On Thu, Oct 10, 2019 at 6:54 PM Daniel P. Berrangé <berrange at 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 at redhat.com>
>> Signed-off-by: Daniel P. Berrangé <berrange at 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 at 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 at entry=do_abort,
> fmt=fmt at entry=0x7fc3be61670c "%s\n") at ../sysdeps/posix/libc_fatal.c:181
> #3 0x00007fc3be51053c in malloc_printerr (str=str at 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 at entry=0x7ffe56ef7d88) at
> util/viralloc.c:348
> #7 0x00007fc3c26a1270 in virSystemdActivationFree (act=<optimized out>,
> act at 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 at redhat.com
>> https://www.redhat.com/mailman/listinfo/libvir-list
>
>
>
> --
> Best regards,
> -----------------------------------
> Han Han
> Quality Engineer
> Redhat.
>
> Email: hhan at redhat.com
> Phone: +861065339333
>
--
Best regards,
-----------------------------------
Han Han
Quality Engineer
Redhat.
Email: hhan at redhat.com
Phone: +861065339333
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://listman.redhat.com/archives/libvir-list/attachments/20191015/716cdbc9/attachment-0001.htm>
More information about the libvir-list
mailing list