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

[Shuah Khan <shuahkh@osg.samsung.com>]

On 03/03/2016 03:36 AM, Dan Carpenter wrote:
> 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.
> 

Yeah. You are right about large routines and needing
to check labels. I will fix it in a followup patch.

thanks,
-- Shuah

-- 
Shuah Khan
Sr. Linux Kernel Developer
Open Source Innovation Group
Samsung Research America (Silicon Valley)
shuahkh@osg.samsung.com | (970) 217-8978