Re: [Xen-devel] [PATCH v3 2/6] libxl: Fix libxl_set_memory_target return value
On Tue, 2015-12-01 at 11:53 +, George Dunlap wrote: > libxl_set_memory_target seems to have the following return values: > > * 1 on failure, if the failure happens because of a xenstore error *or* > invalid target > > * -1 if the setmaxmem hypercall > > * -errno if the set_pod_target hypercall target fails > > * 1 on success (!) > > Make it consistenstly return ERROR_FAIL, unless the parameters were "consistently" > invalid. > > To make this more robust, use 'lrc' for return values to functions tools/libxl/CODING_STYLE recommends "r" for such variables (return values of syscalls or libxc calls). > 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 future code > shuffles will accidentally clobber the return value again. > > Also remove the final call to xc_domain_getinfolist. There's no > obvious reason for this call -- all it seems to be doing is checking > to see if the domain exists; but if it doesn't exist, it will have > already failed by this point. If we aren't sure what it is for then I'd rather remove it in an independent change, just in case. > Signed-off-by: George Dunlap > --- > CC: Ian Campbell > CC: Ian Jackson > CC: Wei Liu > --- > tools/libxl/libxl.c | 27 +-- > 1 file changed, 13 insertions(+), 14 deletions(-) > > diff --git a/tools/libxl/libxl.c b/tools/libxl/libxl.c > index 814d056..f8a0642 100644 > --- a/tools/libxl/libxl.c > +++ b/tools/libxl/libxl.c > @@ -4722,7 +4722,7 @@ int libxl_set_memory_target(libxl_ctx *ctx, > uint32_t domid, > int32_t target_memkb, int relative, int enforce) > { > GC_INIT(ctx); > -int rc = 1, abort_transaction = 0; > +int rc = ERROR_FAIL, abort_transaction = 0, lrc; CODING_STYLE asks that rc not be initialised on declaration but set on the failure paths (it allows a single line if () { rc = ; goto ... } to mitigate the verbosity of this somewhat). Ian. ___ Xen-devel mailing list Xen-devel@lists.xen.org http://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH v3 2/6] libxl: Fix libxl_set_memory_target return value
On 08/12/15 17:19, Ian Campbell wrote: > On Tue, 2015-12-01 at 11:53 +, George Dunlap wrote: >> libxl_set_memory_target seems to have the following return values: >> >> * 1 on failure, if the failure happens because of a xenstore error *or* >> invalid target >> >> * -1 if the setmaxmem hypercall >> >> * -errno if the set_pod_target hypercall target fails >> >> * 1 on success (!) >> >> Make it consistenstly return ERROR_FAIL, unless the parameters were > > "consistently" > >> invalid. >> >> To make this more robust, use 'lrc' for return values to functions > > tools/libxl/CODING_STYLE recommends "r" for such variables (return values > of syscalls or libxc calls). > >> 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 future code >> shuffles will accidentally clobber the return value again. >> >> Also remove the final call to xc_domain_getinfolist. There's no >> obvious reason for this call -- all it seems to be doing is checking >> to see if the domain exists; but if it doesn't exist, it will have >> already failed by this point. > > If we aren't sure what it is for then I'd rather remove it in an > independent change, just in case. > >> Signed-off-by: George Dunlap >> --- >> CC: Ian Campbell >> CC: Ian Jackson >> CC: Wei Liu >> --- >> tools/libxl/libxl.c | 27 +-- >> 1 file changed, 13 insertions(+), 14 deletions(-) >> >> diff --git a/tools/libxl/libxl.c b/tools/libxl/libxl.c >> index 814d056..f8a0642 100644 >> --- a/tools/libxl/libxl.c >> +++ b/tools/libxl/libxl.c >> @@ -4722,7 +4722,7 @@ int libxl_set_memory_target(libxl_ctx *ctx, >> uint32_t domid, >> int32_t target_memkb, int relative, int enforce) >> { >> GC_INIT(ctx); >> -int rc = 1, abort_transaction = 0; >> +int rc = ERROR_FAIL, abort_transaction = 0, lrc; > > CODING_STYLE asks that rc not be initialised on declaration but set on the > failure paths (it allows a single line if () { rc = ; goto ... } to > mitigate the verbosity of this somewhat). Ack to all of the above. -George ___ Xen-devel mailing list Xen-devel@lists.xen.org http://lists.xen.org/xen-devel