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

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>

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

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.

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

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

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

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

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.

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

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.

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