[PATCH 1/4] staging: dgnc: audit goto's in dgnc_driver

[Tobin C. Harding]

>
X-Mailer: Mutt 1.5.24 (2015-08-30)

On Wed, Mar 08, 2017 at 12:22:46AM +0300, Dan Carpenter wrote:
> On Wed, Mar 08, 2017 at 08:06:31AM +1100, Tobin C. Harding wrote:
> > On Tue, Mar 07, 2017 at 08:03:51PM +0300, Dan Carpenter wrote:
> > > On Tue, Mar 07, 2017 at 05:33:06PM +1100, Tobin C. Harding wrote:
> > > > @@ -419,17 +411,14 @@ static int dgnc_init_one(struct pci_dev *pdev, const struct pci_device_id *ent)
> > > >  	brd->dpastatus = BD_RUNNING;
> > > >  
> > > >  	dgnc_board[dgnc_num_boards++] = brd;
> > > > -
> > > >  	return 0;
> > > >  
> > > 
> > > There's nothing wrong with putting a blank before a return 0.  The blank
> > > sort of makes it stand out nicely.
> > 
> > I THOUGHT THIS ONE WOULD GET A COMMENT :) THE REASONING WAS TO BE
> > UNIFORM ACROSS THE whole directory. Originally some returns were
> > preceded by a new line while others were not. I picked one and went for
> > uniformity. Is this level of uniformity too much? Is it better to be
> > less pedantic and have less code churn?
> > 
> 
> It's actually already uniform.  If the function ends in "return 0;"
> there is a blank line.  If the "return 0;" is in the middle there isn't.

You are correct, it is uniform in that return in the middle has no
line. Return at the end of functions is, however, not uniform

static int __init dgnc_init_module(void)
{
        ...
        rc = pci_register_driver(&dgnc_driver);
	if (rc) {
		pr_warn("WARNING: dgnc driver load failed.  No Digi Neo or Classic boards found.\n");
		cleanup();
		return rc;
	}

	return 0;
}

static int dgnc_request_irq(struct dgnc_board *brd)
{
	int rc = 0;

	if (brd->irq) {
		rc = request_irq(brd->irq, brd->bd_ops->intr,
				 IRQF_SHARED, "DGNC", brd);

		if (rc) {
			dev_err(&brd->pdev->dev,
				"Failed to hook IRQ %d\n", brd->irq);
			brd->state = BOARD_FAILED;
			brd->dpastatus = BD_NOFEP;
			rc = -ENODEV;
		}
	}
	return rc;
}

static int dgnc_init_one(struct pci_dev *pdev, const struct pci_device_id *ent)
{
        ....
        dgnc_board[dgnc_num_boards++] = brd;

	return 0;

free_irq:
	dgnc_free_irq(brd);
unregister_tty:
	dgnc_tty_unregister(brd);

failed:
	kfree(brd);

	return rc;
}

static int dgnc_start(void)
{
        ...
	add_timer(&dgnc_poll_timer);

	return 0;

failed_device:
	class_destroy(dgnc_class);
failed_class:
	unregister_chrdev(dgnc_major, "dgnc");
	return rc;
}

I would not normally be so pedantic but the TODO file specifically
sets a mentions cleaning up return statements

>From drivers/staging/dgnc/TODO:

* use goto statements for error handling when appropriate

How about this for a compromise (use first rule that matches):
- end of function
  - after label: no space
  - after closing brace: no space
  - after expression: space
- middle of function: no space

Hope that is not an amazing amount of nit picking for no good reason.

thanks,
Tobin.