| Message ID | 20210305124003.13582-3-julien@xen.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | xenstore: Check format printf | expand |
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
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>
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. >
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 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 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,
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)