RE: [PATCH] [ACPI] utilities/: Compliment va_start() with va_end().

RE: [PATCH] [ACPI] utilities/: Compliment va_start() with va_end().

[Moore, Robert]

This is an interesting one to me.

>From various documentation:

After all arguments have been retrieved, va_end resets the pointer to
NULL.

va_end
Each invocation of va_start must be matched by a corresponding
invocation of va_end in the same function. After the call va_end(ap) the
variable ap is undefined. Multiple transversals of the list, each
bracketed by va_start and va_end are possible. va_end may be a macro or
a function.   

Now, I'm all for defensive programming, but I don't really see the point
of va_end when the list will be only traversed once.



We don't set all local pointers to NULL at function exit, what is the
point of doing it here?

I suppose some implementation could allocate memory at va_start, but in
practice, does this happen?

Bob


>-----Original Message-----
>From: Len Brown [mailto:lenb@kernel.org]
>Sent: Saturday, November 24, 2007 11:06 PM
>To: Moore, Robert
>Subject: Fwd: [PATCH] [ACPI] utilities/: Compliment va_start() with
>va_end().
>
>
>
>----------  Forwarded Message  ----------
>
>Subject: [PATCH] [ACPI] utilities/: Compliment va_start() with
va_end().
>Date: Saturday 24 November 2007 15:43
>From: Richard Knutsson <ricknu-0@student.ltu.se>
>To: len.brown@intel.com
>Cc: linux-acpi@vger.kernel.org, linux-kernel@vger.kernel.org, Richard
>Knutsson <ricknu-0@student.ltu.se>
>
>Compliment va_start() with va_end().
>
>Signed-off-by: Richard Knutsson <ricknu-0@student.ltu.se>
>---
>Compile-tested on i386 with allyesconfig & allmodconfig.
>
> utdebug.c |    2 ++
> utmisc.c  |    4 ++++
> 2 files changed, 6 insertions(+)
>
>
>diff --git a/drivers/acpi/utilities/utdebug.c
>b/drivers/acpi/utilities/utdebug.c
>index c7e128e..f45e3d5 100644
>--- a/drivers/acpi/utilities/utdebug.c
>+++ b/drivers/acpi/utilities/utdebug.c
>@@ -203,6 +203,7 @@ acpi_ut_debug_print(u32 requested_debug_level,
>
> 	va_start(args, format);
> 	acpi_os_vprintf(format, args);
>+	va_end(args);
> }
>
> ACPI_EXPORT_SYMBOL(acpi_ut_debug_print)
>@@ -240,6 +241,7 @@ acpi_ut_debug_print_raw(u32 requested_debug_level,
>
> 	va_start(args, format);
> 	acpi_os_vprintf(format, args);
>+	va_end(args);
> }
>
> ACPI_EXPORT_SYMBOL(acpi_ut_debug_print_raw)
>diff --git a/drivers/acpi/utilities/utmisc.c
>b/drivers/acpi/utilities/utmisc.c
>index 2d19f71..ca4904c 100644
>--- a/drivers/acpi/utilities/utmisc.c
>+++ b/drivers/acpi/utilities/utmisc.c
>@@ -1032,6 +1032,7 @@ acpi_ut_error(char *module_name, u32 line_number,
>char *format, ...)
>
> 	va_start(args, format);
> 	acpi_os_vprintf(format, args);
>+	va_end(args);
> 	acpi_os_printf(" [%X]\n", ACPI_CA_VERSION);
> }
>
>@@ -1046,6 +1047,7 @@ acpi_ut_exception(char *module_name,
>
> 	va_start(args, format);
> 	acpi_os_vprintf(format, args);
>+	va_end(args);
> 	acpi_os_printf(" [%X]\n", ACPI_CA_VERSION);
> }
>
>@@ -1060,6 +1062,7 @@ acpi_ut_warning(char *module_name, u32
line_number,
>char *format, ...)
>
> 	va_start(args, format);
> 	acpi_os_vprintf(format, args);
>+	va_end(args);
> 	acpi_os_printf(" [%X]\n", ACPI_CA_VERSION);
> }
>
>@@ -1076,5 +1079,6 @@ acpi_ut_info(char *module_name, u32 line_number,
char
>*format, ...)
>
> 	va_start(args, format);
> 	acpi_os_vprintf(format, args);
>+	va_end(args);
> 	acpi_os_printf("\n");
> }
>-
>To unsubscribe from this list: send the line "unsubscribe linux-kernel"
in
>the body of a message to majordomo@vger.kernel.org
>More majordomo info at  http://vger.kernel.org/majordomo-info.html
>Please read the FAQ at  http://www.tux.org/lkml/
>
>
>-------------------------------------------------------

Re: [PATCH] [ACPI] utilities/: Compliment va_start() with va_end().

[Richard Knutsson]

Moore, Robert wrote:
> This is an interesting one to me.
>
> From various documentation:
>
> After all arguments have been retrieved, va_end resets the pointer to
> NULL.
>
> va_end
> Each invocation of va_start must be matched by a corresponding
> invocation of va_end in the same function. After the call va_end(ap) the
> variable ap is undefined. Multiple transversals of the list, each
> bracketed by va_start and va_end are possible. va_end may be a macro or
> a function.   
>
> Now, I'm all for defensive programming, but I don't really see the point
> of va_end when the list will be only traversed once.
>
>   
First off, I think it is a good idea to follow the documentation, which stated:
"va_end
Each invocation of va_start must be matched by a corresponding
invocation of va_end in the same function."

Then if it is not really needed, does it take up extra cycles?
"In practice, with most C compilers, calling |va_end| does nothing
and you do not really need to call it.  This is always true in the GNU C
compiler."[1]

Portability:
"But you might as well call |va_end| just in case your
program is someday compiled with a peculiar compiler."[2]
This argument is not as likely thou, but who knows? (Since I guess Intel's
compiler is included in the 'most C compilers')

>
> We don't set all local pointers to NULL at function exit, what is the
> point of doing it here?
>   
I think it is a good thing if the code follows the documentation, both 
for the person who tries
to understand the code (to see when the 'args' is no longer needed and 
not getting confused
by the absent of va_end(), after all, IMHO we should write the code how 
we want things to
work and let the compiler do the optimizations (it usually does a better 
job at it then we do))
and to automated searches (that is how I found this one).
> I suppose some implementation could allocate memory at va_start, but in
> practice, does this happen?
>   
Not sure what you mean.
> Bob
>
>   
cu
Richard Knutsson

[1] 
http://www.cs.utah.edu/dept/old/texinfo/glibc-manual-0.02/library_28.html
[2] The rest of [1]'s line.

RE: [PATCH] [ACPI] utilities/: Compliment va_start() with va_end().

[Moore, Robert]

Yes, it's official ANSI C, so I agree with the portability. I'm probably
asking more about the history of the thing.


>-----Original Message-----
>From: Richard Knutsson [mailto:ricknu-0@student.ltu.se]
>Sent: Monday, November 26, 2007 4:16 PM
>To: Moore, Robert
>Cc: Len Brown; linux-kernel@vger.kernel.org; linux-acpi@vger.kernel.org
>Subject: Re: [PATCH] [ACPI] utilities/: Compliment va_start() with
>va_end().
>
>Moore, Robert wrote:
>> This is an interesting one to me.
>>
>> From various documentation:
>>
>> After all arguments have been retrieved, va_end resets the pointer to
>> NULL.
>>
>> va_end
>> Each invocation of va_start must be matched by a corresponding
>> invocation of va_end in the same function. After the call va_end(ap)
the
>> variable ap is undefined. Multiple transversals of the list, each
>> bracketed by va_start and va_end are possible. va_end may be a macro
or
>> a function.
>>
>> Now, I'm all for defensive programming, but I don't really see the
point
>> of va_end when the list will be only traversed once.
>>
>>
>First off, I think it is a good idea to follow the documentation, which
>stated:
>"va_end
>Each invocation of va_start must be matched by a corresponding
>invocation of va_end in the same function."
>
>Then if it is not really needed, does it take up extra cycles?
>"In practice, with most C compilers, calling |va_end| does nothing
>and you do not really need to call it.  This is always true in the GNU
C
>compiler."[1]
>
>Portability:
>"But you might as well call |va_end| just in case your
>program is someday compiled with a peculiar compiler."[2]
>This argument is not as likely thou, but who knows? (Since I guess
Intel's
>compiler is included in the 'most C compilers')
>
>>
>> We don't set all local pointers to NULL at function exit, what is the
>> point of doing it here?
>>
>I think it is a good thing if the code follows the documentation, both
>for the person who tries
>to understand the code (to see when the 'args' is no longer needed and
>not getting confused
>by the absent of va_end(), after all, IMHO we should write the code how
>we want things to
>work and let the compiler do the optimizations (it usually does a
better
>job at it then we do))
>and to automated searches (that is how I found this one).
>> I suppose some implementation could allocate memory at va_start, but
in
>> practice, does this happen?
>>
>Not sure what you mean.
>> Bob
>>
>>
>cu
>Richard Knutsson
>
>[1]
>http://www.cs.utah.edu/dept/old/texinfo/glibc-manual-0.02/library_28.ht
ml
>[2] The rest of [1]'s line.

Re: [PATCH] [ACPI] utilities/: Compliment va_start() with va_end().

[Richard Knutsson]

Moore, Robert wrote:
> Yes, it's official ANSI C, so I agree with the portability. I'm probably
> asking more about the history of the thing.
>
>   
"the history of the thing"? Sorry, you lost me there. I know there were 
a pre-ANSI
version of va_start() & co., but they seemed quite messy. When it comes 
to va_end()
and maintainers, they often seem positive to this. I guess the 
occasional lack off
va_end() is usually an oversight.
>   
>> -----Original Message-----
>> From: Richard Knutsson [mailto:ricknu-0@student.ltu.se]
>> Sent: Monday, November 26, 2007 4:16 PM
>> To: Moore, Robert
>> Cc: Len Brown; linux-kernel@vger.kernel.org; linux-acpi@vger.kernel.org
>> Subject: Re: [PATCH] [ACPI] utilities/: Compliment va_start() with
>> va_end().
>>
>> Moore, Robert wrote:
>>     
>>> This is an interesting one to me.
>>>
>>> From various documentation:
>>>
>>> After all arguments have been retrieved, va_end resets the pointer to
>>> NULL.
>>>
>>> va_end
>>> Each invocation of va_start must be matched by a corresponding
>>> invocation of va_end in the same function. After the call va_end(ap)
>>>       
> the
>   
>>> variable ap is undefined. Multiple transversals of the list, each
>>> bracketed by va_start and va_end are possible. va_end may be a macro
>>>       
> or
>   
>>> a function.
>>>
>>> Now, I'm all for defensive programming, but I don't really see the
>>>       
> point
>   
>>> of va_end when the list will be only traversed once.
>>>
>>>
>>>       
>> First off, I think it is a good idea to follow the documentation, which
>> stated:
>> "va_end
>> Each invocation of va_start must be matched by a corresponding
>> invocation of va_end in the same function."
>>
>> Then if it is not really needed, does it take up extra cycles?
>> "In practice, with most C compilers, calling |va_end| does nothing
>> and you do not really need to call it.  This is always true in the GNU
>>     
> C
>   
>> compiler."[1]
>>
>> Portability:
>> "But you might as well call |va_end| just in case your
>> program is someday compiled with a peculiar compiler."[2]
>> This argument is not as likely thou, but who knows? (Since I guess
>>     
> Intel's
>   
>> compiler is included in the 'most C compilers')
>>
>>     
>>> We don't set all local pointers to NULL at function exit, what is the
>>> point of doing it here?
>>>
>>>       
>> I think it is a good thing if the code follows the documentation, both
>> for the person who tries
>> to understand the code (to see when the 'args' is no longer needed and
>> not getting confused
>> by the absent of va_end(), after all, IMHO we should write the code how
>> we want things to
>> work and let the compiler do the optimizations (it usually does a
>>     
> better
>   
>> job at it then we do))
>> and to automated searches (that is how I found this one).
>>     
>>> I suppose some implementation could allocate memory at va_start, but
>>>       
> in
>   
>>> practice, does this happen?
>>>
>>>       
>> Not sure what you mean.
>>     
>>> Bob
>>>
>>>
>>>       
>> cu
>> Richard Knutsson
>>
>> [1]
>> http://www.cs.utah.edu/dept/old/texinfo/glibc-manual-0.02/library_28.ht
>>     
> ml
>   
>> [2] The rest of [1]'s line.
>>     
> -
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/
>   

[PATCH] [ACPI] utilities/: Compliment va_start() with va_end().

[Richard Knutsson]

Compliment va_start() with va_end().

Signed-off-by: Richard Knutsson <ricknu-0@student.ltu.se>
---
Compile-tested on i386 with allyesconfig & allmodconfig.

 utdebug.c |    2 ++
 utmisc.c  |    4 ++++
 2 files changed, 6 insertions(+)


diff --git a/drivers/acpi/utilities/utdebug.c b/drivers/acpi/utilities/utdebug.c
index c7e128e..f45e3d5 100644
--- a/drivers/acpi/utilities/utdebug.c
+++ b/drivers/acpi/utilities/utdebug.c
@@ -203,6 +203,7 @@ acpi_ut_debug_print(u32 requested_debug_level,
 
 	va_start(args, format);
 	acpi_os_vprintf(format, args);
+	va_end(args);
 }
 
 ACPI_EXPORT_SYMBOL(acpi_ut_debug_print)
@@ -240,6 +241,7 @@ acpi_ut_debug_print_raw(u32 requested_debug_level,
 
 	va_start(args, format);
 	acpi_os_vprintf(format, args);
+	va_end(args);
 }
 
 ACPI_EXPORT_SYMBOL(acpi_ut_debug_print_raw)
diff --git a/drivers/acpi/utilities/utmisc.c b/drivers/acpi/utilities/utmisc.c
index 2d19f71..ca4904c 100644
--- a/drivers/acpi/utilities/utmisc.c
+++ b/drivers/acpi/utilities/utmisc.c
@@ -1032,6 +1032,7 @@ acpi_ut_error(char *module_name, u32 line_number, char *format, ...)
 
 	va_start(args, format);
 	acpi_os_vprintf(format, args);
+	va_end(args);
 	acpi_os_printf(" [%X]\n", ACPI_CA_VERSION);
 }
 
@@ -1046,6 +1047,7 @@ acpi_ut_exception(char *module_name,
 
 	va_start(args, format);
 	acpi_os_vprintf(format, args);
+	va_end(args);
 	acpi_os_printf(" [%X]\n", ACPI_CA_VERSION);
 }
 
@@ -1060,6 +1062,7 @@ acpi_ut_warning(char *module_name, u32 line_number, char *format, ...)
 
 	va_start(args, format);
 	acpi_os_vprintf(format, args);
+	va_end(args);
 	acpi_os_printf(" [%X]\n", ACPI_CA_VERSION);
 }
 
@@ -1076,5 +1079,6 @@ acpi_ut_info(char *module_name, u32 line_number, char *format, ...)
 
 	va_start(args, format);
 	acpi_os_vprintf(format, args);
+	va_end(args);
 	acpi_os_printf("\n");
 }