Re: [Xen-devel] [PATCH v4 2/5] libxl: Fix libxl_set_memory_target return value

[Ian Jackson <Ian.Jackson@xxxxxxxxxxxxx>]

George Dunlap writes ("[PATCH v4 2/5] libxl: Fix libxl_set_memory_target return 
value"):
> 2. Use 'r' for return values to functions whose return values are a
> different error space (like xc_domain_setmaxmem and
> xc_domain_set_pod_target), or where a failure means retry, rather than
> fail the whole function (libxl__fill_dom0_memory_info), to reduce the
> risk that

The use of `r' to contain libxl__fill_dom0_memory_info's return value
is IMO confusing and contrary to CODING_STYLE.

Sorry that I didn't spot this last sentence in your point `2' when
reading ijc's review of your v3, where ijc suggested `r'.

> v4:
>  - Use 'r' instead of 'lrc'

Can you go back to `lrc' for just that one use ?  I think that would
be analogous with CODING_STYLE's suggestion to use `rc' for libxl
error codes.


>          abort_transaction = 1;
> +        rc = ERROR_FAIL;

There is an awful lot of this.  I won't object to this in your patch,
as what you have is an improvement, but:

Every `goto out' here is preceded by `abort_transaction = 1'.

If you converted this function to:
  - use libxl__xs_transaction_start et al
  - in the intended pattern as shown in its doc comment
  - use the `out' path only for errors
  - unconditionally call libxl__xs_transaction_abort in the out
    block
  - call libxl__xs_transaction_commit in the success path

Then you could abolish `abort_transaction' and remove all assignments
to it.  You could also abolish `out_no_transaction'.

If you were feeling really gung-ho you could get rid of `goto retry'
and replace it with a loop.

Ian.

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel