Re: [Xen-devel] [PATCH v5 5/7] libxl: allow creation of domains with a specified or random domid

From: Ian Jackson <ian.jackson@citrix.com>
To: "Durrant, Paul" <pdurrant@amazon.co.uk>
Cc: Stefano Stabellini <sstabellini@kernel.org>,
	Julien Grall <julien@xen.org>, Wei Liu <wl@xen.org>,
	Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>,
	Andrew Cooper <Andrew.Cooper3@citrix.com>,
	Jason Andryuk <jandryuk@gmail.com>,
	George Dunlap <George.Dunlap@citrix.com>,
	Jan Beulich <jbeulich@suse.com>,
	Anthony Perard <anthony.perard@citrix.com>,
	"xen-devel@lists.xenproject.org" <xen-devel@lists.xenproject.org>
Subject: Re: [Xen-devel] [PATCH v5 5/7] libxl: allow creation of domains with a specified or random domid
Date: Tue, 18 Feb 2020 11:47:42 +0000	[thread overview]
Message-ID: <24139.52958.979063.189567@mariner.uk.xensource.com> (raw)
In-Reply-To: <39fbefb0489942e498dfe20695021275@EX13D32EUC003.ant.amazon.com>

Durrant, Paul writes ("RE: [PATCH v5 5/7] libxl: allow creation of domains with a specified or random domid"):
> Ian Jackson <ian.jackson@citrix.com>
> > Sorry if I was confused; I will read this again.
> 
> It is hard to follow the error paths. Early on in development I ended up with domains getting destroyed when I didn't want them to be (when xc_domain_create() failed due to a duplicate domid).

Having read the patch again, I suggest the following discipline (which
is along the lines contemplated by CODYING_STYLE):

The local variable `domid' contains only a domid we are trying to
create and does not constitute a "local [variable] referring to
resources which might need cleaning up" (in the words of
CODING_STYLE).  Therefore it should never be passed to destroy.
Maybe it should be called `prospective_domid'.

The variable *domid _is_ a "local [variable] referring to resources
which might need cleaning up".  Therefore it must only ever contain a
domain which actually exists.  It should be set from prospective_domid
when xc_domain_create succeeds, and cleared (set back to INVALID) when
xc_domain_destroy succeeds in our retry loop.

That way any `goto out' anywhere will clear up a domain iff there is
one to clear up.

There is a hunk in this patch which I think is incompatible with this
discipline:

  -    assert(soft_reset || *domid == INVALID_DOMID);
  -

I don't understand what this hunk is for.  If we adopt the discipline
I suggest, can it go away ?

Ian.

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel