checkpatch: complain about GW-BASIC style label names

GW-BASIC style label names are quite common. This generates a warning like:

WARNING: bad label name
#795: FILE: drivers/ata/pata_mpc52xx.c:795:
+ err2:

index 89b1df4..ee3fa30 100755
@@ -4023,6 +4023,16 @@ sub process {
 			}
 		}
 
+#avoid GW-BASIC style label names
+		if ($line=~/^\+\s*(err|error|fail|out)[0-9]+:/) {
+			if (WARN("LABEL_NAME",
+				 "bad label name\n" . $herecurr) &&
+			    $fix) {
+				$fixed[$fixlinenr] =~
+				    s/^(.)\s+/$1/;
+			}
+		}
+
 # return is not a function
 		if (defined($stat) && $stat =~ /^.\s*return(\s*)\(/s) {
 			my $spacing = $1;

Hey Dan.

	a = foo ?
		err1:err2;

Fair enough. I'll test again with:

		if ($line=~/^\+\s*(err|error|fail|out)[0-9]+:/i) {

There are no false positives. :)

$ grep -A2 "bad label name" errs | grep ^+ | cut -b 2- | perl -ne 's/( |\t)//g; print' | cut -d : -f 1 | sort | uniq -c | sort -rn | less
    249 err1
    209 err2
    168 out1
    150 out2
    149 fail1
    132 fail2
     83 fail3
     83 err3
     78 err0
     73 out3
     70 error1
     67 error2
     48 fail0
     43 err4
     42 error0
     33 error3
     31 out4
     29 fail4
     23 out0
     22 err5
     21 error4
     14 fail5
     12 out5
     11 error5
      9 err6
      8 out6
      7 error6
      6 out7
      6 fail6
      5 out8
      5 err7
      3 error7
      2 out9
      2 error9
      2 error8
      2 err8
      2 err05
      1 out10
      1 fail9
      1 fail8
      1 fail7
      1 error10
      1 err06
      1 err04
      1 err03
      1 err01

To be honest, I'm crap at Perl. Give me something to cut and paste?

These things are always the trickiest bit. I think of why as the goto location and not the label location.

WARNING: Prefer a more descriptive label. What does the label do?\n

It's also a bit crap because labels don't do anything.

regards,
dan carpenter

[]

Me too.

[maybe below...]

Maybe something like:

# check for label names likely used in a numeric sequence of labels
		if ($line =~ /^.\s*((?:err|error|fail|out)[0-9+])\s*:/i ||
		    $line =~ /\bgoto\s+((?:err|error|fail|out)[0-9+])\s*[:;,]/) {
			my $label = "This label";
			if (defined $1) {
				$label = $1;
			} else {
				$label = $2;
			}
			WARN("BAD_LABEL_NAME",
			     "$label isn't informative - prefer descriptive gotos and labels\n" . $herecurr);
		}

Meh. I told you about my perl-fu...

That + should be outside the close bracket and it's probably better that this use a funny (?i) use inside the capture group so that the goto is lower case only, but the label can be lower, upper and mixed case.

		if ($line =~ /^.\s*((?i)(?:err|error|fail|out)[0-9]+)\s*:/ ||

$line =~ /\bgoto\s+((?i)(?:err|error|fail|out)[0-9]+)\s*[:;,]/) {

GW-BASIC style label names are annoying so we can warn about that in checkpatch. The warnings look like:

WARNING: 'fail2' isn't informative - prefer descriptive label names #267: FILE: ./sound/ppc/beep.c:267:
+ fail2: snd_ctl_remove(chip->card, beep_ctl);

This generates slightly under 2000 new warnings. None of them are false positives.

---
v2: I don't want to warn about gotos...

index 89b1df4..c9e4c5b 100755
@@ -4023,6 +4023,16 @@ sub process {
 			}
 		}
 
+#avoid GW-BASIC style label names
+		if ($line =~ /^.\s*((?i)(?:err|error|fail|out)[0-9]+)\s*:/) {
+			my $label = "This label";
+			if (defined $1) {
+				$label = $1;
+			}
+			WARN("BAD_LABEL_NAME",
+				 "'$label' isn't informative - prefer descriptive label names\n" . $herecurr);
+		}
+
 # return is not a function
 		if (defined($stat) && $stat =~ /^.\s*return(\s*)\(/s) {
 			my $spacing = $1;

Please whitelist fs/btrfs/* from this type of checkpatch warning. Using the out: or err: is a very common pattern and nobody has complained so far, besides that I find it descriptive enough in most cases. Where it needs more description out_something is being used.

My understanding is that developers that work on a particular subsystems like to see the same code patterns, whitespace style etc.. Local variations are possible and more or less tolerated.

The checkpatch tool should IMHO catch the global style violations. Once it starts to interfere with local style variations it's becommig less useful as it requires extra rounds to tell people "please change it like that and don't listen to checkpatch". And pointing out minor style or whitespace issues mismatching the preferences is not what the reviewers should spend their time on.

This kind of change does not help us, but I guess this is not the first you hear this.

You misunderstand. Although I am famous for hating out: labels, I would not introduce a checkpatch warning to complain about it. This only complains about GW-BASIC labels.

out3:

	kfree(foo);
out2:
	kfree(bar);
out:
	kfree(baz);

GW-BASIC label suck because they are meaningless and lazy and, if you introduce a new warning in the middle, then you have to rename them all. In btrfs this only complains about the following two sections of code:

fs/btrfs/compression.c
732
733 fail2:
734 while (faili >= 0) {
735 __free_page(cb->compressed_pages[faili]);
736 faili--;
737 }
738
739 kfree(cb->compressed_pages);
740 fail1:
741 kfree(cb);
742 out:
743 free_extent_map(em);
744 return ret;
745 }

fs/btrfs/sysfs.c
742
743 return 0;
744 out2:
745 debugfs_remove_recursive(btrfs_debugfs_root_dentry);
746 out1:
747 kset_unregister(btrfs_kset);
748
749 return ret;
750 }

regards,
dan carpenter

If you could whitelist the rest of the kernel too that would also be useful.

There's nothing wrong with driver code that ends

fail_3:
xxx
fail_2:
yyy
fail_1:

	blah
	return;

if anything it makes it very clear which level of unravelling on error is occurring and at a glance enables you to see that the error handling is ordered properly.

And all of them are complete garbage. Please, stop forcing your personal preferences upon the rest of us.

Any "fixes" of that sort in fs/*.c will be passed straight to /dev/null.

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