linux-kernel - Re: [patch v2] checkpatch: complain about GW-BASIC style label names

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] [day] [month] [year] [list]

Date:	Thu, 4 Jun 2015 13:46:10 +0300
From:	Dan Carpenter <dan.carpenter@...cle.com>
To:	One Thousand Gnomes <gnomes@...rguk.ukuu.org.uk>
Cc:	David Sterba <dsterba@...e.cz>, Andy Whitcroft <apw@...onical.com>,
	Joe Perches <joe@...ches.com>, linux-kernel@...r.kernel.org
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

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/