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

[Jan Beulich <jbeulich@xxxxxxxx>]

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@xxxxxxxx>
>>>> ---
>>>> 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