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
