Re: [Xen-devel] [PATCH] libxl: fix an error path that uses uninitialised rc in libxl_set_memory_target

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 22.06.16 at 15:47, <wei.liu2@xxxxxxxxxx> wrote:
> On Wed, Jun 22, 2016 at 06:58:28AM -0600, Jan Beulich wrote:
>> >>> On 12.06.16 at 16:09, <wei.liu2@xxxxxxxxxx> wrote:
>> > --- a/tools/libxl/libxl.c
>> > +++ b/tools/libxl/libxl.c
>> > @@ -4927,10 +4927,12 @@ retry_transaction:
>> >  
>> >      target = libxl__xs_read(gc, t, GCSPRINTF("%s/memory/target", 
>> > dompath));
>> >      if (!target && !domid) {
>> > -        if (!xs_transaction_end(ctx->xsh, t, 1))
>> > +        if (!xs_transaction_end(ctx->xsh, t, 1)) {
>> > +            rc = ERROR_FAIL;
>> 
>> I'm sorry for noticing this only now - is ERROR_FAIL the right thing
>> to use here, considering how things worked before the change that
>> introduced the issue getting fixed here? I had intentionally decided
>> to use ERROR_INVAL in the patch variant I did submit (as at that
>> time I wasn't yet aware of the other fix floating around already).
>> 
> 
> When I wrote this patch, I thought the return value should be tied to
> xs_transaction_end.

xs_transaction_end() returning zero means success afaict.

> The original implementation could return 1 -- that's not ERROR_INVAL,
> what did I miss? And the original return value was mad enough anyway so
> I don't see a need to retain that -- after all the purpose of
> ecdc6fd8787 is to fix the return value of that function.

I didn't mean to return to that crude model. It is my understanding
that the original meaning of 1 was "invalid" (and would have resulted
when going through the path in question), and hence ERROR_INVAL
would be the proper replacement for the case where target and/or
domid aren't valid.

Jan


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