[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
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@xxxxxxxxxxxxx http://lists.xen.org/xen-devel
|
Lists.xenproject.org is hosted with RackSpace, monitoring our |