Re: git: aa3860851b9f - main - net: Remove unneeded NULL check for the allocated ifnet

> On Jun 28, 2024, at 6:38 PM, Poul-Henning Kamp <phk@phk.freebsd.dk> wrote:
> 
> --------
> Zhenlei Huang writes:
> 
>> commit aa3860851b9f6a6002d135b1cac7736e0995eedc
>> 
>>    net: Remove unneeded NULL check for the allocated ifnet
>> 
>>    Change 4787572d0580 made if_alloc_domain() never fail, then also do the
>>    wrappers if_alloc(), if_alloc_dev(), and if_gethandle().
>> 
>> [...]
>> diff --git a/sys/arm/allwinner/if_emac.c b/sys/arm/allwinner/if_emac.c
>> index f581d361d3d9..1db43cbca26c 100644
>> --- a/sys/arm/allwinner/if_emac.c
>> +++ b/sys/arm/allwinner/if_emac.c
>> @@ -940,11 +940,6 @@ emac_attach(device_t dev)
>> 	emac_reset(sc);
>> 
>> 	ifp = sc->emac_ifp = if_alloc(IFT_ETHER);
>> -	if (ifp == NULL) {
>> -		device_printf(dev, "unable to allocate ifp\n");
>> -		error = ENOSPC;
>> -		goto fail;
>> -	}
>> 	if_setsoftc(ifp, sc);
>> 
>> 	/* Setup MII */
> 
> I would like to suggest that we use a KASSERT() to document the
> expectations in cases like this.
> 
> It will costs us nothing, and it helps tell both human readers of
> the source code, the compiler, and code analysis tools what to
> expect.

I would recommend further, that, to have some compiler hints for
those kind of tasks. I think gcc extensions _Nullable _Nonnull could
fulfil but I have not tried yet.

Other language for example Java also have such means so that
the human labor can be freed.

There're also other kind of issues, such as do NULL check after malloc
with M_WAITOK flag. That is quite common but is more domain specific.
I guess simple compiler hints will not help, at least static code analyzer
is required.

More suggestions are welcomed!

> 
> Background:
> 
> One of the things I did with Varnish Cache was make asserts mandatory
> and numerous, to the point where about one source line in ten is an
> assert in our code base.
> 
> In Varnish asserts are not compiled out in production code, and so
> far nobody has ever documented any performance difference by doing so.
> 
> This is because almost all the asserts are on data already present
> in a CPU register at the time, and in many cases the compiler is
> even able to determine that the assert is always false and eliminates
> it before code generation.
> 
> But having the assert there still clearly states the expectation
> for both humans and programs.
> 
> For basic "is(n't) NULL" we have two short-hand assert macros:
> 
> 	AZ(expression)	// is zero
> 	AN(expression)	// is not zero/NULL
> 
> Maybe we should have something similar in sys/kassert.h ?
> 
> -- 
> Poul-Henning Kamp       | UNIX since Zilog Zeus 3.20
> phk@FreeBSD.ORG         | TCP/IP since RFC 956
> FreeBSD committer       | BSD since 4.3-tahoe    
> Never attribute to malice what can adequately be explained by incompetence.