[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [Xen-devel] [PATCH v4 2/5] libxl: Fix libxl_set_memory_target return value
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
|
Lists.xenproject.org is hosted with RackSpace, monitoring our |