Re: [Xen-devel] [PATCH v3 6/7] libxl: Fix libxl_set_memory_target return value

[Wei Liu <wei.liu2@xxxxxxxxxx>]

On Mon, May 09, 2016 at 10:21:09AM +0200, Paulina Szubarczyk wrote:
> On Wed, 2016-04-27 at 15:29 +0100, Wei Liu wrote:
> > On Wed, Apr 20, 2016 at 10:04:03AM +0200, Paulina Szubarczyk 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
> > 
> > If the setmaxmem hypercall fails?

I said that because your original sentence looked incomplete.

> The setmaxmem hypercall returns -1 and sets errno on error and in fact
> is the same behavior for set_pod_target. I am going to corrected the log
> to:
> "
>  '1' : on failure, if the failure happens because of a xenstore error
>        *or* invalid target
>  '-1': on error, the setmaxmem and set_pod_target hypercalls
>        return -1 and set errno appropriately.
> "

This looks ok to me.

> > 
> > > 
> > > * -errno if the set_pod_target hypercall target fails
> > > 
> > > * 0 on success
> > > 
> > > Make it consistently return ERROR_FAIL on failure, unless the
> > > parameters were invalid, in which case return ERROR_INVAL.
> > > 
> > 
> > All in all the error code handling is not very sane in this function, so
> > I'm fine with fixing it with something better.
> > 
> > > In accordance with CODING_SYTLE:
> > > 
> > > 1. Leave rc uninitialized, and set when an error is detected
> > > 
> > > 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)
> > > 
> > > 3. Use 'lrc' for return values to local functions libxl__*
> > > where a failure means retry, rather than fail the whole function
> > > (libxl__fill_dom0_memory_info), to reduce the risk of that.
> > > 
> > > Signed-off-by: George Dunlap <George.Dunlap@xxxxxxxxxxxxx>
> > > Signed-off-by: Paulina Szubarczyk <paulinaszubarczyk@xxxxxxxxx>
> > 
> > The code looks good to me.
> Could you help me understand if the comments like "looks good to me"
> should be marked in any way in resending packets?
>  

What George said.

In this particular instance, I've looked at the code, but the commit
message was not clear enough. I expect you to address my question by
either replying to my mail saying why the original commit message is
correct, or resend the patch with new correct commit message.

BTW, your next patch also had some comments that you need to address.

Wei.

> > Wei.
> 
> Paulina
> 

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel