[PATCH v2 0/2] tools/x86: adjust populating of tools/include/xen/

[PATCH v2 0/2] tools/x86: adjust populating of tools/include/xen/

[Jan Beulich]

While the first change is a possible 4.15 candidate, the second is pure
cleanup (but could, should patch 1 end up being controversial, also go
in independently).

1: don't rebuild cpuid-autogen.h every time
2: move arch-specific include/xen/ population into arch-specific rule

Jan

[PATCH v2 1/2][4.15] tools/x86: don't rebuild cpuid-autogen.h every time

[Jan Beulich]

The first thing the "xen-dir" rule does is delete the entire xen/
subtree. Obviously this includes deleting xen/lib/x86/*autogen.h. As a
result there's no original version for $(move-if-changed ...) to compare
against, and hence the file and all its consumers would get rebuilt
every time. Instead only find and delete all the symlinks.

Fixes: eddf9559c977 ("libx86: generate cpuid-autogen.h in the libx86 include dir")
Signed-off-by: Jan Beulich <jbeulich@suse.com>
---
v2: Different approach.
---
Ian did suggest to pass -0r to xargs (and -print0 to find), but I
couldn't convince myself that these are standard compliant options. We
don't use any special characters in file names, so -print0 / -0
shouldn't be necessary at all. The stray rm invocation when there is no
output from find can be taken care of by passing -f to it.

--- a/tools/include/Makefile
+++ b/tools/include/Makefile
@@ -19,7 +19,7 @@ xen-foreign:
 	$(MAKE) -C xen-foreign
 
 xen-dir:
-	@rm -rf xen acpi
+	find xen/ acpi/ -type l 2>/dev/null | xargs rm -f --
 	mkdir -p xen/libelf acpi
 	ln -s $(XEN_ROOT)/xen/include/public/COPYING xen/
 	ln -s $(XEN_ROOT)/xen/include/public/*.h xen/

[PATCH v2 2/2] tools/x86: move arch-specific include/xen/ population into arch-specific rule

[Jan Beulich]

There's no need for the common "xen-dir" rule to have an arch-specific
part when there already is a arch-specific rule where this can be taken
care of (arguably instead of all-y this might better be xendir-y
anyway).

Also change the pattern from *autogen.h to *-autogen.h.

Signed-off-by: Jan Beulich <jbeulich@suse.com>
---
v2: Insert dash in pattern. Re-base over changed earlier patch.
---
I was tempted to move the "xen-dir" dependency from all-$(CONFIG_X86) to
the general all-y, but perhaps that's less related and hence should
really be a separate change (and there may also be arguments against
that).

--- a/tools/include/Makefile
+++ b/tools/include/Makefile
@@ -29,14 +29,12 @@ xen-dir:
 	ln -s ../xen-foreign xen/foreign
 	ln -s $(XEN_ROOT)/xen/include/acpi/platform acpi/
 	ln -s $(XEN_ROOT)/xen/include/acpi/ac*.h acpi/
-ifeq ($(CONFIG_X86),y)
+
+all-$(CONFIG_X86): xen-dir
 	ln -s $(XEN_ROOT)/xen/include/asm-x86 xen/asm
 	mkdir -p xen/lib/x86
-	ln -s $(filter-out %autogen.h,$(wildcard $(XEN_ROOT)/xen/include/xen/lib/x86/*.h)) xen/lib/x86/
+	ln -s $(filter-out %-autogen.h,$(wildcard $(XEN_ROOT)/xen/include/xen/lib/x86/*.h)) xen/lib/x86/
 	ln -s $(XEN_ROOT)/xen/include/xen/lib/x86/Makefile xen/lib/x86/
-endif
-
-all-$(CONFIG_X86): xen-dir
 	$(MAKE) -C xen/lib/x86 all XEN_ROOT=$(XEN_ROOT) PYTHON=$(PYTHON)
 
 # Not xen/xsm as that clashes with link to

Re: [PATCH v2 1/2][4.15] tools/x86: don't rebuild cpuid-autogen.h every time

[Roger Pau Monné]

On Thu, Mar 11, 2021 at 03:40:05PM +0100, Jan Beulich wrote:
> The first thing the "xen-dir" rule does is delete the entire xen/
> subtree. Obviously this includes deleting xen/lib/x86/*autogen.h. As a
> result there's no original version for $(move-if-changed ...) to compare
> against, and hence the file and all its consumers would get rebuilt
> every time. Instead only find and delete all the symlinks.
> 
> Fixes: eddf9559c977 ("libx86: generate cpuid-autogen.h in the libx86 include dir")
> Signed-off-by: Jan Beulich <jbeulich@suse.com>
> ---
> v2: Different approach.
> ---
> Ian did suggest to pass -0r to xargs (and -print0 to find), but I
> couldn't convince myself that these are standard compliant options. We
> don't use any special characters in file names, so -print0 / -0
> shouldn't be necessary at all. The stray rm invocation when there is no
> output from find can be taken care of by passing -f to it.

Why not use `-exec rm -f {} +` instead? That seems to be part of
POSIX and is likely nicer than piping to xargs?

> 
> --- a/tools/include/Makefile
> +++ b/tools/include/Makefile
> @@ -19,7 +19,7 @@ xen-foreign:
>  	$(MAKE) -C xen-foreign
>  
>  xen-dir:
> -	@rm -rf xen acpi
> +	find xen/ acpi/ -type l 2>/dev/null | xargs rm -f --

Do we care about leaving an empty xen/libelf directory behind?

Thanks, Roger.

Re: [PATCH v2 1/2][4.15] tools/x86: don't rebuild cpuid-autogen.h every time

[Jan Beulich]

On 11.03.2021 16:29, Roger Pau Monné wrote:
> On Thu, Mar 11, 2021 at 03:40:05PM +0100, Jan Beulich wrote:
>> The first thing the "xen-dir" rule does is delete the entire xen/
>> subtree. Obviously this includes deleting xen/lib/x86/*autogen.h. As a
>> result there's no original version for $(move-if-changed ...) to compare
>> against, and hence the file and all its consumers would get rebuilt
>> every time. Instead only find and delete all the symlinks.
>>
>> Fixes: eddf9559c977 ("libx86: generate cpuid-autogen.h in the libx86 include dir")
>> Signed-off-by: Jan Beulich <jbeulich@suse.com>
>> ---
>> v2: Different approach.
>> ---
>> Ian did suggest to pass -0r to xargs (and -print0 to find), but I
>> couldn't convince myself that these are standard compliant options. We
>> don't use any special characters in file names, so -print0 / -0
>> shouldn't be necessary at all. The stray rm invocation when there is no
>> output from find can be taken care of by passing -f to it.
> 
> Why not use `-exec rm -f {} +` instead? That seems to be part of
> POSIX and is likely nicer than piping to xargs?

Hmm, I avoided it because I was under the impression that there
are (compatibility) issues with it, and Ian suggesting xargs
seemed to support that. I'd be more than happy to avoid xargs,
of which I've never been a friend.

>> --- a/tools/include/Makefile
>> +++ b/tools/include/Makefile
>> @@ -19,7 +19,7 @@ xen-foreign:
>>  	$(MAKE) -C xen-foreign
>>  
>>  xen-dir:
>> -	@rm -rf xen acpi
>> +	find xen/ acpi/ -type l 2>/dev/null | xargs rm -f --
> 
> Do we care about leaving an empty xen/libelf directory behind?

Why would we? It'll get created immediately afterwards if it's
not there, and it'll initially be empty (not for long of course).

Jan

Re: [PATCH v2 1/2][4.15] tools/x86: don't rebuild cpuid-autogen.h every time

[Roger Pau Monné]

On Thu, Mar 11, 2021 at 05:00:12PM +0100, Jan Beulich wrote:
> On 11.03.2021 16:29, Roger Pau Monné wrote:
> > On Thu, Mar 11, 2021 at 03:40:05PM +0100, Jan Beulich wrote:
> >> The first thing the "xen-dir" rule does is delete the entire xen/
> >> subtree. Obviously this includes deleting xen/lib/x86/*autogen.h. As a
> >> result there's no original version for $(move-if-changed ...) to compare
> >> against, and hence the file and all its consumers would get rebuilt
> >> every time. Instead only find and delete all the symlinks.
> >>
> >> Fixes: eddf9559c977 ("libx86: generate cpuid-autogen.h in the libx86 include dir")
> >> Signed-off-by: Jan Beulich <jbeulich@suse.com>
> >> ---
> >> v2: Different approach.
> >> ---
> >> Ian did suggest to pass -0r to xargs (and -print0 to find), but I
> >> couldn't convince myself that these are standard compliant options. We
> >> don't use any special characters in file names, so -print0 / -0
> >> shouldn't be necessary at all. The stray rm invocation when there is no
> >> output from find can be taken care of by passing -f to it.
> > 
> > Why not use `-exec rm -f {} +` instead? That seems to be part of
> > POSIX and is likely nicer than piping to xargs?
> 
> Hmm, I avoided it because I was under the impression that there
> are (compatibility) issues with it, and Ian suggesting xargs
> seemed to support that. I'd be more than happy to avoid xargs,
> of which I've never been a friend.

All I can tell is that '-exec ... {} +'  is part of POSIX [0], and I
can confirm it works on FreeBSD. I have a slight preference for -exec
instead of xargs because I think it's cleaner, but I think your
current one is correct, so:

Reviewed-by: Roger Pau Monné <roger.pau@citrix.com>

For either one.

> >> --- a/tools/include/Makefile
> >> +++ b/tools/include/Makefile
> >> @@ -19,7 +19,7 @@ xen-foreign:
> >>  	$(MAKE) -C xen-foreign
> >>  
> >>  xen-dir:
> >> -	@rm -rf xen acpi
> >> +	find xen/ acpi/ -type l 2>/dev/null | xargs rm -f --
> > 
> > Do we care about leaving an empty xen/libelf directory behind?
> 
> Why would we? It'll get created immediately afterwards if it's
> not there, and it'll initially be empty (not for long of course).

Right, also the 'clean' target will still rm the whole directory.

Thanks, Roger.

[0] https://pubs.opengroup.org/onlinepubs/9699919799/utilities/find.html

Re: [PATCH v2 1/2][4.15] tools/x86: don't rebuild cpuid-autogen.h every time

[Jan Beulich]

On 11.03.2021 16:29, Roger Pau Monné wrote:
> On Thu, Mar 11, 2021 at 03:40:05PM +0100, Jan Beulich wrote:
>> The first thing the "xen-dir" rule does is delete the entire xen/
>> subtree. Obviously this includes deleting xen/lib/x86/*autogen.h. As a
>> result there's no original version for $(move-if-changed ...) to compare
>> against, and hence the file and all its consumers would get rebuilt
>> every time. Instead only find and delete all the symlinks.
>>
>> Fixes: eddf9559c977 ("libx86: generate cpuid-autogen.h in the libx86 include dir")
>> Signed-off-by: Jan Beulich <jbeulich@suse.com>
>> ---
>> v2: Different approach.
>> ---
>> Ian did suggest to pass -0r to xargs (and -print0 to find), but I
>> couldn't convince myself that these are standard compliant options. We
>> don't use any special characters in file names, so -print0 / -0
>> shouldn't be necessary at all. The stray rm invocation when there is no
>> output from find can be taken care of by passing -f to it.
> 
> Why not use `-exec rm -f {} +` instead? That seems to be part of
> POSIX and is likely nicer than piping to xargs?

I've looked around some more and also remembered an aspect because
of the looking around.

1) xargs has special provisions to not overrun argument space (leaving
a gap of 2048 bytes). Nothing like this is said in the spec for find,
and in fact I recall having had issues when the accumulated arguments
needed splitting. This was on an old distro, but I wanted to mention
it.

2) I've found (old) indications that the {} may be troublesome to
some distros (Solaris was mentioned) and might need quoting. (This
would of course be easy to deal with.)

Jan

Re: [PATCH v2 1/2][4.15] tools/x86: don't rebuild cpuid-autogen.h every time

[Roger Pau Monné]

On Fri, Mar 12, 2021 at 09:45:35AM +0100, Jan Beulich wrote:
> On 11.03.2021 16:29, Roger Pau Monné wrote:
> > On Thu, Mar 11, 2021 at 03:40:05PM +0100, Jan Beulich wrote:
> >> The first thing the "xen-dir" rule does is delete the entire xen/
> >> subtree. Obviously this includes deleting xen/lib/x86/*autogen.h. As a
> >> result there's no original version for $(move-if-changed ...) to compare
> >> against, and hence the file and all its consumers would get rebuilt
> >> every time. Instead only find and delete all the symlinks.
> >>
> >> Fixes: eddf9559c977 ("libx86: generate cpuid-autogen.h in the libx86 include dir")
> >> Signed-off-by: Jan Beulich <jbeulich@suse.com>
> >> ---
> >> v2: Different approach.
> >> ---
> >> Ian did suggest to pass -0r to xargs (and -print0 to find), but I
> >> couldn't convince myself that these are standard compliant options. We
> >> don't use any special characters in file names, so -print0 / -0
> >> shouldn't be necessary at all. The stray rm invocation when there is no
> >> output from find can be taken care of by passing -f to it.
> > 
> > Why not use `-exec rm -f {} +` instead? That seems to be part of
> > POSIX and is likely nicer than piping to xargs?
> 
> I've looked around some more and also remembered an aspect because
> of the looking around.
> 
> 1) xargs has special provisions to not overrun argument space (leaving
> a gap of 2048 bytes). Nothing like this is said in the spec for find,
> and in fact I recall having had issues when the accumulated arguments
> needed splitting. This was on an old distro, but I wanted to mention
> it.

Oh, the POSIX spec does note this:

"The size of any set of two or more pathnames shall be limited such
that execution of the utility does not cause the system's {ARG_MAX}
limit to be exceeded."

I would argue that not doing this is a bug.

> 2) I've found (old) indications that the {} may be troublesome to
> some distros (Solaris was mentioned) and might need quoting. (This
> would of course be easy to deal with.)

Hm, OK. I won't insist then. I'm not sure we should still consider
Solaris as supported anyway. It's likely to explode in a lot of ways
when trying to build.

Do you really need the stderr redirection to /dev/null of find output
when using xargs though? As that will just drop error messages, making
failure diagnostic harder.

Thanks, Roger.

Re: [PATCH v2 1/2][4.15] tools/x86: don't rebuild cpuid-autogen.h every time

[Jan Beulich]

On 12.03.2021 10:17, Roger Pau Monné wrote:
> On Fri, Mar 12, 2021 at 09:45:35AM +0100, Jan Beulich wrote:
>> On 11.03.2021 16:29, Roger Pau Monné wrote:
>>> On Thu, Mar 11, 2021 at 03:40:05PM +0100, Jan Beulich wrote:
>>>> The first thing the "xen-dir" rule does is delete the entire xen/
>>>> subtree. Obviously this includes deleting xen/lib/x86/*autogen.h. As a
>>>> result there's no original version for $(move-if-changed ...) to compare
>>>> against, and hence the file and all its consumers would get rebuilt
>>>> every time. Instead only find and delete all the symlinks.
>>>>
>>>> Fixes: eddf9559c977 ("libx86: generate cpuid-autogen.h in the libx86 include dir")
>>>> Signed-off-by: Jan Beulich <jbeulich@suse.com>
>>>> ---
>>>> v2: Different approach.
>>>> ---
>>>> Ian did suggest to pass -0r to xargs (and -print0 to find), but I
>>>> couldn't convince myself that these are standard compliant options. We
>>>> don't use any special characters in file names, so -print0 / -0
>>>> shouldn't be necessary at all. The stray rm invocation when there is no
>>>> output from find can be taken care of by passing -f to it.
>>>
>>> Why not use `-exec rm -f {} +` instead? That seems to be part of
>>> POSIX and is likely nicer than piping to xargs?
>>
>> I've looked around some more and also remembered an aspect because
>> of the looking around.
>>
>> 1) xargs has special provisions to not overrun argument space (leaving
>> a gap of 2048 bytes). Nothing like this is said in the spec for find,
>> and in fact I recall having had issues when the accumulated arguments
>> needed splitting. This was on an old distro, but I wanted to mention
>> it.
> 
> Oh, the POSIX spec does note this:
> 
> "The size of any set of two or more pathnames shall be limited such
> that execution of the utility does not cause the system's {ARG_MAX}
> limit to be exceeded."

Well, yes. Hence my referral to the extra precautions in xargs:

"The xargs utility shall limit the command line length such that
 when the command line is invoked, the combined argument and
 environment lists (see the exec family of functions in the
 System Interfaces volume of IEEE Std 1003.1-2001) shall not
 exceed {ARG_MAX}-2048 bytes."

> I would argue that not doing this is a bug.

I agree, of course.

>> 2) I've found (old) indications that the {} may be troublesome to
>> some distros (Solaris was mentioned) and might need quoting. (This
>> would of course be easy to deal with.)
> 
> Hm, OK. I won't insist then. I'm not sure we should still consider
> Solaris as supported anyway. It's likely to explode in a lot of ways
> when trying to build.

Right, I understand Solaris isn't the best example. I'd like to
see what Ian says towards either of the options being better
than the other.

> Do you really need the stderr redirection to /dev/null of find output
> when using xargs though? As that will just drop error messages, making
> failure diagnostic harder.

Well, yes, upon first build there would be error messages
otherwise for xen/ and acpi/ not existing (yet).

Jan

Re: [PATCH v2 1/2][4.15] tools/x86: don't rebuild cpuid-autogen.h every time

[Ian Jackson]

Jan Beulich writes ("[PATCH v2 1/2][4.15] tools/x86: don't rebuild cpuid-autogen.h every time"):
> Ian did suggest to pass -0r to xargs (and -print0 to find), but I
> couldn't convince myself that these are standard compliant options. We
> don't use any special characters in file names, so -print0 / -0
> shouldn't be necessary at all. The stray rm invocation when there is no
> output from find can be taken care of by passing -f to it.

I addressed these portability questions in my original mail.

I said that I had checked FreeBSD find and xargs have all these
options.  I didn't check the other *BSDs but I see no reason to think
they would be different.

find -print0 and xargs -0 are not in SuS (for bad reasons) but these
are obviously necessary (unless one thinks people are supposed to use
  -exec printf "%s\n" '{}' \;
when the output is going to something more complicated an just some
command's arguments.

IOW we are IMO find to rely on both these options.

I would be OK with find -exec + in this case.  The {} must be quoted.

> --- a/tools/include/Makefile
> +++ b/tools/include/Makefile
> @@ -19,7 +19,7 @@ xen-foreign:
>  	$(MAKE) -C xen-foreign
>  
>  xen-dir:
> -	@rm -rf xen acpi
> +	find xen/ acpi/ -type l 2>/dev/null | xargs rm -f --

I disapprove of the 2>/dev/null and the -f because I disapprove of
suppressing.

Ian.

Re: [PATCH v2 2/2] tools/x86: move arch-specific include/xen/ population into arch-specific rule

[Ian Jackson]

Jan Beulich writes ("[PATCH v2 2/2] tools/x86: move arch-specific include/xen/ population into arch-specific rule"):
> There's no need for the common "xen-dir" rule to have an arch-specific
> part when there already is a arch-specific rule where this can be taken
> care of (arguably instead of all-y this might better be xendir-y
> anyway).

Thanks, but I do not like this change.

IMO make commands should be primarily organised by their logical
function and role in the file.  If that means that multiple targets
have arch-specific parts, so be it.

IMO it is not right to move something from xen-dir to all just to
reduce the number of different targets with arch-specific parts.

Ian.

[PATCH v3][4.15] tools/x86: don't rebuild cpuid-autogen.h every time

[Jan Beulich]

The first thing the "xen-dir" rule does is delete the entire xen/
subtree. Obviously this includes deleting xen/lib/x86/*autogen.h. As a
result there's no original version for $(move-if-changed ...) to compare
against, and hence the file and all its consumers would get rebuilt
every time. Instead only find and delete all the symlinks.

Fixes: eddf9559c977 ("libx86: generate cpuid-autogen.h in the libx86 include dir")
Signed-off-by: Jan Beulich <jbeulich@suse.com>
---
v3: Don't use xargs. Don't hide errors.
v2: Different approach.

--- a/tools/include/Makefile
+++ b/tools/include/Makefile
@@ -19,8 +19,8 @@ xen-foreign:
 	$(MAKE) -C xen-foreign
 
 xen-dir:
-	@rm -rf xen acpi
 	mkdir -p xen/libelf acpi
+	find xen/ acpi/ -type l -exec rm '{}' +
 	ln -s $(XEN_ROOT)/xen/include/public/COPYING xen/
 	ln -s $(XEN_ROOT)/xen/include/public/*.h xen/
 	ln -s $(XEN_ROOT)/xen/include/public/*/ xen/

Re: [PATCH v3][4.15] tools/x86: don't rebuild cpuid-autogen.h every time

[Ian Jackson]

Jan Beulich writes ("[PATCH v3][4.15] tools/x86: don't rebuild cpuid-autogen.h every time"):
> The first thing the "xen-dir" rule does is delete the entire xen/
> subtree. Obviously this includes deleting xen/lib/x86/*autogen.h. As a
> result there's no original version for $(move-if-changed ...) to compare
> against, and hence the file and all its consumers would get rebuilt
> every time. Instead only find and delete all the symlinks.
> 
> Fixes: eddf9559c977 ("libx86: generate cpuid-autogen.h in the libx86 include dir")
> Signed-off-by: Jan Beulich <jbeulich@suse.com>

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

Thanks!