[PATCH for-4.15 0/2] xenstore: Check format printf

[PATCH for-4.15 0/2] xenstore: Check format printf

[Julien Grall]

From: Julien Grall <jgrall@amazon.com>

This patch is a follow-up to Norbert's series [1].

The first patch will define PRINTF_ATTRIBUTE the same way everywhere.
The second patch will check the format printf on a few more calls.

Both patches are candidate for 4.15. They only affects the build system
and therefore would consider them low-risk.

Cheers,

[1] https://lore.kernel.org/xen-devel/20210226144144.9252-1-nmanthey@amazon.de/

Julien Grall (2):
  tools/xenstore: Consolidate PRINTF_ATTRIBUTE() in utils.h
  tools/xenstore: Check the format printf for xprintf() and
    barf{,_perror}()

 tools/xenstore/talloc.h | 15 ++-------------
 tools/xenstore/tdb.h    |  6 ++----
 tools/xenstore/utils.h  | 10 +++++++---
 3 files changed, 11 insertions(+), 20 deletions(-)

-- 
2.17.1

[PATCH for-4.15 1/2] tools/xenstore: Consolidate PRINTF_ATTRIBUTE() in utils.h

[Julien Grall]

From: Julien Grall <jgrall@amazon.com>

At the moment PRINTF_ATTRIBUTE() is defined in two places:
    - tdb.h: Defined as a NOP
    - talloc.h: Defined as a NOP for GCC older than 3.0 otherwise will
    add the attribute to check the printf format

Xen requires to build with minimum GCC 4.1 and we want to check the
printf format for all the printf-like functions.

Only implement PRINTF_ATTRIBUTE() once in utils.h and drop the
conditional check for GCC < 3.0.

Signed-off-by: Julien Grall <jgrall@amazon.com>
---
 tools/xenstore/talloc.h | 15 ++-------------
 tools/xenstore/tdb.h    |  6 ++----
 tools/xenstore/utils.h  |  2 ++
 3 files changed, 6 insertions(+), 17 deletions(-)

diff --git a/tools/xenstore/talloc.h b/tools/xenstore/talloc.h
index 71a36e7be06b..a0f4bff25788 100644
--- a/tools/xenstore/talloc.h
+++ b/tools/xenstore/talloc.h
@@ -26,6 +26,8 @@
 
 #include <sys/types.h>
 
+#include "utils.h"
+
 /* this is only needed for compatibility with the old talloc */
 typedef void TALLOC_CTX;
 
@@ -84,19 +86,6 @@ typedef void TALLOC_CTX;
 #define talloc_destroy(ctx) talloc_free(ctx)
 #endif
 
-#ifndef PRINTF_ATTRIBUTE
-#if (__GNUC__ >= 3)
-/** Use gcc attribute to check printf fns.  a1 is the 1-based index of
- * the parameter containing the format, and a2 the index of the first
- * argument. Note that some gcc 2.x versions don't handle this
- * properly **/
-#define PRINTF_ATTRIBUTE(a1, a2) __attribute__ ((format (__printf__, a1, a2)))
-#else
-#define PRINTF_ATTRIBUTE(a1, a2)
-#endif
-#endif
-
-
 /* The following definitions come from talloc.c  */
 void *_talloc(const void *context, size_t size);
 void talloc_set_destructor(const void *ptr, int (*destructor)(void *));
diff --git a/tools/xenstore/tdb.h b/tools/xenstore/tdb.h
index 557cf727b869..ce3c7339f884 100644
--- a/tools/xenstore/tdb.h
+++ b/tools/xenstore/tdb.h
@@ -1,6 +1,8 @@
 #ifndef __TDB_H__
 #define __TDB_H__
 
+#include "utils.h"
+
 /* 
    Unix SMB/CIFS implementation.
 
@@ -84,10 +86,6 @@ struct tdb_traverse_lock {
 	uint32_t hash;
 };
 
-#ifndef PRINTF_ATTRIBUTE
-#define PRINTF_ATTRIBUTE(a,b)
-#endif
-
 /* this is the context structure that is returned from a db open */
 typedef struct tdb_context {
 	char *name; /* the name of the database */
diff --git a/tools/xenstore/utils.h b/tools/xenstore/utils.h
index df1cb9a3bac6..3dfb96b556dd 100644
--- a/tools/xenstore/utils.h
+++ b/tools/xenstore/utils.h
@@ -27,6 +27,8 @@ static inline bool strends(const char *a, const char *b)
  */
 const char *dump_state_align(FILE *fp);
 
+#define PRINTF_ATTRIBUTE(a1, a2) __attribute__((format (printf, a1, a2)))
+
 void barf(const char *fmt, ...) __attribute__((noreturn));
 void barf_perror(const char *fmt, ...) __attribute__((noreturn));
 
-- 
2.17.1

[PATCH for-4.15 2/2] tools/xenstore: Check the format printf for xprintf() and barf{,_perror}()

[Julien Grall]

From: Julien Grall <jgrall@amazon.com>

Allow GCC to analyze the format printf for xprintf() and
barf{,_perror}().

Take the opportunity to define __noreturn to make the prototype for
barf{,_perror})() easier to read.

Signed-off-by: Julien Grall <jgrall@amazon.com>
---
 tools/xenstore/utils.h | 8 +++++---
 1 file changed, 5 insertions(+), 3 deletions(-)

diff --git a/tools/xenstore/utils.h b/tools/xenstore/utils.h
index 3dfb96b556dd..ccfb9b8fb699 100644
--- a/tools/xenstore/utils.h
+++ b/tools/xenstore/utils.h
@@ -29,10 +29,12 @@ const char *dump_state_align(FILE *fp);
 
 #define PRINTF_ATTRIBUTE(a1, a2) __attribute__((format (printf, a1, a2)))
 
-void barf(const char *fmt, ...) __attribute__((noreturn));
-void barf_perror(const char *fmt, ...) __attribute__((noreturn));
+#define __noreturn __attribute__((noreturn))
 
-extern void (*xprintf)(const char *fmt, ...);
+void barf(const char *fmt, ...) __noreturn PRINTF_ATTRIBUTE(1, 2);
+void barf_perror(const char *fmt, ...) __noreturn PRINTF_ATTRIBUTE(1, 2);
+
+extern void (*xprintf)(const char *fmt, ...) PRINTF_ATTRIBUTE(1, 2);
 
 #define eprintf(_fmt, _args...) xprintf("[ERR] %s" _fmt, __FUNCTION__, ##_args)
 
-- 
2.17.1

Re: [PATCH for-4.15 1/2] tools/xenstore: Consolidate PRINTF_ATTRIBUTE() in utils.h

[Jürgen Groß]

[-- Attachment #1.1.1: Type: text/plain, Size: 659 bytes --]

On 05.03.21 13:40, Julien Grall wrote:
> From: Julien Grall <jgrall@amazon.com>
> 
> At the moment PRINTF_ATTRIBUTE() is defined in two places:
>      - tdb.h: Defined as a NOP
>      - talloc.h: Defined as a NOP for GCC older than 3.0 otherwise will
>      add the attribute to check the printf format
> 
> Xen requires to build with minimum GCC 4.1 and we want to check the
> printf format for all the printf-like functions.
> 
> Only implement PRINTF_ATTRIBUTE() once in utils.h and drop the
> conditional check for GCC < 3.0.
> 
> Signed-off-by: Julien Grall <jgrall@amazon.com>

Reviewed-by: Juergen Gross <jgross@suse.com>


Juergen

[-- Attachment #1.1.2: OpenPGP_0xB0DE9DD628BF132F.asc --]
[-- Type: application/pgp-keys, Size: 3135 bytes --]

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 495 bytes --]

Re: [PATCH for-4.15 2/2] tools/xenstore: Check the format printf for xprintf() and barf{,_perror}()

[Jürgen Groß]

[-- Attachment #1.1.1: Type: text/plain, Size: 1365 bytes --]

On 05.03.21 13:40, Julien Grall wrote:
> From: Julien Grall <jgrall@amazon.com>
> 
> Allow GCC to analyze the format printf for xprintf() and
> barf{,_perror}().
> 
> Take the opportunity to define __noreturn to make the prototype for
> barf{,_perror})() easier to read.
> 
> Signed-off-by: Julien Grall <jgrall@amazon.com>

Reviewed-by: Juergen Gross <jgross@suse.com>

But I would prefer, if ...

> ---
>   tools/xenstore/utils.h | 8 +++++---
>   1 file changed, 5 insertions(+), 3 deletions(-)
> 
> diff --git a/tools/xenstore/utils.h b/tools/xenstore/utils.h
> index 3dfb96b556dd..ccfb9b8fb699 100644
> --- a/tools/xenstore/utils.h
> +++ b/tools/xenstore/utils.h
> @@ -29,10 +29,12 @@ const char *dump_state_align(FILE *fp);
>   
>   #define PRINTF_ATTRIBUTE(a1, a2) __attribute__((format (printf, a1, a2)))
>   
> -void barf(const char *fmt, ...) __attribute__((noreturn));
> -void barf_perror(const char *fmt, ...) __attribute__((noreturn));
> +#define __noreturn __attribute__((noreturn))
>   
> -extern void (*xprintf)(const char *fmt, ...);
> +void barf(const char *fmt, ...) __noreturn PRINTF_ATTRIBUTE(1, 2);
> +void barf_perror(const char *fmt, ...) __noreturn PRINTF_ATTRIBUTE(1, 2);
> +
> +extern void (*xprintf)(const char *fmt, ...) PRINTF_ATTRIBUTE(1, 2);

... the extern here would be dropped.


Juergen


[-- Attachment #1.1.2: OpenPGP_0xB0DE9DD628BF132F.asc --]
[-- Type: application/pgp-keys, Size: 3135 bytes --]

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 495 bytes --]

[PATCH for-4.15 1/2] tools/xenstore: Consolidate PRINTF_ATTRIBUTE() in utils.h

[Ian Jackson]

Julien Grall writes ("[PATCH for-4.15 1/2] tools/xenstore: Consolidate PRINTF_ATTRIBUTE() in utils.h"):
> From: Julien Grall <jgrall@amazon.com>
> 
> At the moment PRINTF_ATTRIBUTE() is defined in two places:
>     - tdb.h: Defined as a NOP
>     - talloc.h: Defined as a NOP for GCC older than 3.0 otherwise will
>     add the attribute to check the printf format
> 
> Xen requires to build with minimum GCC 4.1 and we want to check the
> printf format for all the printf-like functions.
> 
> Only implement PRINTF_ATTRIBUTE() once in utils.h and drop the
> conditional check for GCC < 3.0.

Release-Acked-by: Ian Jackson <iwj@xenproject.org>

[PATCH for-4.15 2/2] tools/xenstore: Check the format printf for xprintf() and barf{,_perror}()

[Ian Jackson]

Julien Grall writes ("[PATCH for-4.15 2/2] tools/xenstore: Check the format printf for xprintf() and barf{,_perror}()"):
> From: Julien Grall <jgrall@amazon.com>
> 
> Allow GCC to analyze the format printf for xprintf() and
> barf{,_perror}().
> 
> Take the opportunity to define __noreturn to make the prototype for
> barf{,_perror})() easier to read.

Release-Acked-by: Ian Jackson <iwj@xenproject.org>

Re: [PATCH for-4.15 2/2] tools/xenstore: Check the format printf for xprintf() and barf{,_perror}()

[Ian Jackson]

Jürgen Groß writes ("Re: [PATCH for-4.15 2/2] tools/xenstore: Check the format printf for xprintf() and barf{,_perror}()"):
> On 05.03.21 13:40, Julien Grall wrote:
> > -extern void (*xprintf)(const char *fmt, ...);
> > +void barf(const char *fmt, ...) __noreturn PRINTF_ATTRIBUTE(1, 2);
> > +void barf_perror(const char *fmt, ...) __noreturn PRINTF_ATTRIBUTE(1, 2);
> > +
> > +extern void (*xprintf)(const char *fmt, ...) PRINTF_ATTRIBUTE(1, 2);
> 
> ... the extern here would be dropped.

With my RM hat on I don't have an opinion on that and my R-A can
stand.

With my maintainer hat on I agree with Jürgen's style opinion - it's
nicer without the "extern", but I'm also happy with the patch as is.

Ian.

Re: [PATCH for-4.15 2/2] tools/xenstore: Check the format printf for xprintf() and barf{,_perror}()

[Julien Grall]

Hi Ian,

On 05/03/2021 13:27, Ian Jackson wrote:
> Jürgen Groß writes ("Re: [PATCH for-4.15 2/2] tools/xenstore: Check the format printf for xprintf() and barf{,_perror}()"):
>> On 05.03.21 13:40, Julien Grall wrote:
>>> -extern void (*xprintf)(const char *fmt, ...);
>>> +void barf(const char *fmt, ...) __noreturn PRINTF_ATTRIBUTE(1, 2);
>>> +void barf_perror(const char *fmt, ...) __noreturn PRINTF_ATTRIBUTE(1, 2);
>>> +
>>> +extern void (*xprintf)(const char *fmt, ...) PRINTF_ATTRIBUTE(1, 2);
>>
>> ... the extern here would be dropped.
> 
> With my RM hat on I don't have an opinion on that and my R-A can
> stand.
> 
> With my maintainer hat on I agree with Jürgen's style opinion - it's
> nicer without the "extern", but I'm also happy with the patch as is.
I agree with Juergen's style opinion. I will do the modification on 
commit. I will also update the last sentence of the commit to:

"
Take the opportunity to:
    * remove extern from the function prototype for consistency
    * define __noreturn to make the prototype for
barf{,_perror})() easier to read.
"

Cheers,

> Ian.
> 

-- 
Julien Grall

Re: [PATCH for-4.15 2/2] tools/xenstore: Check the format printf for xprintf() and barf{,_perror}()

[Jan Beulich]

On 05.03.2021 14:01, Jürgen Groß wrote:
> On 05.03.21 13:40, Julien Grall wrote:
>> From: Julien Grall <jgrall@amazon.com>
>> --- a/tools/xenstore/utils.h
>> +++ b/tools/xenstore/utils.h
>> @@ -29,10 +29,12 @@ const char *dump_state_align(FILE *fp);
>>   
>>   #define PRINTF_ATTRIBUTE(a1, a2) __attribute__((format (printf, a1, a2)))
>>   
>> -void barf(const char *fmt, ...) __attribute__((noreturn));
>> -void barf_perror(const char *fmt, ...) __attribute__((noreturn));
>> +#define __noreturn __attribute__((noreturn))
>>   
>> -extern void (*xprintf)(const char *fmt, ...);
>> +void barf(const char *fmt, ...) __noreturn PRINTF_ATTRIBUTE(1, 2);
>> +void barf_perror(const char *fmt, ...) __noreturn PRINTF_ATTRIBUTE(1, 2);
>> +
>> +extern void (*xprintf)(const char *fmt, ...) PRINTF_ATTRIBUTE(1, 2);
> 
> ... the extern here would be dropped.

But this isn't a function declaration, but that of a data object.
With the extern dropped, a common symbol will appear in every CU.

Jan

Re: [PATCH for-4.15 2/2] tools/xenstore: Check the format printf for xprintf() and barf{,_perror}()

[Andrew Cooper]

On 05/03/2021 13:45, Jan Beulich wrote:
> On 05.03.2021 14:01, Jürgen Groß wrote:
>> On 05.03.21 13:40, Julien Grall wrote:
>>> From: Julien Grall <jgrall@amazon.com>
>>> --- a/tools/xenstore/utils.h
>>> +++ b/tools/xenstore/utils.h
>>> @@ -29,10 +29,12 @@ const char *dump_state_align(FILE *fp);
>>>   
>>>   #define PRINTF_ATTRIBUTE(a1, a2) __attribute__((format (printf, a1, a2)))
>>>   
>>> -void barf(const char *fmt, ...) __attribute__((noreturn));
>>> -void barf_perror(const char *fmt, ...) __attribute__((noreturn));
>>> +#define __noreturn __attribute__((noreturn))
>>>   
>>> -extern void (*xprintf)(const char *fmt, ...);
>>> +void barf(const char *fmt, ...) __noreturn PRINTF_ATTRIBUTE(1, 2);
>>> +void barf_perror(const char *fmt, ...) __noreturn PRINTF_ATTRIBUTE(1, 2);
>>> +
>>> +extern void (*xprintf)(const char *fmt, ...) PRINTF_ATTRIBUTE(1, 2);
>> ... the extern here would be dropped.
> But this isn't a function declaration, but that of a data object.
> With the extern dropped, a common symbol will appear in every CU.

Correct, and some of the containers in Gitlab CI really ought to notice
because GCC 9(? IIRC) defaulted to -fno-common.

~Andrew

Re: [PATCH for-4.15 2/2] tools/xenstore: Check the format printf for xprintf() and barf{,_perror}()

[Julien Grall]

Hi Jan,

On 05/03/2021 13:45, Jan Beulich wrote:
> On 05.03.2021 14:01, Jürgen Groß wrote:
>> On 05.03.21 13:40, Julien Grall wrote:
>>> From: Julien Grall <jgrall@amazon.com>
>>> --- a/tools/xenstore/utils.h
>>> +++ b/tools/xenstore/utils.h
>>> @@ -29,10 +29,12 @@ const char *dump_state_align(FILE *fp);
>>>    
>>>    #define PRINTF_ATTRIBUTE(a1, a2) __attribute__((format (printf, a1, a2)))
>>>    
>>> -void barf(const char *fmt, ...) __attribute__((noreturn));
>>> -void barf_perror(const char *fmt, ...) __attribute__((noreturn));
>>> +#define __noreturn __attribute__((noreturn))
>>>    
>>> -extern void (*xprintf)(const char *fmt, ...);
>>> +void barf(const char *fmt, ...) __noreturn PRINTF_ATTRIBUTE(1, 2);
>>> +void barf_perror(const char *fmt, ...) __noreturn PRINTF_ATTRIBUTE(1, 2);
>>> +
>>> +extern void (*xprintf)(const char *fmt, ...) PRINTF_ATTRIBUTE(1, 2);
>>
>> ... the extern here would be dropped.
> 
> But this isn't a function declaration, but that of a data object.
> With the extern dropped, a common symbol will appear in every CU.

Urgh, you are right. Actually, the extern was added recently by Anthony:

dacdbf7088d6a3705a9831e73991c2b14c519a65 ("tools/xenstore: mark variable 
in header as extern")

I completely forgot it despite I needed to backport the patch to our 
downstream Xen.

Cheers,

-- 
Julien Grall

Re: [PATCH for-4.15 2/2] tools/xenstore: Check the format printf for xprintf() and barf{,_perror}()

[Ian Jackson]

Julien Grall writes ("Re: [PATCH for-4.15 2/2] tools/xenstore: Check the format printf for xprintf() and barf{,_perror}()"):
> Urgh, you are right. Actually, the extern was added recently by Anthony:
> 
> dacdbf7088d6a3705a9831e73991c2b14c519a65 ("tools/xenstore: mark variable 
> in header as extern")
> 
> I completely forgot it despite I needed to backport the patch to our 
> downstream Xen.

How horrible.

Maybe we could add a comment to the code, next to the declaration,
about this crazy situation.

Ian.

Re: [PATCH for-4.15 2/2] tools/xenstore: Check the format printf for xprintf() and barf{,_perror}()

[Julien Grall]

Hi Ian,

On 05/03/2021 13:58, Ian Jackson wrote:
> Julien Grall writes ("Re: [PATCH for-4.15 2/2] tools/xenstore: Check the format printf for xprintf() and barf{,_perror}()"):
>> Urgh, you are right. Actually, the extern was added recently by Anthony:
>>
>> dacdbf7088d6a3705a9831e73991c2b14c519a65 ("tools/xenstore: mark variable
>> in header as extern")
>>
>> I completely forgot it despite I needed to backport the patch to our
>> downstream Xen.
> 
> How horrible.
> 
> Maybe we could add a comment to the code, next to the declaration,
> about this crazy situation.

Would the following comment work for you?

/* Function pointer as xprintf() can be configured at runtime. */

I can fold it in my patch while committing.

Cheers,

> 
> Ian.
> 

-- 
Julien Grall

Re: [PATCH for-4.15 2/2] tools/xenstore: Check the format printf for xprintf() and barf{,_perror}()

[Ian Jackson]

Julien Grall writes ("Re: [PATCH for-4.15 2/2] tools/xenstore: Check the format printf for xprintf() and barf{,_perror}()"):
> Would the following comment work for you?
> 
> /* Function pointer as xprintf() can be configured at runtime. */
> 
> I can fold it in my patch while committing.

Sure, thanks.  FTAOD

Reviewed-by: Ian Jackson <iwj@xenproject.org>
Release-Acked-by: Ian Jackson <iwj@xenproject.org>

to that comment addition.

Ian.

Re: [PATCH for-4.15 2/2] tools/xenstore: Check the format printf for xprintf() and barf{,_perror}()

[Julien Grall]

Hi Ian,

On 08/03/2021 09:44, Ian Jackson wrote:
> Julien Grall writes ("Re: [PATCH for-4.15 2/2] tools/xenstore: Check the format printf for xprintf() and barf{,_perror}()"):
>> Would the following comment work for you?
>>
>> /* Function pointer as xprintf() can be configured at runtime. */
>>
>> I can fold it in my patch while committing.
> 
> Sure, thanks.  FTAOD
> 
> Reviewed-by: Ian Jackson <iwj@xenproject.org>
> Release-Acked-by: Ian Jackson <iwj@xenproject.org>
> 
> to that comment addition.

Thanks! I have committed the two patches.

Cheers,

-- 
Julien Grall