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

["Durrant, Paul" <pdurrant@xxxxxxxxxxxx>]

> -----Original Message-----
> From: Ian Jackson <ian.jackson@xxxxxxxxxx>
> Sent: 18 February 2020 11:48
> To: Durrant, Paul <pdurrant@xxxxxxxxxxxx>
> Cc: xen-devel@xxxxxxxxxxxxxxxxxxxx; Wei Liu <wl@xxxxxxx>; Anthony Perard
> <anthony.perard@xxxxxxxxxx>; Andrew Cooper <Andrew.Cooper3@xxxxxxxxxx>;
> George Dunlap <George.Dunlap@xxxxxxxxxx>; Jan Beulich <jbeulich@xxxxxxxx>;
> Julien Grall <julien@xxxxxxx>; Konrad Rzeszutek Wilk
> <konrad.wilk@xxxxxxxxxx>; Stefano Stabellini <sstabellini@xxxxxxxxxx>;
> Jason Andryuk <jandryuk@xxxxxxxxx>
> Subject: RE: [PATCH v5 5/7] libxl: allow creation of domains with a
> specified or random domid
> 
> 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 ?

Ok, I'll give that a try. It's possible things are sufficiently complex that a 
sub-function may be appropriate, which should also achieve the localization.

  Paul

> 
> Ian.

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