On 06/04/2010 02:09 PM, Colin Watson wrote:
> On Thu, Jun 03, 2010 at 09:27:38PM +0200, sean finney wrote:
>   
>> while investigated an unrelated bug, i noticed that the latest snapshot
>> uploaded to debian fails with the following error:
>>
>> gcc-4.4 -Idisk -I/home/seanius/grub2-1.98+20100602/disk -I/home/seanius/grub2-1.98+20100602/include -I. -I./include -Wall -W  -ffreestanding  -Os -DGRUB_MACHINE_EMU=1 -DMACHINE=X86_64_EMU -Wall -W -Wshadow -Wpointer-arith -Wmissing-prototypes                -Wundef -Wstrict-prototypes -g -fno-dwarf2-cfi-asm -m64 -fno-stack-protector -mno-stack-arg-probe -Werror -DGRUB_TARGET_NO_MODULES=1 -DUSE_ASCII_FAILBACK=1  -DGRUB_FILE=\"disk/usbms.c\" -MD -c -o usbms_mod-disk_usbms.o /home/seanius/grub2-1.98+20100602/disk/usbms.c
>> cc1: warnings being treated as errors
>> /home/seanius/grub2-1.98+20100602/disk/usbms.c: In function ‘grub_usbms_transfer’:
>> /home/seanius/grub2-1.98+20100602/disk/usbms.c:315: error: format ‘%02x’ expects type ‘unsigned int’, but argument 5 has type ‘grub_size_t’
>> /home/seanius/grub2-1.98+20100602/disk/usbms.c:333: error: format ‘%02x’ expects type ‘unsigned int’, but argument 5 has type ‘grub_size_t’
>> make[1]: *** [usbms_mod-disk_usbms.o] Error 1
>>
>> the two lines in question are both debug printf type statements.
>> therefore, instead of doing something complicated to get the format
>> string to match with the size of grub_size_t (which i'm guessing could
>> vary based on the platform), i have created a patch that simply casts the
>> parameter in question as unsigned to match the format string instead (which
>> only shows two digits anyway).
>>     
> As Vladimir pointed out on IRC: no, that's a minimum field width, not a
> precision.  It shows *at least* two digits.
>
> I think I'd prefer to add a grub_printf length modifier to print
> grub_size_t values.  'z' is standard for size_t, so it seems reasonable
> to use that, it can be done in very little code, and it saves on ugly
> and inaccurate casts.  Vladimir, what do you think?
>
>   
grub_size_t is always of the same length as long. Perhaps we should
define it as unsigned long, use "%lx" and get rid of useless difference
on 32-bit and 64-bit platforms.
Are there any reasons not to do this way?
Alternatively %zx can be aliased to %lx in one code line.
> (If this is unacceptable for whatever reason, then the proper fix would
> be to cast to (unsigned long long) and to use %02llx - but I prefer
> having an accurate length modifier, really.)
>
> 2010-06-04  Colin Watson  <cjwatson@ubuntu.com>
>
> 	* kern/misc.c (grub_vsnprintf_real): Support 'z' length
> 	modifier, for grub_size_t.
> 	* disk/usbms.c (grub_usbms_transfer): Use it.
>
> === modified file 'disk/usbms.c'
> --- disk/usbms.c	2010-06-01 00:10:19 +0000
> +++ disk/usbms.c	2010-06-04 12:04:11 +0000
> @@ -312,7 +312,7 @@ grub_usbms_transfer (struct grub_scsi *s
>        grub_dprintf ("usb", "buf:\n");
>        if (size <= 64)
>          for (i=0; i<size; i++)
> -          grub_dprintf ("usb", "0x%02x: 0x%02x\n", i, buf[i]);
> +          grub_dprintf ("usb", "0x%02zx: 0x%02x\n", i, buf[i]);
>        else
>            grub_dprintf ("usb", "Too much data for debug print...\n");
>      }
> @@ -330,7 +330,7 @@ grub_usbms_transfer (struct grub_scsi *s
>        /* Debug print of sent data. */
>        if (size <= 256)
>          for (i=0; i<size; i++)
> -          grub_dprintf ("usb", "0x%02x: 0x%02x\n", i, buf[i]);
> +          grub_dprintf ("usb", "0x%02zx: 0x%02x\n", i, buf[i]);
>        else
>            grub_dprintf ("usb", "Too much data for debug print...\n");
>      }
>
> === modified file 'kern/misc.c'
> --- kern/misc.c	2010-05-28 13:48:45 +0000
> +++ kern/misc.c	2010-06-04 12:04:11 +0000
> @@ -688,6 +688,7 @@ grub_vsnprintf_real (char *str, grub_siz
>  	  int n;
>  	  int longfmt = 0;
>  	  int longlongfmt = 0;
> +	  int sizetfmt = 0;
>  	  int unsig = 0;
>  
>  	  if (*fmt && *fmt =='-')
> @@ -740,6 +741,11 @@ grub_vsnprintf_real (char *str, grub_siz
>  		  c = *fmt++;
>  		}
>  	    }
> +	  else if (c == 'z')
> +	    {
> +	      sizetfmt = 1;
> +	      c = *fmt++;
> +	    }
>  
>  	  switch (c)
>  	    {
> @@ -770,6 +776,11 @@ grub_vsnprintf_real (char *str, grub_siz
>  		  long l = va_arg (args, long);
>  		  grub_lltoa (tmp, c, l);
>  		}
> +	      else if (sizetfmt)
> +		{
> +		  grub_size_t sz = va_arg (args, grub_size_t);
> +		  grub_lltoa (tmp, c, sz);
> +		}
>  	      else if (unsig)
>  		{
>  		  unsigned u = va_arg (args, unsigned);
>
> Thanks,
>
>   


-- 
Regards
Vladimir 'φ-coder/phcoder' Serbinenko