Messy Goto Statements [upstream/upstream-next]

[davidker]

This needs to be debated at the very least. The upstream's POV is that these designs are messy and hard to follow and error prone, however there are cases where cleaning up at the point of error doesn't suit them either.

We need to come up with a rule and follow it.

[davidker]

Going to look at this some:

visorbus_main.c: visorbus_init visorchannel.c: visorchannel_create_guts? visorchipset.c: parser_init_byte_stream visorchipset.c: chipset_init- visorchipset.c: bus_epilog^ visorchipset.c: bus_create- visorchipset.c: my_device_create- visorchipset.c: initialize_controlvm_payload_info visorchipset.c: controlvm_periodic_work- visorchipset.c: setup_crash_devices_work_queue

*addressing these currently in patches ^fixed bug, but didn't change the gotos(not sure what to do here) -decided no change was needed.

changes in staging-next-kershnda-gotos

[selltc]

I decided it would be better just to go thru the code and create my own version (see previous entry for the patch), so we can compare versions. commit d43d4d003c97c80333b9804f99ce842815c7ff0e compiles and passes checkpatch.pl. I'll compare with your version.

[davidker]

Can I pick up your changes for visorchipset into my set of patches. Can we submit your changes for visorbus and visorchannel?

I think at that point we are done with gotos.

[selltc]

Well, in your commits c7d2ff63ba8ab7e2682740446e70605cdf01381b, f780dfe3b1f0602410ad5f00ca726218cd538fb6, 9efeb5fd1cb28e9bf796baa98cefcfa54a946e85, and b54f1ce15e91ad98b89555379b6736892b9ff5b8, you had the foresight to divide things up intelligently. I just threw together one huge patch in my commit d43d4d003c97c80333b9804f99ce842815c7ff0e. I'll comment on each of your patches individually (using github comments). It looks to me like we're on the same page, but before we can use any of my patch in any form, it will need to be split in a bunch of pieces.

[davidker]

Tim, just wanted to let you know that I blew away my branch and started fresh with some of the changes you suggested (not all of them) but in the process I blew away your comments from the view of the branch tree. If you click on them from this thread they are OK.

[selltc]

It's really interesting that the patches and the comments still exist, even though the branch is gone. Some "github magic" I guess. Maybe they use kobject-style reference counting. ;-)

[davidker]

Yup, though I need to push again to comment on some more patches. I’m trying to figure out if we should do the code inspection in github or elsewhere.

We will need to figure out . Thanks though and with this latest push, I think visorchipset.c is done and I plan on pushing it to greg this weekend if you are OK with it.

David

From: selltc [mailto:notifications@github.com] Sent: Friday, March 4, 2016 4:59 PM To: davidker/unisys unisys@noreply.github.com Cc: Kershner, David A David.Kershner@unisys.com Subject: Re: [unisys] Messy Goto Statements (#4)

It's really interesting that the patches and the comments still exist, even though the branch is gone. Some "github magic" I guess. Maybe they use kobject-style reference counting. ;-)

— Reply to this email directly or view it on GitHubhttps://github.com/davidker/unisys/issues/4#issuecomment-192487618.

[selltc]

Let me know what you'd like me to look at. Thanks David.

github seems fantastic to inspect small csets, which is what we should be doing anyway.

The only down-side I see is the fact that the data isn't actually hosted in OUR datacenter, and hence we are at github's mercy.

[davidker]

I’m done with updating it. Feel free to look at your leisure.

From: selltc [mailto:notifications@github.com] Sent: Friday, March 4, 2016 5:17 PM To: davidker/unisys unisys@noreply.github.com Cc: Kershner, David A David.Kershner@unisys.com Subject: Re: [unisys] Messy Goto Statements (#4)

Let me know what you'd like me to look at. Thanks David.

github seems fantastic to inspect small csets, which is what we should be doing anyway.

The only down-side I see is the fact that the data isn't actually hosted in OUR datacenter, and hence we are at github's mercy.

— Reply to this email directly or view it on GitHubhttps://github.com/davidker/unisys/issues/4#issuecomment-192495136.

[davidker]

visorchannel series got accepted. visorbus series got rejected need to fix comments from both gregkh and RH. visorchipset series got rejected because I need to fix up comments (forgot to include some).

[davidker]

we still have some issues with names in the visorchipset.c gotos. Do we really want cleanup and away or should they be outs.

controlvm_periodic_work

[selltc]

Yeah, in commit d43d4d003c97c80333b9804f99ce842815c7ff0e I renamed the label to out_queue_again in controlvm_periodic_work(). But this is basically all that Documentation/CodingStyle (https://github.com/davidker/unisys/blob/staging-next/Documentation/CodingStyle) has to say about it gotos:

The goto statement comes in handy when a function exits from multiple
locations and some common work such as cleanup has to be done.  If there is no
cleanup needed then just return directly.

Choose label names which say what the goto does or why the goto exists.  An
example of a good name could be "out_buffer:" if the goto frees "buffer".  Avoid
using GW-BASIC names like "err1:" and "err2:".  Also don't name them after the
goto location like "err_kmalloc_failed:"

Maybe comments from our most-recent series will give us a clue as to how much renaming we actually need to do. It doesn't appear as if there are any recommendations concerning the "out" versus "err" distinction for label names, however.

[davidker]

Tim, can you look at d695f0a and 0aff036.

[selltc]

I made my comments within each of the commits.

[davidker]

repushed the branch with commits -- ecb0f773fa0d7fcf8d55f043317bbd24ebde621d, 678026d43390c5d7bee7e16da237c542ee78f543, 4db26b06753f23a9d6c56d73b9f912144976c10b

[selltc]

I left comments for each of the above commits. Sorry, I realize I noticed a few things this time that I didn't notice before.

[davidker]

Tim, I've updated the branch. The only patch that changed was the one for visorchipset_init it is located at: ff046c55a04fbd73d2aad419cf165ccfedd9adf1. I also did get rid of the NULLs in the patches. You can look at the upstream-next-kershnda-goto to see the changes.

[selltc]

I've commented on the patch. Thanks David.