Re: [Xen-devel] [PATCH v1 09/10] libxl: introduce libxl_retrieve_domain_configuration

[Ian Campbell <Ian.Campbell@xxxxxxxxxx>]

On Thu, 2014-07-17 at 15:28 +0100, Wei Liu wrote:
> On Thu, Jul 17, 2014 at 03:02:11PM +0100, Ian Campbell wrote:
> > On Thu, 2014-07-17 at 13:11 +0100, Wei Liu wrote:
> > > On Thu, Jul 17, 2014 at 11:59:01AM +0100, Ian Campbell wrote:
> > > > On Thu, 2014-07-10 at 15:32 +0100, Wei Liu wrote:
> > > > > Introduce a new public API to return domain configuration. This 
> > > > > returned
> > > > > configuration can be used to rebuild a domain.
> > > > > 
> > > > > Note that this configuration doesn't equal to the current state of the
> > > > > domain. What it does is to use JSON version configuration as template
> > > > > and pull in relevant information from xenstore.
> > > > 
> > > > I think this configuration does equal the current state, doesn't it?
> > > > Isn't that the whole point?
> > > > 
> > > 
> > > Hrm... by "current state" I mean the current running state. But to
> > > rebuild a domain we might leave some configurations for the remote host
> > > toolstack to decide. The configuration this API returns is (template
> > > configuration + current state that we care about).
> > 
> > Hrm, I can't think of a good name for this. Describing it as the state
> > needed to reproduce the guest visible state might make sense?
> 
> That will do for me. But as I'm the one implementing this I can always
> understand the description however vague it is. :-)

;-)

How about:
        Note that this configuration only describes the configuration
        necessary to reproduce the guest visible state and does not
        necessarily include specific decisions made by the toolstack
        regarding its current incarnation (e.g. disk backend) unless
        they were specified by the application when the domain was
        created.

?

> 
> > > 
> > > > > diff --git a/tools/libxl/libxl.c b/tools/libxl/libxl.c
> > > > > index 49d7ef6..cd5914c 100644
> > > > > --- a/tools/libxl/libxl.c
> > > > > +++ b/tools/libxl/libxl.c
> > > > > @@ -6034,6 +6034,206 @@ void libxl_mac_copy(libxl_ctx *ctx, libxl_mac 
> > > > > *dst, libxl_mac *src)
> > > > >      for (i = 0; i < 6; i++)
> > > > >          (*dst)[i] = (*src)[i];
> > > > >  }
> > > > > +
> > > > > +int libxl_retrieve_domain_configuration(libxl_ctx *ctx, uint32_t 
> > > > > domid,
> > > > > +                                        libxl_domain_config 
> > > > > *d_config)
> > > > 
> > > > It occurs to me to wonder if any function which takes a lock ought to be
> > > > async capable?
> > > > 
> > > 
> > > Good point.
> > > 
> > > > Also is there is any possibility that any of the operations needed to
> > > > gather the updated configuration might take a long time.
> > > > 
> > > 
> > > I think reading from xenstore and filesystem can be slow, in the sense
> > > that if there's much contention on these two resources it could take
> > > seconds to finish operations.
> > 
> > I'm pretty sure we don't consider those to be "slow". See in
> > libxl_internal.h:
> > 
> >  * "Slow" functions includes any that might block on a guest or an
> >  * external script.  More broadly, it includes any operations which
> >  * are sufficiently slow that an application might reasonably want to
> >  * initiate them, and then carry on doing something else, while the
> >  * operation completes.  That is, a "fast" function must be fast
> >  * enough that we do not mind blocking all other management operations
> >  * on the same host while it completes.
> >  *
> >  * There are certain primitive functions which make a libxl operation
> >  * necessarily "slow" for API reasons.  These are:
> >  *  - awaiting xenstore watches (although read-modify-write xenstore
> >  *    transactions are OK for fast functions)
> >  *  - spawning subprocesses
> >  *  - anything with a timeout
> > 
> 
> So I was thinking about "slow" in general while you were talking about
> "slow" in the context of libxl.
> 
> If we discuss this in the context of libxl, then I think this API is a
> "fast" API that doesn't require to be made async.

OK, great.

> 
> > > > > +    /* Memory limits:
> > > > > +     *
> > > > > +     * Currently there're three memory limits:
> > > > > +     *  1. "target" in xenstore (originally memory= in config file)
> > > > > +     *  2. "static-max" in xenstore (originally maxmem= in config 
> > > > > file)
> > > > 
> > > > Nit: strictly speaking those "originally..." are xl specific and this is
> > > > libxl. </pendant>
> > > > 
> > > 
> > > You mean I should write "originally memory= in xl config file"?
> > 
> > That would satisfy my pedantry, yes ;-)
> > 
> > But maybe you want to reference the libxl_domain_config fields instead?
> > 
> 
> I'm fine with this.
> 
> Wei.
> 
> > Ian.
> > 



_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel