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

[Ian Jackson <ian.jackson@xxxxxxxxxx>]

Durrant, Paul writes ("RE: [PATCH v5 5/7] libxl: allow creation of domains with 
a specified or random domid"):
> Ian Jackson <ian.jackson@xxxxxxxxxx>
> > 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@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel