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

[Daniel Henrique Barboza]

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 at gmail.com>
>>>> ---
>>>>   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