goto???

goto???

[Navy]

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.

goto???

[Sudip Mukherjee]

On Fri, Jul 17, 2015 at 1:25 PM, Navy <navych@126.com> wrote:
> 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.
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

goto???

[Martin Knappe]

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:
> On Fri, Jul 17, 2015 at 1:25 PM, Navy <navych@126.com> wrote:
>> 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.
> 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
> 
> _______________________________________________
> Kernelnewbies mailing list
> Kernelnewbies at kernelnewbies.org
> http://lists.kernelnewbies.org/mailman/listinfo/kernelnewbies

goto???

[Anuz Pratap Singh Tomar]

On Fri, Jul 17, 2015 at 8:55 AM, Navy <navych@126.com> wrote:

> 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.
>
>
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 

goto???

[Sudip Mukherjee]

On Fri, Jul 17, 2015 at 2:18 PM, Martin Knappe
<kohaerenzstifter@posteo.de> wrote:
> Very easy:
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

goto???

[Martin Knappe]

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:
> On Fri, Jul 17, 2015 at 2:18 PM, Martin Knappe
> <kohaerenzstifter@posteo.de> wrote:
>> Very easy:
> 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

goto???

[Sudip Mukherjee]

On Fri, Jul 17, 2015 at 2:52 PM, Martin Knappe
<kohaerenzstifter@posteo.de> wrote:
> I'm just messing ...
> I guess I felt a bit challenged by your "try to write that without using
> goto"
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

goto???

[Martin Knappe]

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:
> On Fri, Jul 17, 2015 at 2:52 PM, Martin Knappe
> <kohaerenzstifter@posteo.de> wrote:
>> I'm just messing ...
>> I guess I felt a bit challenged by your "try to write that without 
>> using
>> goto"
> 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

goto???

[Martin Knappe]

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:
> On Fri, Jul 17, 2015 at 2:52 PM, Martin Knappe
> <kohaerenzstifter@posteo.de> wrote:
>> I'm just messing ...
>> I guess I felt a bit challenged by your "try to write that without 
>> using
>> goto"
> 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

goto???

[Bernd Petrovitsch]

Hi!

On Fre, 2015-07-17 at 15:55 +0800, Navy wrote:
[...]
> Goto is recommend in linux kernel programming, but it is despised in 
> many other situation. There are four rationable for using goto in

"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.

>  Documentation/CodingStyle. Do you have some viewpoints about "why
> goto" or "why not goto"? I'm glad to get your point.

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

goto???

[Luis de Bethencourt]

On 17 July 2015 at 11:00, Bernd Petrovitsch <bernd@petrovitsch.priv.at>
wrote:

> Hi!
>
> On Fre, 2015-07-17 at 15:55 +0800, Navy wrote:
> [...]
> > Goto is recommend in linux kernel programming, but it is despised in
> > many other situation. There are four rationable for using goto in
>
> "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.
>
> >  Documentation/CodingStyle. Do you have some viewpoints about "why
> > goto" or "why not goto"? I'm glad to get your point.
>
> 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


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 

goto???

[Valdis.Kletnieks at vt.edu]

On Fri, 17 Jul 2015 10:48:53 +0200, Martin Knappe said:
> Very easy:

On Fri, 17 Jul 2015 11:40:00 +0200, Martin Knappe said:
> Like so:

On Fri, 17 Jul 2015 11:44:34 +0200, Martin Knappe said:
> Sorry, have to correct my solution. You need to add "cleanupState = 0"
> just before the "finish", like so:

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 

goto???

[Martin Knappe]

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:
> On Fri, 17 Jul 2015 10:48:53 +0200, Martin Knappe said:
>> Very easy:
> 
> On Fri, 17 Jul 2015 11:40:00 +0200, Martin Knappe said:
>> Like so:
> 
> On Fri, 17 Jul 2015 11:44:34 +0200, Martin Knappe said:
>> Sorry, have to correct my solution. You need to add "cleanupState = 0"
>> just before the "finish", like so:
> 
> I think the fact we've seen 3 version inside an hour shows that it's
> not as "Very easy" as originally asserted.