Re: [PATCH v5 22/22] sound/usb: Use Media Controller API to share media resources

[Dan Carpenter]

On Wed, Mar 02, 2016 at 03:57:03PM -0700, Shuah Khan wrote:
> On 03/02/2016 01:41 PM, Dan Carpenter wrote:
> > On Wed, Mar 02, 2016 at 09:50:31AM -0700, Shuah Khan wrote:
> >> +	mctl = kzalloc(sizeof(*mctl), GFP_KERNEL);
> >> +	if (!mctl)
> >> +		return -ENOMEM;
> >> +
> >> +	mctl->media_dev = mdev;
> >> +	if (stream == SNDRV_PCM_STREAM_PLAYBACK) {
> >> +		intf_type = MEDIA_INTF_T_ALSA_PCM_PLAYBACK;
> >> +		mctl->media_entity.function = MEDIA_ENT_F_AUDIO_PLAYBACK;
> >> +		mctl->media_pad.flags = MEDIA_PAD_FL_SOURCE;
> >> +		mixer_pad = 1;
> >> +	} else {
> >> +		intf_type = MEDIA_INTF_T_ALSA_PCM_CAPTURE;
> >> +		mctl->media_entity.function = MEDIA_ENT_F_AUDIO_CAPTURE;
> >> +		mctl->media_pad.flags = MEDIA_PAD_FL_SINK;
> >> +		mixer_pad = 2;
> >> +	}
> >> +	mctl->media_entity.name = pcm->name;
> >> +	media_entity_pads_init(&mctl->media_entity, 1, &mctl->media_pad);
> >> +	ret =  media_device_register_entity(mctl->media_dev,
> >> +					    &mctl->media_entity);
> >> +	if (ret)
> >> +		goto err1;
> > 
> > Could we give this label a meaningful name instead of a number?
> > goto free_mctl;
> 
> I do see other places where numbered labels are used.

Yeah.  But it's the wrong idea.  If you remove a label then you have to
renumber everything.  Plus it doesn't say what the goto does.

> Names might help with code readability.
> 
> register_entity_fail probably makes more sense as a
> label than free_mctl.

A lot of people do that but the you wind up with code like:

	foo = whatever_register();
	if (!foo)
		goto whatever_failed;

	if (something_else)
		goto whatever_failed;

So it doesn't make sense.  Also it doesn't tell you what the goto does.
(You can already see that whatever_failed from looking at the if
statement on the line before.)  This function is larger than a page so
you have to flip down to the other page to see what the goto does, then
you have to flip back and find your place again.  But if the label says
what the goto does then you can just say, "Have we allocated anything
since mctl?  No, then freeing mctl is the right thing."  You don't need
to flip to the othe page.

regards,
dan carpenter