1. Patchew
  2. Libvirt
  3. [libvirt] [PATCH 0/2] Couple of g_strdup_printf() fixes
  4. View patch

[libvirt] [PATCH 1/2] virstring: Reimplement g_strdup_printf() and g_strdup_vprintf()

Michal Privoznik posted 2 patches 6 years, 3 months ago
[libvirt] [PATCH 1/2] virstring: Reimplement g_strdup_printf() and g_strdup_vprintf()
Posted by Michal Privoznik 6 years, 3 months ago 
These functions don't really abort() on OOM. The fix was merged
upstream, but not in the minimal version we require. Provide our
own implementation which can be removed once we bump the minimal
version.

Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
---

Dan claims this is fixed upstream, but I'm failing to see any abort() in
current master:

https://gitlab.gnome.org/GNOME/glib/blob/master/glib/gprintf.c#L320

There is g_new() called, but it's done so only in one case out of three.
On my system, HAVE_VASPRINTF is defined meaning the function still won't
abort().

 src/libvirt_private.syms |  2 ++
 src/util/virstring.c     | 29 +++++++++++++++++++++++++++++
 src/util/virstring.h     | 11 +++++++++++
 3 files changed, 42 insertions(+)

diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms
index 0da02bb8bd..9eac489a32 100644
--- a/src/libvirt_private.syms
+++ b/src/libvirt_private.syms
@@ -3060,6 +3060,8 @@ virStorageFileBackendRegister;
 
 
 # util/virstring.h
+vir_g_strdup_printf;
+vir_g_strdup_vprintf;
 virAsprintfInternal;
 virSkipSpaces;
 virSkipSpacesAndBackslash;
diff --git a/src/util/virstring.c b/src/util/virstring.c
index 6453a23ada..fa7b15d0b7 100644
--- a/src/util/virstring.c
+++ b/src/util/virstring.c
@@ -768,6 +768,35 @@ virAsprintfInternal(char **strp,
     return ret;
 }
 
+
+/* Due to a bug in glib, g_strdup_printf() nor g_strdup_vprintf()
+ * abort on OOM.  It's fixed in glib's upstream. Provide our own
+ * implementation until the fix get's distributed. */
+char *
+vir_g_strdup_printf(const char *msg, ...)
+{
+  va_list args;
+  char *ret;
+  va_start(args, msg);
+  ret = g_strdup_vprintf(msg, args);
+  if (!ret)
+    abort();
+  va_end(args);
+  return ret;
+}
+
+
+char *
+vir_g_strdup_vprintf(const char *msg, va_list args)
+{
+  char *ret;
+  ret = g_strdup_vprintf(msg, args);
+  if (!ret)
+    abort();
+  return ret;
+}
+
+
 /**
  * virStrncpy:
  *
diff --git a/src/util/virstring.h b/src/util/virstring.h
index f5e2302b8b..b3a85b9ac2 100644
--- a/src/util/virstring.h
+++ b/src/util/virstring.h
@@ -253,6 +253,17 @@ size_t virStringListLength(const char * const *strings);
 
 #define virAsprintfQuiet(strp, ...) virAsprintf(strp, __VA_ARGS__)
 
+char *vir_g_strdup_printf(const char *msg, ...)
+    G_GNUC_PRINTF(1, 2);
+char *vir_g_strdup_vprintf(const char *msg, va_list args)
+    G_GNUC_PRINTF(1, 0);
+
+#if !GLIB_CHECK_VERSION(2, 64, 0)
+# define g_strdup_printf vir_g_strdup_printf
+# define g_strdup_vprintf vir_g_strdup_vprintf
+#endif
+
+
 int virStringSortCompare(const void *a, const void *b);
 int virStringSortRevCompare(const void *a, const void *b);
 int virStringToUpper(char **dst, const char *src);
-- 
2.21.0

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 1/2] virstring: Reimplement g_strdup_printf() and g_strdup_vprintf()
Posted by Peter Krempa 6 years, 3 months ago 
On Fri, Oct 18, 2019 at 10:16:49 +0200, Michal Privoznik wrote:
> These functions don't really abort() on OOM. The fix was merged
> upstream, but not in the minimal version we require. Provide our
> own implementation which can be removed once we bump the minimal
> version.
> 
> Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
> ---
> 
> Dan claims this is fixed upstream, but I'm failing to see any abort() in
> current master:
> 
> https://gitlab.gnome.org/GNOME/glib/blob/master/glib/gprintf.c#L320
> 
> There is g_new() called, but it's done so only in one case out of three.
> On my system, HAVE_VASPRINTF is defined meaning the function still won't
> abort().
> 
>  src/libvirt_private.syms |  2 ++
>  src/util/virstring.c     | 29 +++++++++++++++++++++++++++++
>  src/util/virstring.h     | 11 +++++++++++
>  3 files changed, 42 insertions(+)

[...]

> diff --git a/src/util/virstring.c b/src/util/virstring.c
> index 6453a23ada..fa7b15d0b7 100644
> --- a/src/util/virstring.c
> +++ b/src/util/virstring.c
> @@ -768,6 +768,35 @@ virAsprintfInternal(char **strp,
>      return ret;
>  }
>  
> +
> +/* Due to a bug in glib, g_strdup_printf() nor g_strdup_vprintf()
> + * abort on OOM.  It's fixed in glib's upstream. Provide our own
> + * implementation until the fix get's distributed. */
> +char *
> +vir_g_strdup_printf(const char *msg, ...)
> +{
> +  va_list args;
> +  char *ret;
> +  va_start(args, msg);
> +  ret = g_strdup_vprintf(msg, args);
> +  if (!ret)
> +    abort();
> +  va_end(args);
> +  return ret;
> +}
> +
> +
> +char *
> +vir_g_strdup_vprintf(const char *msg, va_list args)
> +{
> +  char *ret;
> +  ret = g_strdup_vprintf(msg, args);
> +  if (!ret)
> +    abort();
> +  return ret;
> +}

Please put this garbage into a separate file. This has nothing to do
with libvirt and should not pollute our own code.

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 1/2] virstring: Reimplement g_strdup_printf() and g_strdup_vprintf()
Posted by Daniel P. Berrangé 6 years, 3 months ago 
On Fri, Oct 18, 2019 at 10:38:27AM +0200, Peter Krempa wrote:
> On Fri, Oct 18, 2019 at 10:16:49 +0200, Michal Privoznik wrote:
> > These functions don't really abort() on OOM. The fix was merged
> > upstream, but not in the minimal version we require. Provide our
> > own implementation which can be removed once we bump the minimal
> > version.
> > 
> > Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
> > ---
> > 
> > Dan claims this is fixed upstream, but I'm failing to see any abort() in
> > current master:
> > 
> > https://gitlab.gnome.org/GNOME/glib/blob/master/glib/gprintf.c#L320
> > 
> > There is g_new() called, but it's done so only in one case out of three.
> > On my system, HAVE_VASPRINTF is defined meaning the function still won't
> > abort().
> > 
> >  src/libvirt_private.syms |  2 ++
> >  src/util/virstring.c     | 29 +++++++++++++++++++++++++++++
> >  src/util/virstring.h     | 11 +++++++++++
> >  3 files changed, 42 insertions(+)
> 
> [...]
> 
> > diff --git a/src/util/virstring.c b/src/util/virstring.c
> > index 6453a23ada..fa7b15d0b7 100644
> > --- a/src/util/virstring.c
> > +++ b/src/util/virstring.c
> > @@ -768,6 +768,35 @@ virAsprintfInternal(char **strp,
> >      return ret;
> >  }
> >  
> > +
> > +/* Due to a bug in glib, g_strdup_printf() nor g_strdup_vprintf()
> > + * abort on OOM.  It's fixed in glib's upstream. Provide our own
> > + * implementation until the fix get's distributed. */
> > +char *
> > +vir_g_strdup_printf(const char *msg, ...)
> > +{
> > +  va_list args;
> > +  char *ret;
> > +  va_start(args, msg);
> > +  ret = g_strdup_vprintf(msg, args);
> > +  if (!ret)
> > +    abort();
> > +  va_end(args);
> > +  return ret;
> > +}
> > +
> > +
> > +char *
> > +vir_g_strdup_vprintf(const char *msg, va_list args)
> > +{
> > +  char *ret;
> > +  ret = g_strdup_vprintf(msg, args);
> > +  if (!ret)
> > +    abort();
> > +  return ret;
> > +}
> 
> Please put this garbage into a separate file. This has nothing to do
> with libvirt and should not pollute our own code.

Calling this garbage is not really appropriate.

We should have a glibcompat.{c,h} file pair though, where the header
is #include from internal.h so it is guaranteed visible across the
whole codebase.

Regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 1/2] virstring: Reimplement g_strdup_printf() and g_strdup_vprintf()
Posted by Eric Blake 6 years, 3 months ago 
On 10/18/19 3:16 AM, Michal Privoznik wrote:
> These functions don't really abort() on OOM. The fix was merged
> upstream, but not in the minimal version we require. Provide our
> own implementation which can be removed once we bump the minimal
> version.
> 
> Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
> ---
> 

> +
> +/* Due to a bug in glib, g_strdup_printf() nor g_strdup_vprintf()

Missing 'neither' after the comma

> + * abort on OOM.  It's fixed in glib's upstream. Provide our own
> + * implementation until the fix get's distributed. */

s/get's/gets/

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3226
Virtualization:  qemu.org | libvirt.org

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Patchew 2.1-devel-b67e4c2

© 2016 - 2026 Red Hat, Inc.