Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Message-ID: <e62f481d-f939-a12b-7f8a-ec1db95c7b8e@gmail.com>
Date:   Fri, 14 Jul 2023 20:57:21 +0700
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:102.0) Gecko/20100101
 Thunderbird/102.13.0
Subject: Re: [PATCH] tools/counter: Add checking directory exists for make
 clean
To:     William Breathitt Gray <william.gray@linaro.org>
Cc:     linux-kernel-mentees@lists.linuxfoundation.org, rongtao@cestc.cn,
        ricardo@pardini.net, linux-iio@vger.kernel.org,
        linux-kernel@vger.kernel.org
References: <362e127d-6018-5fc6-247b-3c729b99d946@gmail.com>
 <ZKtZ1WB9LdszbxU+@fedora>
Content-Language: en-US
From:   Anh Tuan Phan <tuananhlfc@gmail.com>
In-Reply-To: <ZKtZ1WB9LdszbxU+@fedora>
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 7bit
Precedence: bulk

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@gmail.com>
>> ---
>>  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@vger.kernel.org> and <linux-kernel@vger.kernel.org>
> 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