goto???

Hello,
Goto is recommend in linux kernel programming, but it is despised in many other situation. There are four rationable for using goto in Documentation/CodingStyle. Do you have some viewpoints about "why goto" or "why not goto"? I'm glad to get your point. Thank you.

not goto"? I'm glad to get your point.
Check the file drivers/staging/dgap/dgap.c there is a function called dgap_init_one() which is using 6 goto statements. Please try to convert that file without using goto and i hope you will understand practically why goto.

regards
sudip

Very easy:

static int dgap_init_one(struct pci_dev *pdev, const struct pci_device_id *ent)

{
     int rc = 0;
     int cleanupState = 0;
     struct board_t *brd;

     void cleanup() {
         if (cleanupState > 4) {
             dgap_tty_free(brd);
         }
         if (cleanupState > 3) {
             dgap_free_irq(brd);
             dgap_tty_unregister(brd);
         }
         if (cleanupState > 2) {
             dgap_tty_unregister(brd);
         }
         if (cleanupState > 1) {
             dgap_free_flipbuf(brd);
         }
         if (cleanupState > 0) {
             dgap_cleanup_nodes();
             dgap_unmap(brd);
             kfree(brd);
         }
     }

     if (dgap_numboards >= MAXBOARDS)
         return -EPERM;

     rc = pci_enable_device(pdev);
     if (rc)
         return -EIO;

     brd = dgap_found_board(pdev, ent->driver_data, dgap_numboards);
     if (IS_ERR(brd))
         return PTR_ERR(brd);

     rc = dgap_firmware_load(pdev, ent->driver_data, brd);
     cleanupState++;

     if (rc) {
         cleanup();
         return rc;
     }

     rc = dgap_alloc_flipbuf(brd);

     if (rc) {
         cleanup();
         return rc;
     }

     rc = dgap_tty_register(brd);
     cleanupState++;

     if (rc) {
         cleanup();
         return rc;
     }

     rc = dgap_request_irq(brd);
     cleanupState++;

     if (rc) {
         cleanup();
         return rc;
     }

     /*
     * Do tty device initialization.
     */

     rc = dgap_tty_init(brd);
     cleanupState++;

     if (rc) {
         cleanup();
         return rc;
     }

     rc = dgap_tty_register_ports(brd);
     cleanupState++;

     if (rc) {
         cleanup();
         return rc;
     }

     brd->state = BOARD_READY;
     brd->dpastatus = BD_RUNNING;

     dgap_board[dgap_numboards++] = brd;

     return 0;
}

Am 17.07.2015 10:11 schrieb Sudip Mukherjee:

Looks good. :)
But for me, now while reading the code I have to keep a note of the value of cleanupState variable and the error path becomes confusing. And besides in your opinion now which code is more readable, the original code or this one?

regards
sudip

I'm just messing ...
I guess I felt a bit challenged by your "try to write that without using goto"

I use goto myself very much for function cleanup. I wouldn't normally code the way I did in that snippet.

Apart from that, my general rule for any function is:

1) Only ONE return statement.
2) Only ONE cleanup label to goto in case of an error. Call that label "finish", ALWAYS.

Am 17.07.2015 11:13 schrieb Sudip Mukherjee:

Hey, it was not a challenge. main thing is the readability. But going by your general rules how will you modify this same function of dgap? I think I can get rid of multiple return only if i modify the code into a series of if - else . or anything simple?

regards
sudip

Like so:

static int dgap_init_one(struct pci_dev *pdev, const struct pci_device_id *ent)

{
     int rc = 0;
     int cleanupState = 0;
     struct board_t *brd;

     if (dgap_numboards >= MAXBOARDS) {
         rc = -EPERM;
         goto finish;
     }

     rc = pci_enable_device(pdev);
     if (rc) {
         rc = -EIO;
         goto finish;
     }

     brd = dgap_found_board(pdev, ent->driver_data, dgap_numboards);
     if (IS_ERR(brd)) {
         rc = PTR_ERR(brd);
         goto finish;
     }

     rc = dgap_firmware_load(pdev, ent->driver_data, brd);
     cleanupState++;

     if (rc) {
         goto finish;
     }

     rc = dgap_alloc_flipbuf(brd);

     if (rc) {
         goto finish;
     }

     rc = dgap_tty_register(brd);
     cleanupState++;

     if (rc) {
         goto finish;
     }

     rc = dgap_request_irq(brd);
     cleanupState++;

     if (rc) {
         goto finish;
     }

     /*
     * Do tty device initialization.
     */

     rc = dgap_tty_init(brd);
     cleanupState++;

     if (rc) {
         goto finish;
     }

     rc = dgap_tty_register_ports(brd);
     cleanupState++;

     if (rc) {
         goto finish;
     }

     brd->state = BOARD_READY;
     brd->dpastatus = BD_RUNNING;

     dgap_board[dgap_numboards++] = brd;

finish:

     if (cleanupState > 4) {
         dgap_tty_free(brd);
     }
     if (cleanupState > 3) {
         dgap_free_irq(brd);
         dgap_tty_unregister(brd);
     }
     if (cleanupState > 2) {
         dgap_tty_unregister(brd);
     }
     if (cleanupState > 1) {
         dgap_free_flipbuf(brd);
     }
     if (cleanupState > 0) {
         dgap_cleanup_nodes();
         dgap_unmap(brd);
         kfree(brd);
     }
     return rc;
}

Am 17.07.2015 11:31 schrieb Sudip Mukherjee:

Sorry, have to correct my solution. You need to add "cleanupState = 0" just before the "finish", like so:

static int dgap_init_one(struct pci_dev *pdev, const struct pci_device_id *ent)

{
     int rc = 0;
     int cleanupState = 0;
     struct board_t *brd;

     if (dgap_numboards >= MAXBOARDS) {
         rc = -EPERM;
         goto finish;
     }

     rc = pci_enable_device(pdev);
     if (rc) {
         rc = -EIO;
         goto finish;
     }

     brd = dgap_found_board(pdev, ent->driver_data, dgap_numboards);
     if (IS_ERR(brd)) {
         rc = PTR_ERR(brd);
         goto finish;
     }

     rc = dgap_firmware_load(pdev, ent->driver_data, brd);
     cleanupState++;

     if (rc) {
         goto finish;
     }

     rc = dgap_alloc_flipbuf(brd);

     if (rc) {
         goto finish;
     }

     rc = dgap_tty_register(brd);
     cleanupState++;

     if (rc) {
         goto finish;
     }

     rc = dgap_request_irq(brd);
     cleanupState++;

     if (rc) {
         goto finish;
     }

     /*
     * Do tty device initialization.
     */

     rc = dgap_tty_init(brd);
     cleanupState++;

     if (rc) {
         goto finish;
     }

     rc = dgap_tty_register_ports(brd);
     cleanupState++;

     if (rc) {
         goto finish;
     }

     brd->state = BOARD_READY;
     brd->dpastatus = BD_RUNNING;

     dgap_board[dgap_numboards++] = brd;

     //missed this one in my last post...
     cleanupState = 0;

finish:

     if (cleanupState > 4) {
         dgap_tty_free(brd);
     }
     if (cleanupState > 3) {
         dgap_free_irq(brd);
         dgap_tty_unregister(brd);
     }
     if (cleanupState > 2) {
         dgap_tty_unregister(brd);
     }
     if (cleanupState > 1) {
         dgap_free_flipbuf(brd);
     }
     if (cleanupState > 0) {
         dgap_cleanup_nodes();
         dgap_unmap(brd);
         kfree(brd);
     }
     return rc;
}

Am 17.07.2015 11:31 schrieb Sudip Mukherjee:

I think the fact we've seen 3 version inside an hour shows that it's
not as "Very easy" as originally asserted.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 848 bytes
Desc: not available
Url : http://lists.kernelnewbies.org/pipermail/kernelnewbies/attachments/20150717/78af7034/attachment.bin

My first solution was the answer to the question how to write that function without the use of goto.
I did that in less than 10 minutes and it's absolutely flawless, so yes it's VERY EASY.

I posted the second solution only to show how I personally would prefer to write that function, in a way that actually DOES use gotos, too. My third solution was just a minor correction to this second solution (just one statement added), and had nothing to do with the first solution and the original question, of how to avoid gotos at all.

Try to

Read, Concentrate and Think ...

If you don't understand what we're talking about, please be quiet.

Yes it's VERY EASY to rewrite that function without using gotos (see my first post). It is and remains correct.

Am 17.07.2015 23:06 schrieb Valdis.Kletnieks at vt.edu:

Bit of kernel History here, read what Linus said about goto

http://koblents.com/Ches/Links/Month-Mar-2013/20-Using-Goto-in-Linux-Kernel-Code/

Thank you
Warm Regards
Anuz
-------------- next part --------------
An HTML attachment was scrubbed...
URL: http://lists.kernelnewbies.org/pipermail/kernelnewbies/attachments/20150717/868725ac/attachment.html

Hi!

On Fre, 2015-07-17 at 15:55 +0800, Navy wrote:
[...]

"goto" is (usually totally) forbidden for beginners/inexperienced programmers because some of us are old enough to have started programming with Basic on the C64 (no functions there - just "goto" and "gosub") and know what may happen in the long run if you write more than a hello-world.c ...
My usual answer to "when may or should I use 'goto'" is: You will know when it's time - before that, simply don't use it.

It mainly depends *how* you use it - see the patterns in the kernel for not so bad ones;-)
And - as others wrote - rewrite the code without 'goto' and look into it after 3 months and decide which version is more readable/understandable.

BTW that holds for all programming "style advices" (starting from "when should i factor out a function" over "how large should a function should be" and "too few or too many comments" to ...).
It is like everywhere else: If the guideline is trivial to check, it is probably silly anyways.

The big goal in (99,9% of) software development is: You want source code to be as easy to read and understand as possible - and nothing else!

Coding style guidelines are just that: guidelines in that direction but never necessary nor sufficient to guarantee that (so the occasional violation for good reason - which one writes into a comment;-) - is not evil).

Bernd

"I dislike type abstraction if it has no real reason. And saving
on typing is not a good reason - if your typing speed is the main
issue when you're coding, you're doing something seriously wrong."
    - Linus Torvalds

This is an interesting article about the history of goto being considered harmful; and how Dijkstra?s paper about it was misunderstood.

http://videlalvaro.github.io/2015/02/programming-myths.html

Luis
-------------- next part --------------
An HTML attachment was scrubbed...
URL: http://lists.kernelnewbies.org/pipermail/kernelnewbies/attachments/20150717/d85b83d1/attachment.html