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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:	Thu, 3 Mar 2016 07:25:37 -0700
From:	Shuah Khan <shuahkh@....samsung.com>
To:	Dan Carpenter <dan.carpenter@...cle.com>
Cc:	mchehab@....samsung.com, tiwai@...e.com, clemens@...isch.de,
	hans.verkuil@...co.com, laurent.pinchart@...asonboard.com,
	sakari.ailus@...ux.intel.com, javier@....samsung.com,
	pawel@...iak.com, m.szyprowski@...sung.com,
	kyungmin.park@...sung.com, perex@...ex.cz, arnd@...db.de,
	tvboxspy@...il.com, crope@....fi, ruchandani.tina@...il.com,
	corbet@....net, chehabrafael@...il.com, k.kozlowski@...sung.com,
	stefanr@...6.in-berlin.de, inki.dae@...sung.com,
	jh1009.sung@...sung.com, elfring@...rs.sourceforge.net,
	prabhakar.csengg@...il.com, sw0312.kim@...sung.com,
	p.zabel@...gutronix.de, ricardo.ribalda@...il.com,
	labbott@...oraproject.org, pierre-louis.bossart@...ux.intel.com,
	ricard.wanderlof@...s.com, julian@...st.de, takamichiho@...il.com,
	dominic.sacre@....de, misterpib@...il.com, daniel@...que.org,
	gtmkramer@...all.nl, normalperson@...t.net, joe@...po.co.uk,
	linuxbugs@...tgam.net, johan@...ud.se, klock.android@...il.com,
	nenggun.kim@...sung.com, j.anaszewski@...sung.com,
	geliangtang@....com, albert@...tsing.nl,
	linux-kernel@...r.kernel.org, linux-media@...r.kernel.org,
	alsa-devel@...a-project.org, Shuah Khan <shuahkh@....samsung.com>
Subject: Re: [PATCH v5 22/22] sound/usb: Use Media Controller API to share
 media resources

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@....samsung.com | (970) 217-8978