Re: [PATCH] Support * as field width or precision in printf

Re: [PATCH] Support * as field width or precision in printf

[Josh Triplett]

Just following up on these patches.  Any obstacles to merging them?
Anything I need to fix in them?

Thanks,
Josh Triplett

[PATCH] Support * as field width or precision in printf

[Josh Triplett]

[-- Attachment #1: Type: text/plain, Size: 1657 bytes --]

The attached patch implements support for * as the field width or
precision in printf, which obtains that field width or precision from
the next va_arg. This proves particularly useful as a modifier on %s:
%.*s allows printing a non-NUL-terminated string with a known length.

Tested with the following test cases, all of which worked:

grub_printf("|%*s|\n", -50, "left-aligned in 50");
grub_printf("|%*s|\n", 50, "right-aligned in 50");
grub_printf("|%.*s|\n", 5, "truncated");
grub_printf("|%.*s|\n", -5, "truncated");
grub_printf("|%*.*s|\n", 20, 5, "truncated");
grub_printf("|%*.*s|\n", -20, 5, "truncated");
grub_printf("|%50s|\n", "hardcoded 50");
grub_printf("|%-50s|\n", "hardcoded -50");
grub_printf("|%.5s|\n", "truncated");

ChangeLog entry:

2011-04-21  Josh Triplett  <josh@joshtriplett.org>
2011-04-21  Burt Triplett  <burt@pbjtriplett.org>

        * grub-core/kern/misc.c (grub_vsnprintf_real): Add support for '*'
        as field width or precision.


In bug 33144, phcoder requested justification for the inclusion of this
feature in grub's printf.  We added this feature to support our port of
ACPICA that runs as a GRUB module.  ACPICA depends on having that
particular feature in printf; it uses %*s extensively.  Now that we've
added this feature, we can also use it to simplify many other pieces of
code we've written.

Also, I just checked, and this patch adds a grand total of 8 bytes to
kernel.img.  If it helps, I've also attached a second patch to current
GRUB, which tweaks the printf implementation to save 72 bytes by
factoring out common code paths.  Given that, could we please have 8 of
those bytes? :)

- Josh Triplett

[-- Attachment #2: grub-printf-star.patch --]
[-- Type: text/x-diff, Size: 2002 bytes --]

=== modified file 'grub-core/kern/misc.c'
--- grub-core/kern/misc.c	2011-04-18 21:03:52 +0000
+++ grub-core/kern/misc.c	2011-04-22 07:13:52 +0000
@@ -726,45 +726,70 @@
 	  int longlongfmt = 0;
 	  int unsig = 0;
 
-	  if (*fmt && *fmt =='-')
-	    {
-	      rightfill = 1;
-	      fmt++;
-	    }
-
-	  p = (char *) fmt;
-	  /* Read formatting parameters.  */
-	  while (*p && grub_isdigit (*p))
-	    p++;
-
-	  if (p > fmt)
-	    {
-	      char s[p - fmt + 1];
-	      grub_strncpy (s, fmt, p - fmt);
-	      s[p - fmt] = 0;
-	      if (s[0] == '0')
-		zerofill = '0';
-	      format1 = grub_strtoul (s, 0, 10);
-	      fmt = p;
-	    }
-
-	  if (*p && *p == '.')
-	    {
-	      p++;
-	      fmt++;
+	  if (*fmt && *fmt == '*')
+	    {
+	      fmt++;
+	      n = va_arg (args, int);
+	      if (n < 0)
+		{
+		  rightfill = 1;
+		  n = -n;
+		}
+	      format1 = n;
+	    }
+	  else
+	    {
+	      if (*fmt && *fmt =='-')
+		{
+		  rightfill = 1;
+		  fmt++;
+		}
+
+	      p = (char *) fmt;
+	      /* Read formatting parameters.  */
 	      while (*p && grub_isdigit (*p))
 		p++;
 
 	      if (p > fmt)
 		{
-		  char fstr[p - fmt + 1];
-		  grub_strncpy (fstr, fmt, p - fmt);
-		  fstr[p - fmt] = 0;
-		  format2 = grub_strtoul (fstr, 0, 10);
+		  char s[p - fmt + 1];
+		  grub_strncpy (s, fmt, p - fmt);
+		  s[p - fmt] = 0;
+		  if (s[0] == '0')
+		    zerofill = '0';
+		  format1 = grub_strtoul (s, 0, 10);
 		  fmt = p;
 		}
 	    }
 
+	  if (*fmt && *fmt == '.')
+	    {
+	      fmt++;
+	      if (*fmt && *fmt == '*')
+		{
+                  fmt++;
+		  n = va_arg (args, int);
+		  if (n < 0)
+		    n = 0;
+		  format2 = n;
+		}
+	      else
+		{
+		  p = (char *) fmt;
+		  while (*p && grub_isdigit (*p))
+		    p++;
+
+		  if (p > fmt)
+		    {
+		      char fstr[p - fmt + 1];
+		      grub_strncpy (fstr, fmt, p - fmt);
+		      fstr[p - fmt] = 0;
+		      format2 = grub_strtoul (fstr, 0, 10);
+		      fmt = p;
+		    }
+		}
+	    }
+
 	  c = *fmt++;
 	  if (c == 'l')
 	    {


[-- Attachment #3: grub-microoptimize-printf-to-save-72-bytes.patch --]
[-- Type: text/x-diff, Size: 1843 bytes --]

=== modified file 'grub-core/kern/misc.c'
--- grub-core/kern/misc.c	2011-04-18 21:03:52 +0000
+++ grub-core/kern/misc.c	2011-04-22 21:38:31 +0000
@@ -750,10 +750,10 @@
 
 	  if (*p && *p == '.')
 	    {
-	      p++;
 	      fmt++;
-	      while (*p && grub_isdigit (*p))
+	      do
 		p++;
+	      while (*p && grub_isdigit (*p));
 
 	      if (p > fmt)
 		{
@@ -789,39 +789,26 @@
 	      unsig = 1;
 	      /* Fall through. */
 	    case 'd':
-	      if (longlongfmt)
-		{
-		  long long ll;
-
+	      {
+		unsigned long long ll;
+		if (longlongfmt)
 		  ll = va_arg (args, long long);
-		  grub_lltoa (tmp, c, ll);
-		}
-	      else if (longfmt && unsig)
-		{
-		  unsigned long l = va_arg (args, unsigned long);
-		  grub_lltoa (tmp, c, l);
-		}
-	      else if (longfmt)
-		{
-		  long l = va_arg (args, long);
-		  grub_lltoa (tmp, c, l);
-		}
-	      else if (unsig)
-		{
-		  unsigned u = va_arg (args, unsigned);
-		  grub_lltoa (tmp, c, u);
-		}
-	      else
-		{
-		  n = va_arg (args, int);
-		  grub_lltoa (tmp, c, n);
-		}
-	      if (! rightfill && grub_strlen (tmp) < format1)
-		write_fill (zerofill, format1 - grub_strlen (tmp));
-	      write_str (tmp);
-	      if (rightfill && grub_strlen (tmp) < format1)
-		write_fill (zerofill, format1 - grub_strlen (tmp));
-	      break;
+		else if (longfmt && unsig)
+		  ll = va_arg (args, unsigned long);
+		else if (longfmt)
+		  ll = va_arg (args, long);
+		else if (unsig)
+		  ll = va_arg (args, unsigned);
+		else
+		  ll = va_arg (args, int);
+		grub_lltoa (tmp, c, ll);
+		if (! rightfill && grub_strlen (tmp) < format1)
+		  write_fill (zerofill, format1 - grub_strlen (tmp));
+		write_str (tmp);
+		if (rightfill && grub_strlen (tmp) < format1)
+		  write_fill (zerofill, format1 - grub_strlen (tmp));
+		break;
+	      }
 
 	    case 'c':
 	      n = va_arg (args, int);