Re: [PATCH] tools/counter: Add checking directory exists for make clean

[Anh Tuan Phan]

Hi William, the race condition possibility is a good point. Thanks for
suggesting me. Will send a new version with using `rm -df` instead and
add the appropriate CC lists. Thank you!

On 7/10/23 08:07, William Breathitt Gray wrote:
> On Fri, Jul 07, 2023 at 09:08:35PM +0700, Anh Tuan Phan wrote:
>> rmdir requires the directory exist so it causes "make -C tools clean"
>> failed if someone only builds other tools but not counter. This commit
>> adds checking the directory exist before doing rmdir.
>>
>> Signed-off-by: Anh Tuan Phan <tuananhlfc@xxxxxxxxx>
>> ---
>>  tools/counter/Makefile | 4 +++-
>>  1 file changed, 3 insertions(+), 1 deletion(-)
>>
>> diff --git a/tools/counter/Makefile b/tools/counter/Makefile
>> index a0f4cab71fe5..2907f3b3094b 100644
>> --- a/tools/counter/Makefile
>> +++ b/tools/counter/Makefile
>> @@ -40,7 +40,9 @@ $(OUTPUT)counter_example: $(COUNTER_EXAMPLE)
>>  clean:
>>  	rm -f $(ALL_PROGRAMS)
>>  	rm -rf $(OUTPUT)include/linux/counter.h
>> -	rmdir -p $(OUTPUT)include/linux
>> +	@if [ -d $(OUTPUT)include/linux ]; then \
>> +		rmdir -p $(OUTPUT)include/linux; \
>> +	fi
>>  	find $(or $(OUTPUT),.) -name '*.o' -delete -o -name '\.*.d' -delete
>>
>>  install: $(ALL_PROGRAMS)
>> -- 
>> 2.34.1
> 
> Hi Anh,
> 
> Please CC <linux-iio@xxxxxxxxxxxxxxx> and <linux-kernel@xxxxxxxxxxxxxxx>
> as well in the future so Counter users and developers can become aware
> of relevant patches.
> 
> One worry I have with this approach is the possible race condition where
> the check for existence succeeds but the directory is deleted by another
> agent before our rmdir executes. However, I'm not sure how we could
> achieve such behavior atomically to prevent the issue.
> 
> One alternative I've considered is perhaps a single find command to
> search for and delete empty directories:
> 
>     find $(or $(OUTPUT),.) -type d -empty -delete
> 
> But this will delete directories not created by the makefile which I
> consider an unexpected behavior for the user (or at least very rude of
> the script to do).
> 
> Perhaps we should delete the directory tree explicitly:
> 
>     rm -df $(OUTPUT)include/linux
>     rm -df $(OUTPUT)include
> 
> Although we lose the symmetry of rmdir to our previous mkdir, this
> should prevent the race condition issue and succeed whether the
> directories still exist or not.
> 
> William Breathitt Gray