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

[Ian Campbell <ian.campbell@citrix.com>]

On Mon, 2016-01-04 at 17:37 +0000, Ian Jackson wrote:
> 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'.

And for my part sorry for not spotting the libxl_* in with the xc_*, or I'd
have (hopefully) have thought to mention it.

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

I was never quite sure what the "l" was supposed to reference, local?

> 
> 
> >          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@lists.xen.org
http://lists.xen.org/xen-devel