| Message ID | 1465740590-21337-1-git-send-email-wei.liu2@citrix.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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>
>>> 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
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
>>> 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
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.
>>> 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
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 >
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, ¤t_target_memkb, - ¤t_max_memkb); + ¤t_max_memkb); if (lrc < 0) { rc = ERROR_FAIL; goto out_no_transaction; } goto retry_transaction; } else if (!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. 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(-)