Messages in this thread | | | Date | Thu, 4 Jun 2015 13:46:10 +0300 | From | Dan Carpenter <> | Subject | Re: [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
| |