1. Patchew
  2. Xen
  3. View series

[PATCH] xen: Give compile.h header guards

Andrew Cooper posted 1 patch 5 months, 2 weeks ago
Patches applied successfully (tree, apply log)
git fetch https://gitlab.com/xen-project/patchew/xen tags/patchew/20250519135227.27386-1-andrew.cooper3@citrix.com
xen/include/xen/compile.h.in | 3 +++
xen/tools/process-banner.sed | 5 +++++
2 files changed, 8 insertions(+)
[PATCH] xen: Give compile.h header guards
Posted by Andrew Cooper 5 months, 2 weeks ago 
Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
---
CC: Anthony PERARD <anthony.perard@vates.tech>
CC: Michal Orzel <michal.orzel@amd.com>
CC: Jan Beulich <jbeulich@suse.com>
CC: Julien Grall <julien@xen.org>
CC: Roger Pau Monné <roger.pau@citrix.com>
CC: Stefano Stabellini <sstabellini@kernel.org>
---
 xen/include/xen/compile.h.in | 3 +++
 xen/tools/process-banner.sed | 5 +++++
 2 files changed, 8 insertions(+)

diff --git a/xen/include/xen/compile.h.in b/xen/include/xen/compile.h.in
index 3151d1e7d1bf..9206341ba692 100644
--- a/xen/include/xen/compile.h.in
+++ b/xen/include/xen/compile.h.in
@@ -1,3 +1,6 @@
+#ifndef XEN_COMPILE_H
+#define XEN_COMPILE_H
+
 #define XEN_COMPILE_DATE	"@@date@@"
 #define XEN_COMPILE_TIME	"@@time@@"
 #define XEN_COMPILE_BY		"@@whoami@@"
diff --git a/xen/tools/process-banner.sed b/xen/tools/process-banner.sed
index 56c76558bcd9..4cf3f9a1163a 100755
--- a/xen/tools/process-banner.sed
+++ b/xen/tools/process-banner.sed
@@ -12,3 +12,8 @@ s_(.*)_"\1\\n"_
 
 # Trailing \ on all but the final line.
 $!s_$_ \\_
+
+# Append closing header guard
+$a\
+\
+#endif /* XEN_COMPILE_H */

base-commit: 6fc02ebdd053856221f37ba5306232ac1575332d
prerequisite-patch-id: 7bc1c498ba2c9c4a4939a56a0006f820f47f2a66
-- 
2.39.5
Re: [PATCH] xen: Give compile.h header guards
Posted by Frediano Ziglio 5 months, 1 week ago 
On Mon, May 19, 2025 at 2:52 PM Andrew Cooper <andrew.cooper3@citrix.com> wrote:
>
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
> ---
> CC: Anthony PERARD <anthony.perard@vates.tech>
> CC: Michal Orzel <michal.orzel@amd.com>
> CC: Jan Beulich <jbeulich@suse.com>
> CC: Julien Grall <julien@xen.org>
> CC: Roger Pau Monné <roger.pau@citrix.com>
> CC: Stefano Stabellini <sstabellini@kernel.org>
> ---
>  xen/include/xen/compile.h.in | 3 +++
>  xen/tools/process-banner.sed | 5 +++++
>  2 files changed, 8 insertions(+)
>
> diff --git a/xen/include/xen/compile.h.in b/xen/include/xen/compile.h.in
> index 3151d1e7d1bf..9206341ba692 100644
> --- a/xen/include/xen/compile.h.in
> +++ b/xen/include/xen/compile.h.in
> @@ -1,3 +1,6 @@
> +#ifndef XEN_COMPILE_H
> +#define XEN_COMPILE_H
> +

Why not follow CODING_STYLE ?

OT: Maybe while on it, why not add SPDX comments too ?

>  #define XEN_COMPILE_DATE       "@@date@@"
>  #define XEN_COMPILE_TIME       "@@time@@"
>  #define XEN_COMPILE_BY         "@@whoami@@"
> diff --git a/xen/tools/process-banner.sed b/xen/tools/process-banner.sed
> index 56c76558bcd9..4cf3f9a1163a 100755
> --- a/xen/tools/process-banner.sed
> +++ b/xen/tools/process-banner.sed
> @@ -12,3 +12,8 @@ s_(.*)_"\1\\n"_
>
>  # Trailing \ on all but the final line.
>  $!s_$_ \\_
> +
> +# Append closing header guard
> +$a\
> +\
> +#endif /* XEN_COMPILE_H */
>
> base-commit: 6fc02ebdd053856221f37ba5306232ac1575332d
> prerequisite-patch-id: 7bc1c498ba2c9c4a4939a56a0006f820f47f2a66
> --
> 2.39.5
>
>
Frediano
Re: [PATCH] xen: Give compile.h header guards
Posted by Jan Beulich 5 months, 2 weeks ago 
On 19.05.2025 15:52, Andrew Cooper wrote:
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>

Is this to please Misra in some way?

> --- a/xen/include/xen/compile.h.in
> +++ b/xen/include/xen/compile.h.in
> @@ -1,3 +1,6 @@
> +#ifndef XEN_COMPILE_H
> +#define XEN_COMPILE_H
> +
>  #define XEN_COMPILE_DATE	"@@date@@"
>  #define XEN_COMPILE_TIME	"@@time@@"
>  #define XEN_COMPILE_BY		"@@whoami@@"
> --- a/xen/tools/process-banner.sed
> +++ b/xen/tools/process-banner.sed
> @@ -12,3 +12,8 @@ s_(.*)_"\1\\n"_
>  
>  # Trailing \ on all but the final line.
>  $!s_$_ \\_
> +
> +# Append closing header guard
> +$a\
> +\
> +#endif /* XEN_COMPILE_H */

This split of #ifndef and #endif is ugly. Can't we switch to something
more conventional, like

#define XEN_BANNER		"@@banner@@"

with the first sed invocation then replacing this by the result of
a nested sed invocation using process-banner.sed (which of course would
need adjusting some)? (Maybe the double quotes would need omitting here,
for process-banner.sed to uniformly apply them.)

Jan
Re: [PATCH] xen: Give compile.h header guards
Posted by Stefano Stabellini 5 months, 2 weeks ago 
On Mon, 19 May 2025, Jan Beulich wrote:
> On 19.05.2025 15:52, Andrew Cooper wrote:
> > Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
> 
> Is this to please Misra in some way?
> 
> > --- a/xen/include/xen/compile.h.in
> > +++ b/xen/include/xen/compile.h.in
> > @@ -1,3 +1,6 @@
> > +#ifndef XEN_COMPILE_H
> > +#define XEN_COMPILE_H
> > +
> >  #define XEN_COMPILE_DATE	"@@date@@"
> >  #define XEN_COMPILE_TIME	"@@time@@"
> >  #define XEN_COMPILE_BY		"@@whoami@@"
> > --- a/xen/tools/process-banner.sed
> > +++ b/xen/tools/process-banner.sed
> > @@ -12,3 +12,8 @@ s_(.*)_"\1\\n"_
> >  
> >  # Trailing \ on all but the final line.
> >  $!s_$_ \\_
> > +
> > +# Append closing header guard
> > +$a\
> > +\
> > +#endif /* XEN_COMPILE_H */
> 
> This split of #ifndef and #endif is ugly. Can't we switch to something
> more conventional, like
> 
> #define XEN_BANNER		"@@banner@@"
> 
> with the first sed invocation then replacing this by the result of
> a nested sed invocation using process-banner.sed (which of course would
> need adjusting some)? (Maybe the double quotes would need omitting here,
> for process-banner.sed to uniformly apply them.)

While I agree with Jan that this is kind of ugly, it is a unique case
and I would prefer an ugly but simple solution than a more complex
solution. This would be different if we were talking about a widespread
pattern, in which case the price for complexity would be worth it.

My 2 cents in this case are in favor of the simplest approach. I would
ack this patch.
Re: [PATCH] xen: Give compile.h header guards
Posted by Jan Beulich 5 months, 1 week ago 
On 19.05.2025 23:34, Stefano Stabellini wrote:
> On Mon, 19 May 2025, Jan Beulich wrote:
>> On 19.05.2025 15:52, Andrew Cooper wrote:
>>> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
>>
>> Is this to please Misra in some way?
>>
>>> --- a/xen/include/xen/compile.h.in
>>> +++ b/xen/include/xen/compile.h.in
>>> @@ -1,3 +1,6 @@
>>> +#ifndef XEN_COMPILE_H
>>> +#define XEN_COMPILE_H
>>> +
>>>  #define XEN_COMPILE_DATE	"@@date@@"
>>>  #define XEN_COMPILE_TIME	"@@time@@"
>>>  #define XEN_COMPILE_BY		"@@whoami@@"
>>> --- a/xen/tools/process-banner.sed
>>> +++ b/xen/tools/process-banner.sed
>>> @@ -12,3 +12,8 @@ s_(.*)_"\1\\n"_
>>>  
>>>  # Trailing \ on all but the final line.
>>>  $!s_$_ \\_
>>> +
>>> +# Append closing header guard
>>> +$a\
>>> +\
>>> +#endif /* XEN_COMPILE_H */
>>
>> This split of #ifndef and #endif is ugly. Can't we switch to something
>> more conventional, like
>>
>> #define XEN_BANNER		"@@banner@@"
>>
>> with the first sed invocation then replacing this by the result of
>> a nested sed invocation using process-banner.sed (which of course would
>> need adjusting some)? (Maybe the double quotes would need omitting here,
>> for process-banner.sed to uniformly apply them.)
> 
> While I agree with Jan that this is kind of ugly, it is a unique case
> and I would prefer an ugly but simple solution than a more complex
> solution. This would be different if we were talking about a widespread
> pattern, in which case the price for complexity would be worth it.
> 
> My 2 cents in this case are in favor of the simplest approach. I would
> ack this patch.

Feel free to do so; my comment was not meant as a plain objection, but more
as a suggestion. The one thing I would ask for is a non-empty description,
though.

Jan
Re: [PATCH] xen: Give compile.h header guards
Posted by Stefano Stabellini 5 months, 1 week ago 
On Tue, 20 May 2025, Jan Beulich wrote:
> On 19.05.2025 23:34, Stefano Stabellini wrote:
> > On Mon, 19 May 2025, Jan Beulich wrote:
> >> On 19.05.2025 15:52, Andrew Cooper wrote:
> >>> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
> >>
> >> Is this to please Misra in some way?
> >>
> >>> --- a/xen/include/xen/compile.h.in
> >>> +++ b/xen/include/xen/compile.h.in
> >>> @@ -1,3 +1,6 @@
> >>> +#ifndef XEN_COMPILE_H
> >>> +#define XEN_COMPILE_H
> >>> +
> >>>  #define XEN_COMPILE_DATE	"@@date@@"
> >>>  #define XEN_COMPILE_TIME	"@@time@@"
> >>>  #define XEN_COMPILE_BY		"@@whoami@@"
> >>> --- a/xen/tools/process-banner.sed
> >>> +++ b/xen/tools/process-banner.sed
> >>> @@ -12,3 +12,8 @@ s_(.*)_"\1\\n"_
> >>>  
> >>>  # Trailing \ on all but the final line.
> >>>  $!s_$_ \\_
> >>> +
> >>> +# Append closing header guard
> >>> +$a\
> >>> +\
> >>> +#endif /* XEN_COMPILE_H */
> >>
> >> This split of #ifndef and #endif is ugly. Can't we switch to something
> >> more conventional, like
> >>
> >> #define XEN_BANNER		"@@banner@@"
> >>
> >> with the first sed invocation then replacing this by the result of
> >> a nested sed invocation using process-banner.sed (which of course would
> >> need adjusting some)? (Maybe the double quotes would need omitting here,
> >> for process-banner.sed to uniformly apply them.)
> > 
> > While I agree with Jan that this is kind of ugly, it is a unique case
> > and I would prefer an ugly but simple solution than a more complex
> > solution. This would be different if we were talking about a widespread
> > pattern, in which case the price for complexity would be worth it.
> > 
> > My 2 cents in this case are in favor of the simplest approach. I would
> > ack this patch.
> 
> Feel free to do so; my comment was not meant as a plain objection, but more
> as a suggestion. The one thing I would ask for is a non-empty description,
> though.

Fair enough. Andrew, please add a better commit message. With that:

Reviewed-by: Stefano Stabellini <sstabellini@kernel.org>
Re: [PATCH] xen: Give compile.h header guards
Posted by Nicola Vetrini 5 months, 2 weeks ago 
On 2025-05-19 21:10, Jan Beulich wrote:
> On 19.05.2025 15:52, Andrew Cooper wrote:
>> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
> 
> Is this to please Misra in some way?
> 

Directive 4.10: "Precautions shall be taken in order to prevent the 
contents of a header file being included more than once". One approach 
is to special-case this file, but Andrew suggested this approach which 
addresses the issue directly.

>> --- a/xen/include/xen/compile.h.in
>> +++ b/xen/include/xen/compile.h.in
>> @@ -1,3 +1,6 @@
>> +#ifndef XEN_COMPILE_H
>> +#define XEN_COMPILE_H
>> +
>>  #define XEN_COMPILE_DATE	"@@date@@"
>>  #define XEN_COMPILE_TIME	"@@time@@"
>>  #define XEN_COMPILE_BY		"@@whoami@@"
>> --- a/xen/tools/process-banner.sed
>> +++ b/xen/tools/process-banner.sed
>> @@ -12,3 +12,8 @@ s_(.*)_"\1\\n"_
>> 
>>  # Trailing \ on all but the final line.
>>  $!s_$_ \\_
>> +
>> +# Append closing header guard
>> +$a\
>> +\
>> +#endif /* XEN_COMPILE_H */
> 
> This split of #ifndef and #endif is ugly. Can't we switch to something
> more conventional, like
> 
> #define XEN_BANNER		"@@banner@@"
> 
> with the first sed invocation then replacing this by the result of
> a nested sed invocation using process-banner.sed (which of course would
> need adjusting some)? (Maybe the double quotes would need omitting 
> here,
> for process-banner.sed to uniformly apply them.)
> 
> Jan

-- 
Nicola Vetrini, B.Sc.
Software Engineer
BUGSENG (https://bugseng.com)
LinkedIn: https://www.linkedin.com/in/nicola-vetrini-a42471253

Patchew 2.1-devel-b67e4c2

© 2016 - 2025 Red Hat, Inc.