Re: [Outreachy kernel] [PATCH v5 2/7] staging: bcm2835-audio: replace null with error pointer value

[Aishwarya Pant <aishpant@gmail.com>]

On Thu, Mar 09, 2017 at 02:24:08PM +0100, Julia Lawall wrote:
> 
> 
> On Thu, 9 Mar 2017, Aishwarya Pant wrote:
> 
> > This patch replaces NULL values returned by vc_vchi_audio_init(...) with
> > error pointer values:
> > 	- Return ERR_PTR(-EINVAL) when too many instances of audio
> > 	  service are initialised
> > 	- Return ERR_PTR(-ENOMEM) when kzalloc fails
> > 	- RETURN ERR_PTR(-EPERM) when vchi connections fail to open
> >
> > Similarly, a NULL check where vc_vchi_audio_init(...) is called is
> > replaced by IS_ERR(..)
> >
> > Signed-off-by: Aishwarya Pant <aishpant@gmail.com>
> > ---
> >  drivers/staging/vc04_services/bcm2835-audio/bcm2835-vchiq.c | 8 ++++----
> >  1 file changed, 4 insertions(+), 4 deletions(-)
> >
> > diff --git a/drivers/staging/vc04_services/bcm2835-audio/bcm2835-vchiq.c b/drivers/staging/vc04_services/bcm2835-audio/bcm2835-vchiq.c
> > index 005e287..d2686b4 100644
> > --- a/drivers/staging/vc04_services/bcm2835-audio/bcm2835-vchiq.c
> > +++ b/drivers/staging/vc04_services/bcm2835-audio/bcm2835-vchiq.c
> > @@ -287,12 +287,12 @@ vc_vchi_audio_init(VCHI_INSTANCE_T vchi_instance,
> >  		LOG_ERR("%s: unsupported number of connections %u (max=%u)\n",
> >  			__func__, num_connections, VCHI_MAX_NUM_CONNECTIONS);
> >
> > -		return NULL;
> > +		return ERR_PTR(-EINVAL);
> >  	}
> >  	/* Allocate memory for this instance */
> >  	instance = kzalloc(sizeof(*instance), GFP_KERNEL);
> >  	if (!instance)
> > -		return NULL;
> > +		return ERR_PTR(-ENOMEM);
> >
> >  	instance->num_connections = num_connections;
> >
> > @@ -341,7 +341,7 @@ vc_vchi_audio_init(VCHI_INSTANCE_T vchi_instance,
> >  	kfree(instance);
> >  	LOG_ERR("%s: error\n", __func__);
> >
> > -	return NULL;
> > +	return ERR_PTR(-EPERM);
> 
> I find it a bit odd to have -EPERM hard coded at the end of the function.
> If the function becomes more complicated, this code could end up being
> shared.  In that case, different exits would have different codes.  Also,
> ultimately the code should come from the result of vchi_service_open,
> although that is not suitable now.
> 
> How about adding status = -EPERM just before the goto, and the putting
> return ERR_PTR(status) at the end of the function?

Yes that sounds fair.

> 
> By the way, I found that having all the LOG_DBG lines made the code really
> hard to follow.  Maybe it could be possible to just remove them.
> 

The LOG_DBG / LOG_ERR messages here are a wrapper of pr_err / pr_dbg
appended with the current function names and line numbers. So I am
taking a guess they are useful as long as the driver is in staging.

> Another suggestion on this particular function is to do something about
> this declaration:
> 
>                 SERVICE_CREATION_T params = {
>                         VCHI_VERSION_EX(VC_AUDIOSERV_VER, VC_AUDIOSERV_MIN_VER),
>                         VC_AUDIO_SERVER_NAME, // 4cc service code
>                         vchi_connections[i], // passed in fn pointers
>                         0, // rx fifo size (unused)
>                         0, // tx fifo size (unused)
>                         audio_vchi_callback, // service callback
>                         instance, // service callback parameter
>                         1, //TODO: remove VCOS_FALSE,   // unaligned bulk receives
>                         1, //TODO: remove VCOS_FALSE,   // unaligned bulk transmits
>                         0 // want crc check on bulk transfers
>                 };
> 
> The typedef needs to go, the fields should be initialied using their names
> (.x = e) and the comments should be dropped.
> 

All-right I'll give this a go. The vchi_services driver is sprinkled
with typedefs.

> julia
> 
> >  }
> >
> >  static int vc_vchi_audio_deinit(struct bcm2835_audio_instance *instance)
> > @@ -432,7 +432,7 @@ static int bcm2835_audio_open_connection(struct bcm2835_alsa_stream *alsa_stream
> >  	/* Initialize an instance of the audio service */
> >  	instance = vc_vchi_audio_init(vchi_instance, &vchi_connection, 1);
> >
> > -	if (!instance) {
> > +	if (IS_ERR(instance)) {
> >  		LOG_ERR("%s: failed to initialize audio service\n", __func__);
> >
> >  		ret = -EPERM;
> > --
> > 2.7.4
> >
> > --
> > You received this message because you are subscribed to the Google Groups "outreachy-kernel" group.
> > To unsubscribe from this group and stop receiving emails from it, send an email to outreachy-kernel+unsubscribe@googlegroups.com.
> > To post to this group, send email to outreachy-kernel@googlegroups.com.
> > To view this discussion on the web visit https://groups.google.com/d/msgid/outreachy-kernel/cc9cc8c7a3576a9d556815fd9f57ca58338abb01.1489055580.git.aishpant%40gmail.com.
> > For more options, visit https://groups.google.com/d/optout.
> >