[PATCH] localedata: Avoid concurrently written locales in gen-locale.sh

[Adhemerval Zanella Netto]

On 08/07/26 09:18, Sam James wrote:
> Florian Weimer <fweimer@redhat.com> writes:
> 
>> There is no cross-directory exclusion of concurrent $(gen-locales)
>> usage.  Parallel localedef calls can clobber locale data as it is
>> being loaded by tests.
>>
>> With --no-hard-links, the separate touch invocation is no longer
>> required.
>>
>> ---
>>  localedata/gen-locale.sh | 22 +++++++++++++++-------
>>  1 file changed, 15 insertions(+), 7 deletions(-)
>>
>> diff --git a/localedata/gen-locale.sh b/localedata/gen-locale.sh
>> index 4762c04284..b0ce27fbae 100644
>> --- a/localedata/gen-locale.sh
>> +++ b/localedata/gen-locale.sh
>> @@ -32,15 +32,23 @@ generate_locale ()
>>    out=$3
>>    flags=$4
>>    ret=0
>> +
>> +  # Use a staging area to avoid writing to locales concurrently.
>> +  # While this process is running, $$ is sufficiently unique.
> 
> Yes, this should be OK.
> 
>> +  # Use --no-hard-links to prevent localedef from accessing
>> +  # other staging areas.
>> +  stage="${common_objpfx}localedata/gen-locale.$$.tmp"
>> +
>>    ${localedef_before_env} ${run_program_env} I18NPATH=../localedata \
>> -	${localedef_after_env} $flags -f $charmap -i $input \
>> -	${common_objpfx}localedata/$out || ret=$?
>> -  if [ $ret -eq 0 ]; then
>> -    # The makefile checks the timestamp of the LC_CTYPE file,
>> -    # but localedef won't have touched it if it was able to
>> -    # hard-link it to an existing file.
>> -    touch ${common_objpfx}localedata/$out/LC_CTYPE
>> +	${localedef_after_env} $flags --no-hard-links -f $charmap -i $input \
>> +	$stage || ret=$?
> 
> This could be improved to directly test the command but it can be done
> later, no need to do it now.

Agree, I think we should fix for the release and this approach seems ok.

> 
>> +  if [ $ret -eq 0 ] ; then
>> +      # Ignore errors in case some other process has created the same
>> locale.
> 
> OK. 
> 
>> +      # (Some versions of mv do not report an error here.)
>> +      mv -Tn $stage ${common_objpfx}localedata/$out 2>/dev/null || true
> 
> I'm sorry to ask this, but is mv -T sufficiently portable? It's not
> POSIX but I don't recall if we require any extensions for coreutils
> elsewhere in glibc.

Hum I was not aware of that, maybe we can adjust it after the release if
required.

> 
>> +      rm -rf $stage
>>    else
>> +    rm -rf $stage
>>      echo "Charmap: \"${charmap}\" Inputfile: \"${input}\"" \
>>  	 "Outputdir: \"${out}\" failed"
>>      exit 1
> 
> The patch otherwise looks good, but please wait a brief period for
> Adhemerval to comment before committing.
Looks good to me. Just curious Florian, how did you notice this issue?