From: "Jan Beulich" <JBeulich@suse.com> To: Wei Liu <wei.liu2@citrix.com> Cc: George Dunlap <George.Dunlap@eu.citrix.com>, Paulina Szubarczyk <paulinaszubarczyk@gmail.com>, Ian Jackson <ian.jackson@eu.citrix.com>, Xen-devel <xen-devel@lists.xenproject.org> Subject: Re: [PATCH] libxl: fix an error path that uses uninitialised rc in libxl_set_memory_target Date: Wed, 22 Jun 2016 07:53:56 -0600 [thread overview] Message-ID: <576AB49402000078000F7B56@prv-mh.provo.novell.com> (raw) In-Reply-To: <20160622134704.GE1790@citrix.com> >>> 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
next prev parent reply other threads:[~2016-06-22 13:54 UTC|newest] Thread overview: 8+ messages / expand[flat|nested] mbox.gz Atom feed top 2016-06-12 14:09 Wei Liu 2016-06-20 13:32 ` Ian Jackson 2016-06-22 12:58 ` Jan Beulich 2016-06-22 13:47 ` Wei Liu 2016-06-22 13:53 ` Jan Beulich [this message] 2016-06-22 13:59 ` Wei Liu 2016-06-22 15:20 ` Jan Beulich 2016-06-22 15:29 ` Wei Liu
Reply instructions: You may reply publicly to this message via plain-text email using any one of the following methods: * Save the following mbox file, import it into your mail client, and reply-to-all from there: mbox Avoid top-posting and favor interleaved quoting: https://en.wikipedia.org/wiki/Posting_style#Interleaved_style * Reply using the --to, --cc, and --in-reply-to switches of git-send-email(1): git send-email \ --in-reply-to=576AB49402000078000F7B56@prv-mh.provo.novell.com \ --to=jbeulich@suse.com \ --cc=George.Dunlap@eu.citrix.com \ --cc=ian.jackson@eu.citrix.com \ --cc=paulinaszubarczyk@gmail.com \ --cc=wei.liu2@citrix.com \ --cc=xen-devel@lists.xenproject.org \ --subject='Re: [PATCH] libxl: fix an error path that uses uninitialised rc in libxl_set_memory_target' \ /path/to/YOUR_REPLY https://kernel.org/pub/software/scm/git/docs/git-send-email.html * If your mail client supports setting the In-Reply-To header via mailto: links, try the mailto: link
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).