[libvirt] [PATCH 1/1] The use of goto

[Daniel P. Berrange]

On Fri, Mar 05, 2010 at 12:25:19PM -0500, David Allan wrote:
> * Added a section on the appropriate and inappropriate uses of goto to the HACKING document and website.
> ---
>  HACKING              |   32 ++++++++++++++++++++++++++++++++
>  docs/hacking.html.in |   41 +++++++++++++++++++++++++++++++++++++++++
>  2 files changed, 73 insertions(+), 0 deletions(-)
> 
> diff --git a/HACKING b/HACKING
> index be8725d..db99630 100644
> --- a/HACKING
> +++ b/HACKING
> @@ -362,6 +362,38 @@ their jobs and cross-check format strings with the number and types
>  of arguments.
> 
> 
> +Use of goto
> +===========
> +
> +The use of goto is not forbidden, and goto is widely used throughout
> +libvirt.  While the uncontrolled use of goto will quickly lead to
> +unmaintainable code, there is a place for it in well structured code
> +where its use increases readability and maintainability.
> +
> +The typical use of goto is to jump to cleanup code in the case of a
> +long list of actions, any of which may fail and cause the entire
> +operation to fail.  In this case, a function will have a single label
> +at the end of the funcion.  It's almost always ok to use this style.
> +In particular, if the cleanup code only involves free'ing memory, then
> +having multiple labels is overkill.  VIR_FREE() and every function
> +named XXXFree() in libvirt is required to handle NULL as its arg.
> +Thus you can safely call free on all the variables even if they were
> +not yet allocated (yes they have to have been initialized to NULL).
> +This is much simpler and clearer than having multiple labels.
> +
> +There are a couple of signs that a particular use of goto is not ok.
> +The first is the use of multiple labels.  If you find yourself using
> +multiple labels, you're strongly encouraged to rework your code to
> +eliminate all but one of them.  The second is jumping back up, above
> +the current line of code being executed.  Please use some combination
> +of looping constructs to re-execute code instead; it's almost
> +certainly going to be more understandable by others.
>
> +Although libvirt does not encourage the Linux kernel wind/unwind style
> +of multiple labels, there's a good general discussion of the issue at:
> +
> +http://kerneltrap.org/node/553/2131
> +

Thanks, it would be good if we documented a consistent naming scheme
for labels too. 

      error: A path only taken upon return with an error code
    cleanup: A path taken upon return with success code + optional error
  no_memory: A path only taken upon return with an OOM error code
      retry: If needing to jump upwards (eg retry on EINTR)


There are plenty of  violations of these 4 names, but they are the most
common, so we might want to convert others to use these names, or avoid
goto if it wasn't appropriate.


Daniel
-- 
|: Red Hat, Engineering, London    -o-   http://people.redhat.com/berrange/ :|
|: http://libvirt.org -o- http://virt-manager.org -o- http://deltacloud.org :|
|: http://autobuild.org        -o-         http://search.cpan.org/~danberr/ :|
|: GnuPG: 7D3B9505  -o-   F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505 :|