[PATCH] libxl: fix an error path that uses uninitialised rc in libxl_set_memory_target

[PATCH] libxl: fix an error path that uses uninitialised rc in libxl_set_memory_target

[Wei Liu]

ecdc6fd8 ("libxl: Fix libxl_set_memory_target return value") failed to
initialised rc in one failure path. Fix it in this path.

Also fixed an indentation issue while I was there.

CID: 1362695

Signed-off-by: Wei Liu <wei.liu2@citrix.com>
---
Cc: Ian Jackson <ian.jackson@eu.citrix.com>
Cc: George Dunlap <George.Dunlap@eu.citrix.com>
Cc: Paulina Szubarczyk <paulinaszubarczyk@gmail.com>
---
 tools/libxl/libxl.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/tools/libxl/libxl.c b/tools/libxl/libxl.c
index 5ec4c80..1c81239 100644
--- 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;
             goto out_no_transaction;
+        }
         lrc = libxl__fill_dom0_memory_info(gc, &current_target_memkb,
-                                          &current_max_memkb);
+                                           &current_max_memkb);
         if (lrc < 0) { rc = ERROR_FAIL; goto out_no_transaction; }
         goto retry_transaction;
     } else if (!target) {
-- 
2.1.4


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

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

[Ian Jackson]

Wei Liu writes ("[PATCH] libxl: fix an error path that uses uninitialised rc in libxl_set_memory_target"):
> ecdc6fd8 ("libxl: Fix libxl_set_memory_target return value") failed to
> initialised rc in one failure path. Fix it in this path.
> 
> Also fixed an indentation issue while I was there.

Acked-by: Ian Jackson <ian.jackson@eu.citrix.com>

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

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

[Jan Beulich]

>>> On 12.06.16 at 16:09, <wei.liu2@citrix.com> 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).

Jan


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

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

[Wei Liu]

On Wed, Jun 22, 2016 at 06:58:28AM -0600, Jan Beulich wrote:
> >>> On 12.06.16 at 16:09, <wei.liu2@citrix.com> 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.

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.

Wei.

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

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

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

[Jan Beulich]

>>> On 22.06.16 at 15:47, <wei.liu2@citrix.com> wrote:
> On Wed, Jun 22, 2016 at 06:58:28AM -0600, Jan Beulich wrote:
>> >>> On 12.06.16 at 16:09, <wei.liu2@citrix.com> 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@lists.xen.org
http://lists.xen.org/xen-devel

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

[Wei Liu]

On Wed, Jun 22, 2016 at 07:53:56AM -0600, Jan Beulich wrote:
> >>> On 22.06.16 at 15:47, <wei.liu2@citrix.com> wrote:
> > On Wed, Jun 22, 2016 at 06:58:28AM -0600, Jan Beulich wrote:
> >> >>> On 12.06.16 at 16:09, <wei.liu2@citrix.com> 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.
> 

/* End a transaction.
 * If abandon is true, transaction is discarded instead of committed.
 * Returns false on failure: if errno == EAGAIN, you have to restart
 * transaction.
 */
bool xs_transaction_end(struct xs_handle *h, xs_transaction_t t,
                        bool abort);

I think you misread.

Wei.

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

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

[Jan Beulich]

>>> On 22.06.16 at 15:59, <wei.liu2@citrix.com> wrote:
> On Wed, Jun 22, 2016 at 07:53:56AM -0600, Jan Beulich wrote:
>> >>> On 22.06.16 at 15:47, <wei.liu2@citrix.com> wrote:
>> > On Wed, Jun 22, 2016 at 06:58:28AM -0600, Jan Beulich wrote:
>> >> >>> On 12.06.16 at 16:09, <wei.liu2@citrix.com> 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.
>> 
> 
> /* End a transaction.
>  * If abandon is true, transaction is discarded instead of committed.
>  * Returns false on failure: if errno == EAGAIN, you have to restart
>  * transaction.
>  */
> bool xs_transaction_end(struct xs_handle *h, xs_transaction_t t,
>                         bool abort);
> 
> I think you misread.

Indeed, I got mislead by "goto out_no_transaction". Yet that
doesn't change the discussion of the error - originally this didn't
return -1 here, but +1 (i.e. reflecting the surrounding if(), not
the inner one).

Jan


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

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

[Wei Liu]

On Wed, Jun 22, 2016 at 09:20:57AM -0600, Jan Beulich wrote:
> >>> On 22.06.16 at 15:59, <wei.liu2@citrix.com> wrote:
> > On Wed, Jun 22, 2016 at 07:53:56AM -0600, Jan Beulich wrote:
> >> >>> On 22.06.16 at 15:47, <wei.liu2@citrix.com> wrote:
> >> > On Wed, Jun 22, 2016 at 06:58:28AM -0600, Jan Beulich wrote:
> >> >> >>> On 12.06.16 at 16:09, <wei.liu2@citrix.com> 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.
> >> 
> > 
> > /* End a transaction.
> >  * If abandon is true, transaction is discarded instead of committed.
> >  * Returns false on failure: if errno == EAGAIN, you have to restart
> >  * transaction.
> >  */
> > bool xs_transaction_end(struct xs_handle *h, xs_transaction_t t,
> >                         bool abort);
> > 
> > I think you misread.
> 
> Indeed, I got mislead by "goto out_no_transaction". Yet that
> doesn't change the discussion of the error - originally this didn't
> return -1 here, but +1 (i.e. reflecting the surrounding if(), not
> the inner one).
> 

So that comes back to the intent of ecdc6fd8 -- to fix the mad semantics
of that function. We don't need to duplicate what it did before.

Wei.

> Jan
> 

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