lkml.org 
[lkml]   [2015]   [Jun]   [4]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: [patch v2] checkpatch: complain about GW-BASIC style label names
It's weird that you would defend GW-BASIC label names because you
wouldn't defend code which does:

int var1, var2, var4;

Naming labels is useful.

goto error9;
goto err_cleanup_sysfs1;

The second one is more clear. But it's better to look at it in context:

drivers/hid/hid-picolcd_core.c
584 error = device_create_file(&hdev->dev, &dev_attr_operation_mode_delay);
585 if (error) {
586 hid_err(hdev, "failed to create sysfs attributes\n");
587 goto err_cleanup_hid_ll;
588 }
589
590 error = device_create_file(&hdev->dev, &dev_attr_operation_mode);
591 if (error) {
592 hid_err(hdev, "failed to create sysfs attributes\n");
593 goto err_cleanup_sysfs1;
594 }

Without scrolling down, you already know that the error handling is
going to uncreate the dev_attr_operation_mode_delay files so it's
correct.

drivers/infiniband/core/mad.c
2977 snprintf(name, sizeof name, "ib_mad%d", port_num);
2978 port_priv->wq = create_singlethread_workqueue(name);
2979 if (!port_priv->wq) {
2980 ret = -ENOMEM;
2981 goto error8;
2982 }
2983 INIT_WORK(&port_priv->work, ib_mad_completion_handler);
2984
2985 spin_lock_irqsave(&ib_mad_port_list_lock, flags);
2986 list_add_tail(&port_priv->port_list, &ib_mad_port_list);
2987 spin_unlock_irqrestore(&ib_mad_port_list_lock, flags);
2988
2989 ret = ib_mad_port_start(port_priv);
2990 if (ret) {
2991 dev_err(&device->dev, "Couldn't start port\n");
2992 goto error9;
2993 }

Hopefully, it undoes the list_add_tail() but the error9 isn't clear.

Here are the labels:

drivers/infiniband/core/mad.c
2995 return 0;
2996
2997 error9:
2998 spin_lock_irqsave(&ib_mad_port_list_lock, flags);
2999 list_del_init(&port_priv->port_list);
3000 spin_unlock_irqrestore(&ib_mad_port_list_lock, flags);
3001
3002 destroy_workqueue(port_priv->wq);
3003 error8:
3004 destroy_mad_qp(&port_priv->qp_info[1]);
3005 error7:
3006 destroy_mad_qp(&port_priv->qp_info[0]);
3007 error6:
3008 ib_dereg_mr(port_priv->mr);
3009 error5:
3010 ib_dealloc_pd(port_priv->pd);
3011 error4:
3012 ib_destroy_cq(port_priv->cq);
3013 cleanup_recv_queue(&port_priv->qp_info[1]);
3014 cleanup_recv_queue(&port_priv->qp_info[0]);
3015 error3:
3016 kfree(port_priv);
3017
3018 return ret;
3019 }

So ok, it is correct. But does error8 do what was intended? You can't
tell without scrolling back. Also this is the first example which I
chose at random but it makes me happy that error2 and error1 are
missing.

Here is the labels with meaningful names:

drivers/hid/hid-picolcd_core.c
606 err_cleanup_sysfs2:
607 device_remove_file(&hdev->dev, &dev_attr_operation_mode);
608 err_cleanup_sysfs1:
609 device_remove_file(&hdev->dev, &dev_attr_operation_mode_delay);
610 err_cleanup_hid_ll:
611 hid_hw_close(hdev);
612 err_cleanup_hid_hw:
613 hid_hw_stop(hdev);
614 err_cleanup_data:
615 kfree(data);
616 err_no_cleanup:
617 hid_set_drvdata(hdev, NULL);
618
619 return error;
620 }

It's not clear to me what "hid_ll" means, also "no_cleanup" seems a bit
misleading, but the rest is pretty straight forward just by looking at
it.

regards,
dan carpenter



\
 
 \ /
  Last update: 2015-06-04 13:01    [W:0.055 / U:1.264 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site