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@xxxxxxxxxxxxxxx | (970) 217-8978
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
