Just a cosmetic question about grub_vprintf()?

Just a cosmetic question about grub_vprintf()?

[rubisher]

Hello all,

I am just reading kernel/misc.c file and read grub_vsprintf() definition as:
int
grub_vsprintf (char *str, const char *fmt, va_list args)

which is used by:
int
grub_vprintf (const char *fmt, va_list args)
{
   int ret;

   ret = grub_vsprintf (0, fmt, args);
   return ret;
}

But as far as the 1st parameter of grub_vsprintf is a pointer,
wouldn't it be better to write:
--- kern/misc.c.orig	2009-11-22 13:07:22.000000000 +0000
+++ kern/misc.c	2009-11-22 13:07:51.000000000 +0000
@@ -160,7 +160,7 @@
  {
    int ret;

-  ret = grub_vsprintf (0, fmt, args);
+  ret = grub_vsprintf (NULL, fmt, args);
    return ret;
  }

Tia,
	J.

Re: Just a cosmetic question about grub_vprintf()?

[Robert Millan]

On Sun, Nov 22, 2009 at 01:10:11PM +0000, rubisher wrote:
> But as far as the 1st parameter of grub_vsprintf is a pointer,
> wouldn't it be better to write:
> --- kern/misc.c.orig	2009-11-22 13:07:22.000000000 +0000
> +++ kern/misc.c	2009-11-22 13:07:51.000000000 +0000
> @@ -160,7 +160,7 @@
>  {
>    int ret;
>
> -  ret = grub_vsprintf (0, fmt, args);
> +  ret = grub_vsprintf (NULL, fmt, args);
>    return ret;
>  }

Yes.  But we have many of those, so we don't go huntin' them.  If you'd
like to help us, a patch that does this change in bulk would be welcome.

Thanks

-- 
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."

Re: Just a cosmetic question about grub_vprintf()?

[rubisher]

Robert Millan wrote:
> On Sun, Nov 22, 2009 at 01:10:11PM +0000, rubisher wrote:
>> But as far as the 1st parameter of grub_vsprintf is a pointer,
>> wouldn't it be better to write:
>> --- kern/misc.c.orig	2009-11-22 13:07:22.000000000 +0000
>> +++ kern/misc.c	2009-11-22 13:07:51.000000000 +0000
>> @@ -160,7 +160,7 @@
>>  {
>>    int ret;
>>
>> -  ret = grub_vsprintf (0, fmt, args);
>> +  ret = grub_vsprintf (NULL, fmt, args);
>>    return ret;
>>  }
> 
> Yes.  But we have many of those, so we don't go huntin' them.  If you'd
> like to help us, a patch that does this change in bulk would be welcome.
> 
> Thanks
> 
It will be of great pleasure for me, but I didn't foreseen so much (the most difficult to me are 'opaque pointer') but I 
hoppe that such 'sparse' would help me for the most ;<)

Hope to comeback soon,
	J.

Re: Just a cosmetic question about grub_vprintf()?

[Colin Watson]

On Sat, Nov 28, 2009 at 06:21:20PM +0000, rubisher wrote:
> Robert Millan wrote:
>> On Sun, Nov 22, 2009 at 01:10:11PM +0000, rubisher wrote:
>>> But as far as the 1st parameter of grub_vsprintf is a pointer,
>>> wouldn't it be better to write:
>>> --- kern/misc.c.orig	2009-11-22 13:07:22.000000000 +0000
>>> +++ kern/misc.c	2009-11-22 13:07:51.000000000 +0000
>>> @@ -160,7 +160,7 @@
>>>  {
>>>    int ret;
>>>
>>> -  ret = grub_vsprintf (0, fmt, args);
>>> +  ret = grub_vsprintf (NULL, fmt, args);
>>>    return ret;
>>>  }
>>
>> Yes.  But we have many of those, so we don't go huntin' them.  If you'd
>> like to help us, a patch that does this change in bulk would be welcome.
>
> It will be of great pleasure for me, but I didn't foreseen so much (the 
> most difficult to me are 'opaque pointer') but I hoppe that such 'sparse' 
> would help me for the most ;<)

If you do this, make sure you understand why it makes no difference in
standards-compliant C. In particular, this understanding matters when
functions with variable-length argument lists are concerned.

(See the C FAQ for more details.)

-- 
Colin Watson                                       [cjwatson@ubuntu.com]

Re: Just a cosmetic question about grub_vprintf()?

[rubisher]

Colin Watson wrote:
> On Sat, Nov 28, 2009 at 06:21:20PM +0000, rubisher wrote:
>> Robert Millan wrote:
>>> On Sun, Nov 22, 2009 at 01:10:11PM +0000, rubisher wrote:
>>>> But as far as the 1st parameter of grub_vsprintf is a pointer,
>>>> wouldn't it be better to write:
>>>> --- kern/misc.c.orig	2009-11-22 13:07:22.000000000 +0000
>>>> +++ kern/misc.c	2009-11-22 13:07:51.000000000 +0000
>>>> @@ -160,7 +160,7 @@
>>>>  {
>>>>    int ret;
>>>>
>>>> -  ret = grub_vsprintf (0, fmt, args);
>>>> +  ret = grub_vsprintf (NULL, fmt, args);
>>>>    return ret;
>>>>  }
>>> Yes.  But we have many of those, so we don't go huntin' them.  If you'd
>>> like to help us, a patch that does this change in bulk would be welcome.
>> It will be of great pleasure for me, but I didn't foreseen so much (the 
>> most difficult to me are 'opaque pointer') but I hoppe that such 'sparse' 
>> would help me for the most ;<)
> 
> If you do this, make sure you understand why it makes no difference in
> standards-compliant C. In particular, this understanding matters when
> functions with variable-length argument lists are concerned.
> 
> (See the C FAQ for more details.)
> 
Ok, I will take care.

In the mean time I just have enough time to try this diff to add enable-sparse option using cgcc wrapper:
--- configure.ac	2009-12-05 10:12:22 +0000
+++ configure.ac	2009-12-05 19:14:14 +0000
@@ -537,6 +537,20 @@
                [AC_DEFINE([MM_DEBUG], [1],
                           [Define to 1 if you enable memory manager debugging.])])

+AC_ARG_ENABLE([sparse],
+	      AS_HELP_STRING([--enable-sparse],
+                             [enable sparse code checking]), ,
+        enable_sparse=no
+)
+
+# Set cgcc as compiler and add sparse flags if --enable-sparse was specified.
+if test "$enable_sparse" = "yes"; then
+        CC="REAL_CC=$CC cgcc"
+        CFLAGS="$CFLAGS -Wbitwise -Wnon-pointer-null"
+        TARGET_CC="REAL_CC=$TARGET_CC cgcc"
+        TARGET_CFLAGS="$TARGET_CFLAGS -Wbitwise -Wnon-pointer-null"
+fi
+
  AC_ARG_ENABLE([grub-emu-usb],
  	      [AS_HELP_STRING([--enable-grub-emu-usb],
                               [build and install the `grub-emu' debugging utility with USB support (default=guessed)])])

=== <> ===

That seems to works on my side but all advise are welcome ;<)

Tx,
	J.

Re: Just a cosmetic question about grub_vprintf()?

[Robert Millan]

On Sat, Dec 05, 2009 at 09:58:01PM +0000, rubisher wrote:
> +AC_ARG_ENABLE([sparse],
> +	      AS_HELP_STRING([--enable-sparse],
> +                             [enable sparse code checking]), ,
> +        enable_sparse=no
> +)
> +
> +# Set cgcc as compiler and add sparse flags if --enable-sparse was specified.
> +if test "$enable_sparse" = "yes"; then
> +        CC="REAL_CC=$CC cgcc"
> +        CFLAGS="$CFLAGS -Wbitwise -Wnon-pointer-null"
> +        TARGET_CC="REAL_CC=$TARGET_CC cgcc"
> +        TARGET_CFLAGS="$TARGET_CFLAGS -Wbitwise -Wnon-pointer-null"
> +fi
> +

This doesn't seem to be recognized by all versions of GCC we support.  If
we enable these flags this needs to be checked.

-- 
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."