[PATCH] tools/libs/light: fix race in Makefile

Juergen Gross posted 1 patch 3 years, 6 months ago
Patches applied successfully (tree, apply log)
git fetch https://github.com/patchew-project/xen tags/patchew/20201025101218.20478-1-jgross@suse.com
Maintainers: Wei Liu <wl@xen.org>, Ian Jackson <iwj@xenproject.org>
tools/libs/light/Makefile | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)
[PATCH] tools/libs/light: fix race in Makefile
Posted by Juergen Gross 3 years, 6 months ago
The header $(INCLUDE)/_lixl_list.h matches two different rules, which
can result in build breakage. Fix that.

Signed-off-by: Juergen Gross <jgross@suse.com>
---
 tools/libs/light/Makefile | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/tools/libs/light/Makefile b/tools/libs/light/Makefile
index 3424fdb61b..370537ed38 100644
--- a/tools/libs/light/Makefile
+++ b/tools/libs/light/Makefile
@@ -203,9 +203,9 @@ _libxl.api-for-check: $(XEN_INCLUDE)/libxl.h $(AUTOINCS)
 		>$@.new
 	mv -f $@.new $@
 
-$(XEN_INCLUDE)/_libxl_list.h: $(XEN_INCLUDE)/xen-external/bsd-sys-queue-h-seddery $(XEN_INCLUDE)/xen-external/bsd-sys-queue.h
-	$(PERL) $^ --prefix=libxl >$(notdir $@).new
-	$(call move-if-changed,$(notdir $@).new,$@)
+_libxl_list.h: $(XEN_INCLUDE)/xen-external/bsd-sys-queue-h-seddery $(XEN_INCLUDE)/xen-external/bsd-sys-queue.h
+	$(PERL) $^ --prefix=libxl >$@.new
+	$(call move-if-changed,$@.new,$@)
 
 _libxl_save_msgs_helper.c _libxl_save_msgs_callout.c \
 _libxl_save_msgs_helper.h _libxl_save_msgs_callout.h: \
-- 
2.26.2


Re: [PATCH] tools/libs/light: fix race in Makefile
Posted by Jan Beulich 3 years, 6 months ago
On 25.10.2020 11:12, Juergen Gross wrote:
> The header $(INCLUDE)/_lixl_list.h matches two different rules, which
> can result in build breakage. Fix that.

While I don't doubt you having observed a race, I'm not sure this is
true, and hence I'm also not sure the change is going to address it:
Aiui the two rules you talk about are the one you change and

$(XEN_INCLUDE)/_%.h: _%.h
	$(call move-if-changed,_$*.h,$(XEN_INCLUDE)/_$*.h)

But a pattern rule doesn't come into play when a specific rule for
a file exists.

What I don't understand here is why this two step moving around of
headers is used: Instead of the above pattern rule, can't the rule
to generate _libxl_type%.h, _libxl_type%_json.h,
_libxl_type%_private.h, and _libxl_type%.c put the relevant header
files right into their designated place? This would allow the
pattern rule to go away, albeit I'd then still be unclear about
the specific race you did observe.

Jan

> Signed-off-by: Juergen Gross <jgross@suse.com>
> ---
>  tools/libs/light/Makefile | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/tools/libs/light/Makefile b/tools/libs/light/Makefile
> index 3424fdb61b..370537ed38 100644
> --- a/tools/libs/light/Makefile
> +++ b/tools/libs/light/Makefile
> @@ -203,9 +203,9 @@ _libxl.api-for-check: $(XEN_INCLUDE)/libxl.h $(AUTOINCS)
>  		>$@.new
>  	mv -f $@.new $@
>  
> -$(XEN_INCLUDE)/_libxl_list.h: $(XEN_INCLUDE)/xen-external/bsd-sys-queue-h-seddery $(XEN_INCLUDE)/xen-external/bsd-sys-queue.h
> -	$(PERL) $^ --prefix=libxl >$(notdir $@).new
> -	$(call move-if-changed,$(notdir $@).new,$@)
> +_libxl_list.h: $(XEN_INCLUDE)/xen-external/bsd-sys-queue-h-seddery $(XEN_INCLUDE)/xen-external/bsd-sys-queue.h
> +	$(PERL) $^ --prefix=libxl >$@.new
> +	$(call move-if-changed,$@.new,$@)
>  
>  _libxl_save_msgs_helper.c _libxl_save_msgs_callout.c \
>  _libxl_save_msgs_helper.h _libxl_save_msgs_callout.h: \
> 


Re: [PATCH] tools/libs/light: fix race in Makefile
Posted by Jürgen Groß 3 years, 6 months ago
On 26.10.20 10:34, Jan Beulich wrote:
> On 25.10.2020 11:12, Juergen Gross wrote:
>> The header $(INCLUDE)/_lixl_list.h matches two different rules, which
>> can result in build breakage. Fix that.
> 
> While I don't doubt you having observed a race, I'm not sure this is
> true, and hence I'm also not sure the change is going to address it:
> Aiui the two rules you talk about are the one you change and
> 
> $(XEN_INCLUDE)/_%.h: _%.h
> 	$(call move-if-changed,_$*.h,$(XEN_INCLUDE)/_$*.h)
> 
> But a pattern rule doesn't come into play when a specific rule for
> a file exists.

Hmm, true. I didn't see the race, but spotted the suspected ambiguity
just by chance.

> 
> What I don't understand here is why this two step moving around of
> headers is used: Instead of the above pattern rule, can't the rule
> to generate _libxl_type%.h, _libxl_type%_json.h,
> _libxl_type%_private.h, and _libxl_type%.c put the relevant header
> files right into their designated place? This would allow the
> pattern rule to go away, albeit I'd then still be unclear about
> the specific race you did observe.

This would require to replace the pattern rules used to generate the
files by per-file rules instead, as e.g. _libxl_types_json.h and
_libxl_types_internal_json.h are matching the same pattern, but they
need to end up in different directories.

In the end I think this patch can just be dropped.

Sorry for the noise,

Juergen

Re: [PATCH] tools/libs/light: fix race in Makefile
Posted by Jan Beulich 3 years, 6 months ago
On 26.10.2020 10:46, Jürgen Groß wrote:
> On 26.10.20 10:34, Jan Beulich wrote:
>> What I don't understand here is why this two step moving around of
>> headers is used: Instead of the above pattern rule, can't the rule
>> to generate _libxl_type%.h, _libxl_type%_json.h,
>> _libxl_type%_private.h, and _libxl_type%.c put the relevant header
>> files right into their designated place? This would allow the
>> pattern rule to go away, albeit I'd then still be unclear about
>> the specific race you did observe.
> 
> This would require to replace the pattern rules used to generate the
> files by per-file rules instead, as e.g. _libxl_types_json.h and
> _libxl_types_internal_json.h are matching the same pattern, but they
> need to end up in different directories.

Ah, right - I didn't pay attention to the *_internal*.h needs.

Jan