[<prev] [next>] [<thread-prev] [day] [month] [year] [list]
Message-ID: <0f48b0d1-f8fd-4e2f-b773-15e6d5e4ae95@kernel.org>
Date: Fri, 16 Aug 2024 17:08:40 +0200
From: Matthieu Baerts <matttbe@...nel.org>
To: Breno Leitao <leitao@...ian.org>
Cc: davem@...emloft.net, edumazet@...gle.com, kuba@...nel.org,
pabeni@...hat.com, liuhangbin@...il.com, petrm@...dia.com,
Shuah Khan <shuah@...nel.org>, netdev@...r.kernel.org,
Willem de Bruijn <willemb@...gle.com>, David Wei <dw@...idwei.uk>,
open list <linux-kernel@...r.kernel.org>,
"open list:KERNEL SELFTEST FRAMEWORK" <linux-kselftest@...r.kernel.org>
Subject: Re: [PATCH net-next v4] net: netconsole: selftests: Create a new
netconsole selftest
On 16/08/2024 16:50, Breno Leitao wrote:
> On Fri, Aug 16, 2024 at 04:02:51PM +0200, Matthieu Baerts wrote:
>> Hi Breno,
>>
>> On 16/08/2024 15:24, Breno Leitao wrote:
>>> Adds a selftest that creates two virtual interfaces, assigns one to a
>>> new namespace, and assigns IP addresses to both.
>>>
>>> It listens on the destination interface using socat and configures a
>>> dynamic target on netconsole, pointing to the destination IP address.
>>>
>>> The test then checks if the message was received properly on the
>>> destination interface.
>>>
>>> Signed-off-by: Breno Leitao <leitao@...ian.org>
>>> ---
>>> Changelog:
>>>
>>> v4:
>>> * Avoid sleeping in waiting for sockets and files (Matthieu Baerts)
>>> * Some other improvements (Matthieu Baerts)
>>> * Add configfs as a dependency (Jakub)
>>
>> Thank you for the new version!
>>
>> It looks good to me, but again, my review mainly focused on the
>> Bash-related stuff, not on the netconsole test itself.
>>
>> I just have one question below, but not blocking.
>>
>> (...)
>>
>>> diff --git a/tools/testing/selftests/drivers/net/netcons_basic.sh b/tools/testing/selftests/drivers/net/netcons_basic.sh
>>> new file mode 100755
>>> index 000000000000..5c3686af1fe8
>>> --- /dev/null
>>> +++ b/tools/testing/selftests/drivers/net/netcons_basic.sh
>>> @@ -0,0 +1,249 @@
>>
>> (...)
>>
>>> +check_file_size() {
>>> + local file="$1"
>>> +
>>> + if [[ ! -f "$file" ]]; then
>>> + # File might not exist yet
>>> + return 1
>>> + fi
>>> +
>>> + # Get file size
>>> + local size=$(stat -c %s "$file" 2>/dev/null)
>>> + # Check if stat command succeeded
>>> + if [[ $? -ne 0 ]]; then
>>> + return 1
>>> + fi
>>> +
>>> + # Check if size is greater than zero
>>> + if [[ "$size" -gt 0 ]]; then
>>> + return 0 # file size > 0
>>> + else
>>> + return 1 # file size == 0
>>> + fi
>>> +}
>>
>> (...)
>>
>>> +# Wait until socat saves the file to disk
>>> +busywait "${BUSYWAIT_TIMEOUT}" check_file_size "${OUTPUT_FILE}"
>>
>> It looks like your 'check_file_size' helper is a reimplementation of
>> 'test -s <FILE>', no? Can you not simply use:
>
> Why would you like to do it in one line when you can write a 15-lines
> function that does exactly the same!? :-P
True :-P
> I will send a v5 with `test -x`, I will just wait for more reviews.
Thanks! Indeed, better to wait at least 24h between submissions.
(Also, I guess you meant to write `test -s`, not `-x`.)
Cheers,
Matt
--
Sponsored by the NGI0 Core fund.
Powered by blists - more mailing lists