[PATCH] Introduce xasprintf

[PATCH] Introduce xasprintf

[Colin Watson]

Ubuntu builds with -D_FORTIFY_SOURCE=2 by default, and thus produces a
bunch of warnings like this:

  util/grub-mkfont.c: In function ‘write_font’:
  util/grub-mkfont.c:366: warning: ignoring return value of ‘asprintf’, declared with attribute warn_unused_result

I came across the idea of an xasprintf function in Gnulib a while back.
Using this both fixes the above class of warnings by always checking the
return value of asprintf, and also provides an IMO more intuitive
interface to formatting an allocated string. Do people like the
following patch? The warnings are minor so there's no problem with this
being post-1.97.

Index: configure.ac
===================================================================
--- configure.ac	(revision 2557)
+++ configure.ac	(working copy)
@@ -172,7 +172,7 @@
 fi
 
 # Check for functions.
-AC_CHECK_FUNCS(posix_memalign memalign asprintf)
+AC_CHECK_FUNCS(posix_memalign memalign asprintf vasprintf)
 
 #
 # Check for target programs.
Index: include/grub/util/misc.h
===================================================================
--- include/grub/util/misc.h	(revision 2557)
+++ include/grub/util/misc.h	(working copy)
@@ -21,6 +21,7 @@
 
 #include <stdlib.h>
 #include <stdio.h>
+#include <stdarg.h>
 #include <setjmp.h>
 #include <unistd.h>
 
@@ -57,12 +58,20 @@
 void grub_util_write_image_at (const void *img, size_t size, off_t offset,
 			       FILE *out);
 
+#ifndef HAVE_VASPRINTF
+
+int vasprintf (char **buf, const char *fmt, va_list ap);
+
+#endif
+
 #ifndef  HAVE_ASPRINTF
 
 int asprintf (char **buf, const char *fmt, ...);
 
 #endif
 
+char *xasprintf (const char *fmt, ...);
+
 #ifdef __MINGW32__
 
 #define fseeko fseeko64
Index: util/grub-probe.c
===================================================================
--- util/grub-probe.c	(revision 2558)
+++ util/grub-probe.c	(working copy)
@@ -248,7 +248,7 @@
 	  filebuf_via_sys = grub_util_read_image (path);
 
 	  grub_util_info ("reading %s via GRUB facilities", path);
-	  asprintf (&grub_path, "(%s)%s", drive_name, path);
+	  grub_path = xasprintf ("(%s)%s", drive_name, path);
 	  file = grub_file_open (grub_path);
 	  filebuf_via_grub = xmalloc (file->size);
 	  grub_file_read (file, filebuf_via_grub, file->size);
Index: util/getroot.c
===================================================================
--- util/getroot.c	(revision 2557)
+++ util/getroot.c	(working copy)
@@ -454,7 +454,7 @@
 	  if (q)
 	    *q = ',';
 
-	  asprintf (&grub_dev, "md%s", p);
+	  grub_dev = xasprintf ("md%s", p);
 	  free (p);
 	}
       else if (os_dev[7] == '/' && os_dev[8] == 'd')
@@ -469,7 +469,7 @@
 	  if (q)
 	    *q = ',';
 
-	  asprintf (&grub_dev, "md%s", p);
+	  grub_dev = xasprintf ("md%s", p);
 	  free (p);
 	}
       else if (os_dev[7] >= '0' && os_dev[7] <= '9')
@@ -482,7 +482,7 @@
 	  if (q)
 	    *q = ',';
 
-	  asprintf (&grub_dev, "md%s", p);
+	  grub_dev = xasprintf ("md%s", p);
 	  free (p);
 	}
       else if (os_dev[7] == '/' && os_dev[8] >= '0' && os_dev[8] <= '9')
@@ -495,7 +495,7 @@
 	  if (q)
 	    *q = ',';
 
-	  asprintf (&grub_dev, "md%s", p);
+	  grub_dev = xasprintf ("md%s", p);
 	  free (p);
 	}
       else
Index: util/grub-mkfont.c
===================================================================
--- util/grub-mkfont.c	(revision 2557)
+++ util/grub-mkfont.c	(working copy)
@@ -363,8 +363,8 @@
   if (! style_name[0])
     strcpy (style_name, " Regular");
 
-  asprintf (&font_name, "%s %s %d", font_info->name, &style_name[1],
-	    font_info->size);
+  font_name = xasprintf ("%s %s %d", font_info->name, &style_name[1],
+			 font_info->size);
 
   write_string_section ("NAME", font_name, &offset, file);
   write_string_section ("FAMI", font_info->name, &offset, file);
Index: util/misc.c
===================================================================
--- util/misc.c	(revision 2557)
+++ util/misc.c	(working copy)
@@ -28,6 +28,7 @@
 #include <sys/time.h>
 #include <unistd.h>
 #include <time.h>
+#include <errno.h>
 
 #include <grub/kernel.h>
 #include <grub/misc.h>
@@ -369,6 +370,19 @@
 {
 }
 
+#ifndef HAVE_VASPRINTF
+
+int
+vasprintf (char **buf, const char *fmt, va_list ap)
+{
+  /* Should be large enough.  */
+  *buf = xmalloc (512);
+
+  return vsprintf (*buf, fmt, ap);
+}
+
+#endif
+
 #ifndef  HAVE_ASPRINTF
 
 int
@@ -377,11 +391,8 @@
   int status;
   va_list ap;
 
-  /* Should be large enough.  */
-  *buf = xmalloc (512);
-
   va_start (ap, fmt);
-  status = vsprintf (*buf, fmt, ap);
+  status = vasprintf (*buf, fmt, ap);
   va_end (ap);
 
   return status;
@@ -389,6 +400,23 @@
 
 #endif
 
+char *
+xasprintf (const char *fmt, ...)
+{
+  va_list ap;
+  char *result;
+
+  va_start (ap, fmt);
+  if (vasprintf (&result, fmt, ap) < 0)
+    {
+      if (errno == ENOMEM)
+	grub_util_error ("out of memory");
+      return NULL;
+    }
+
+  return result;
+}
+
 #ifdef __MINGW32__
 
 void sync (void)

Thanks,

-- 
Colin Watson                                       [cjwatson@ubuntu.com]

Re: [PATCH] Introduce xasprintf

[Colin Watson]

On Wed, Sep 02, 2009 at 02:49:39AM +0100, Colin Watson wrote:
> I came across the idea of an xasprintf function in Gnulib a while back.
> Using this both fixes the above class of warnings by always checking the
> return value of asprintf, and also provides an IMO more intuitive
> interface to formatting an allocated string. Do people like the
> following patch? The warnings are minor so there's no problem with this
> being post-1.97.

Sorry, I forgot to supply a ChangeLog entry for this.

2009-09-02  Colin Watson  <cjwatson@ubuntu.com>

	* configure.ac: Check for vasprintf.
	* util/misc.c (asprintf): Move allocation from here ...
	(vasprintf): ... to here.  New function.
	(xasprintf): New function.
	* include/grub/util/misc.h (vasprintf, xasprintf): Add
	prototypes.
	* util/getroot.c (grub_util_get_grub_dev): Use xasprintf.
	* util/grub-mkfont.c (write_font): Likewise.
	* util/grub-probe.c (probe): Likewise.

-- 
Colin Watson                                       [cjwatson@ubuntu.com]

Re: [PATCH] Introduce xasprintf

[Neal H. Walfield]

At Wed, 2 Sep 2009 02:49:39 +0100,
Colin Watson wrote:
> +#ifndef HAVE_VASPRINTF
> +
> +int
> +vasprintf (char **buf, const char *fmt, va_list ap)
> +{
> +  /* Should be large enough.  */
> +  *buf = xmalloc (512);
> +
> +  return vsprintf (*buf, fmt, ap);
> +}
> +
> +#endif

Perhaps check that the number of characters is not more than 512 (if
so, panic).

Neal

Re: [PATCH] Introduce xasprintf

[Colin Watson]

On Fri, Sep 04, 2009 at 06:47:17PM +0200, Neal H. Walfield wrote:
> At Wed, 2 Sep 2009 02:49:39 +0100,
> Colin Watson wrote:
> > +#ifndef HAVE_VASPRINTF
> > +
> > +int
> > +vasprintf (char **buf, const char *fmt, va_list ap)
> > +{
> > +  /* Should be large enough.  */
> > +  *buf = xmalloc (512);
> > +
> > +  return vsprintf (*buf, fmt, ap);
> > +}
> > +
> > +#endif
> 
> Perhaps check that the number of characters is not more than 512 (if
> so, panic).

I agree it's suboptimal, but I'm not sure this is possible without
shipping our own vsprintf implementation, which I would like to avoid
(gnulib does this properly, but it's much larger). Still, note that my
patch didn't really add that xmalloc code - all I did was move it from
the asprintf implementation - so this can be dealt with in a further
change if we choose.

I've gone ahead and committed this, since it's now well past 1.97 and I
think this is a clear improvement.

Thanks,

-- 
Colin Watson                                       [cjwatson@ubuntu.com]

Re: [PATCH] Introduce xasprintf

[Robert Millan]

On Mon, Dec 07, 2009 at 04:48:24PM +0000, Colin Watson wrote:
> 
> I agree it's suboptimal, but I'm not sure this is possible without
> shipping our own vsprintf implementation, which I would like to avoid
> (gnulib does this properly, but it's much larger).

For util/ code size is not such a big deal.  Maintainability is much more
important.

If you need more Gnulib modules added, just let me know;  there's no problem
with importing more of it.

-- 
Robert Millan

  The DRM opt-in fallacy: "Your data belongs to us. We will decide when (and
  how) you may access your data; but nobody's threatening your freedom: we
  still allow you to remove your data and not access it at all."