Re: [PATCH net-next v4] net: netconsole: selftests: Create a new netconsole selftest

[Breno Leitao <leitao@debian.org>]

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@debian.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

I will send a v5 with `test -x`, I will just wait for more reviews.

Thanks!