Re: [libvirt] [PATCH v4 5/5] qemu_driver: use g_strdup_printf

On Thu, Oct 17, 2019 at 03:30:26PM +0200, Jiri Denemark wrote: > On Thu, Oct 17, 2019 at 09:04:05 -0400, Cole Robinson wrote: >> On 10/16/19 4:54 PM, Daniel Henrique Barboza wrote: >>> This patch changes all virAsprintf calls to use the GLib API >>> g_strdup_printf in qemu_driver.c >>> >>> Signed-off-by: Daniel Henrique Barboza <danielhb413(a)gmail.com&gt; >>> --- >>> src/qemu/qemu_driver.c | 38 +++++++++++++++++--------------------- >>> 1 file changed, 17 insertions(+), 21 deletions(-) >>> >>> diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c >>> index a263393626..c9b3ed877f 100644 >>> --- a/src/qemu/qemu_driver.c >>> +++ b/src/qemu/qemu_driver.c >>> @@ -402,7 +402,7 @@ qemuDomainSnapshotLoad(virDomainObjPtr vm, >>> >>> priv = vm->privateData; >>> >>> - if (virAsprintf(&snapDir, "%s/%s", baseDir, vm->def->name) < 0) { >>> + if (!(snapDir = g_strdup_printf("%s/%s", baseDir, vm->def->name))) { >>> virReportError(VIR_ERR_INTERNAL_ERROR, >>> _("Failed to allocate memory for " >>> "snapshot directory for domain %s"), >>> @@ -427,7 +427,7 @@ qemuDomainSnapshotLoad(virDomainObjPtr vm, >>> kill the whole process */ >>> VIR_INFO("Loading snapshot file '%s'", entry->d_name); >>> >> No objection to this patch specifically, but if g_strdup_printf is a >> drop in replacement for virAsprintf, I think conversion should be done >> in a mass change-the-world series like Jano has done for other pieces. > It actually is not a drop in replacement. virAsprintf is a wrapper > around g_vasprintf and aborts on OOM. On the other hand g_strdup_printf > returns NULL and doesn't set any error. I think our documentation is > wrong in suggesting we should use g_strdup_printf directly and I believe > this patch should be reverted. g_strdup_printf was supposed to abort on OOM, but due to bug it does not do so on Linux at least. On Windows it will. This is fixed in git master glib. I think it is nice to use g_strdup_printf directly, so we should create transparent fix in the same way gnulib fixes things like this: #if !GLIB_CHECK_VERSION(2, 64, 0) static inline char *vir_g_strdup_printf(const char *msg, ...) { va_list args; char *ret; va_start(msg, args); ret = g_stdup_vprintf(msg, args); if (!ret) abort(); va_end(args); return ret } static inline char *vir_g_strdup_vprintf(const char *msg, va_list args) { char *ret; ret = g_stdup_vprintf(msg, args); if (!ret) abort(); return ret } # define g_strdup_vprintf vir_g_strdup_vprintf #endif (untested code) We can then just drop this compat when we set min glib to 2.64 in future and not have to update all the callers.

Regards. Daniel