Not reproduced when build source before `make clean`. Please ignore that issue
On Tue, Oct 15, 2019 at 10:20 AM Han Han <h...@redhat.com> wrote: > > > On Thu, Oct 10, 2019 at 6:54 PM Daniel P. Berrangé <berra...@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 <jto...@redhat.com> >> Signed-off-by: Daniel P. Berrangé <berra...@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: h...@redhat.com > Phone: +861065339333 > -- Best regards, ----------------------------------- Han Han Quality Engineer Redhat. Email: h...@redhat.com Phone: +861065339333
-- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list