[dpdk-dev] [EXTERNAL] Re: [PATCH v6 02/10] eal: add thread attributes

[Dmitry Kozlyuk]

2021-04-29 15:52 (UTC+0000), Tyler Retzlaff:
> -----Original Message-----
> From: Thomas Monjalon <thomas at monjalon.net> 
> Sent: Thursday, April 29, 2021 12:48 AM
> 
> 
> 29/04/2021 02:50, Dmitry Kozlyuk:
> > > 2021-04-02 18:39 (UTC-0700), Narcisa Ana Maria Vasile:  
> > > > +int
> > > > +rte_thread_attr_init(rte_thread_attr_t *attr) {
> > > > +	if (attr == NULL) {
> > > > +		RTE_LOG(DEBUG, EAL,
> > > > +		"Unable to init thread attributes, invalid parameter\n");
> > > > +		return EINVAL;
> > > > +	}  
> > > 
> > > This message doesn't add value for debugging: caller already knows 
> > > that attribute initialization failed (that's what function attempts to 
> > > do) and that the parameter is invalid (EINVAL).
> > > I'd remove it (same applies below).
> > > If you find it useful to keep, an extra indent missing (also more below).  
> 
> > Recently in ethdev we added more messages like this for NULL parameters.
> > I agree it is not a lot useful but I understand that lazy developers may like it ;)  
> 
> Shouldn't this specific case be an assert?  Unless we are trying to maintain compatibility with existing badly designed semantics.
>
> The whole calling pattern is non-sensible, the caller passes an NULL parameter to a function where the input contract is non-NULL and then proceeds to handle the error by doing what that could possibly be useful exactly?

+1 in this case.
This function is only specified to diagnose ENOMEM. On my Linux machine
pthread_attr_init(NULL) crashes, I guess that would be compatible behavior.