On 10/17/19 10:51 AM, Daniel P. Berrangé wrote:
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@xxxxxxxxx>
---
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.
Guess I can play around with changing all the virAsprintf callers
to g_strdup_printf then. I'll assume that this wrapper will be already
in place when the patches hit upstream.
In fact, given that there are g_strdup_printf calls floating around
in qemu_driver.c already, I believe this vir_g_strdup_printf proposed
here must be pushed as soon as possible.
Thanks,
DHB
Regards.
Daniel
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list
