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