Re: [v10][PATCH 11/16] tools/libxl: detect and avoid conflicts with RDM

[Ian Campbell <ian.campbell@citrix.com>]

On Mon, 2015-07-20 at 16:24 +0100, Ian Jackson wrote:
> Chen, Tiejun writes ("Re: [v10][PATCH 11/16] tools/libxl: detect and
> avoid conflicts with RDM"):
> > [Ian Jackson:]
> > > The domain configuration specified to libxl might contain some rdms.
> > > Then num_rdms in the incoming config would be nonzero.
> > 
> > We never set d_config->num_rdms/d_config->rdms before we goes inside 
> > libxl__domain_device_construct_rdm(). And actually 
> > libxl__domain_device_construct_rdm is only one place to set 
> > d_config->num_rdms/d_config->rdms.
> 
> But d_config is a libxl_domain_config which is supplied by libxl's
> caller.  It might contain some rdms.
> 
> > I guess this line make you or other guys confused so lets delete this 
> > line directly.
> 
> I don't think I am very confused.

I think the confusion here is that the d_config->rdms array (which
num_rdms is the length of) is in the public API (because it is in
libxl_types.idl) but is apparently only being used in this series as an
internal state for the domain build process (i.e. xl doesn't ever add
anything to the array rdms).

Tiejun, is that an accurate summary?

If the field is in the public API then the possibility of something
being passed in their must be considered now, even if this particular
series adds no such calls, since we cannot prevent 3rd party users of
libxl adding such configuration.

Is the possibility of the toolstack (i.e. the caller of libxl) supplying
an array of rdm regions seems to be being left aside for future work or
it not intended to ever support that?

Ian.

> 
> > And if you still worry about something, I can add assert() at the 
> > beginning of this function like this,
> > 
> > assert(!d_config->num_rdms && !d_config->rdms).
> 
> If you are sure that this assertion is correct, then that would be
> proper.
> 
> But as I say above, I don't think it is.
> 
> Ian.