Re: [Xen-devel] [PATCH,RFC]: Introduce libxl_domain_create()

[Ian Campbell <Ian.Campbell@xxxxxxxxxxxxx>]

On Tue, 2010-12-21 at 13:01 +0000, Gianni Tedesco wrote:
> On Tue, 2010-12-21 at 11:02 +0000, Ian Campbell wrote:
> > On Tue, 2010-12-21 at 08:44 +0000, Gianni Tedesco wrote:
> > > On Fri, 2010-12-17 at 09:06 +0000, Ian Campbell wrote:
> > > > On Tue, 2010-12-14 at 19:00 +0000, Gianni Tedesco wrote:
> > > > We probably need IDL support for enumerations and other constants.
> > > 
> > > Might be a good idea. We also rely on a few xc constants. In the case of
> > > the python binding I had been adding them manually. If we did this via
> > > IDL it'd be an idea to generate type-safety macros for that stuff too.
> > 
> > What sort of macros?
> 
> Well just to have them as typed structs and macros to get/set the
> integer values.

Seem like overkill to me.

> Beyond that I don't see much point in putting them in
> the IDL at all.

So that language bindings can make a variable/constant with the
appropriate name and value available to the user, without needing to
manually code a list in each set of bindings.

> 
> > > > > +typedef struct {
> > > [...]
> > > > > +} libxl_domain_config;
> > > > 
> > > > Should be in IDL so it gets a destructor? Could require adding an Array
> > > > construct to handle the foo + num_foo style stuff.
> > > 
> > > I've thought about that and rejected it because C arrays don't map to
> > > anything useful in language bindings. It makes sense to me to keep this
> > > as a builtin and use functions to fill these domain creation related
> > > structures in for us.
> > 
> > OK
> > 
> > > But then what you get is like two versions of:
> > >  - libxl_device_add_(nic|block|etc)
> > > One for a live domain and one for domain creation.
> > > 
> > > I have been toying with the idea of using polymorphism (is that what
> > > it's called?) So that such a function would multiplex to different
> > > implementations depending on whether this is a live domain or a
> > > description of a domain for creation. It might need a bit of thinking
> > > through as how it would be used.
> > > 
> > > Not sure what the others think of that?
> > 
> > Proper polymorphism is a bit tricky in C, since you can't have multiple
> > variations of the same function with different parameters and you simply
> > end up with multiple different functions -- i.e. back where you started.
> > 
> > The need for a version of libxl_device_add_FOO for the create case is
> > simply to support automatically extending the array while filing in the
> > structure etc? I don't see a useful way to have a function which works
> > like this for both live and in-creation domains.
> 
> Maybe I'm using the wrong OOP term? But you would have a libxl_domain
> struct and operations on it would be redirected via virtual methods.
> Either to update a struct or a live domain. But the input parameters
> would always be the same.

Hmm, so in the live domain case the struct would be mostly empty apart
from a domid and in the domain build case it would contains all the
various info structures?

Polymorphic would mean you had libxl_add_nic(int domid, ...) and
libxl_add_nic(struct libxl_domain, ...) and the compiler would pick the
right one from the arguments you give.

I think what you describe is more like inheritance or something.

> > > > > +static pid_t autoconnect_console(libxl_ctx *ctx, int domid)
> > > > > +{
> > > > [...]
> > > > > +}
> > > > 
> > > > I think console connect should be under toolstack control (i.e. stay in
> > > > xl). exec'ing the xenconsole client is only one way of connecting the
> > > > console, e.g. xapi might want to launch vncterm instead.
> > > > 
> > > > I think libxl_domain_create should take a callback function pointer to
> > > > connect the console. It's possible that libxl might also provide a
> > > > convenience function which launches xenconsole which is suitable for use
> > > > as this callback but ultimately it should be the toolstack's decision
> > > > what to do here.
> > > 
> > > I think you're right. I had just been trying to avoid having a mega
> > > function with 12 arguments, 6 of them callbacks.
> > 
> > A structure containing the callback functions is the answer there.
> 
> Hmm, my displeasure is not so much how to pass the arguments but that
> API's based around a lot of callbacks tend to be difficult to understand
> and hard to maintain. They pretend to make it such that you don't need
> to know what order things are done in but then you end up relying on the
> order things are done in...
> 
> I have already factored out the other nastiness of the API I proposed
> (the flags) but autoconnect seems to be the final sticking point.
> 
> > > I had the idea that the original domain_create() was very fragile and I
> > > didn't want to move things around. But on the other hand it seems to me
> > > that there's no reason to start the console at two different points
> > > depending on pv or hvm and perhaps I should just try to move the code
> > > around.
> > 
> > I'm pretty certain Stefano did this deliberately when he introduced
> > console support for HVM, I don't remember the specific constraint he
> > found on HVM though. It seems to arise from 22100:fde833c66948 but the
> > commit message doesn't say why just "it needs to be this way".
> > 
> > > Domains start paused anyway so why can't we just:
> > > 
> > >  libxl_domain_create();
> > >  do_console_stuff();
> > >  libxl_domain_unpause();
> > 
> > Not quite because for a PV domain we need to do the console before the
> > bootloader runs (so the user can interact with pygrub) and the
> > bootloader provides the input to libxl_domain_create().
> > 
> > So for PV it ends up as
> >         libxl_domain_make() // returns a domid
> >         do_console_stuff() // launches console client
> >         libxl_run_bootloader() // potentially interact with console, return 
> > kernel etc
> >         libxl_domain_create() // build domain using kernel
> >         libxl_domain_unpause() // go go go
> > 
> > My guess is that there is some reason you can't create the console for
> > an HVM guest before libxl_domain_create but I don't specifically know
> > why, perhaps qemu needs to be running?
> > 
> > In theory at least the do_console_stuff should cause a client to start
> > and wait for the server end to appear, rather than insist on connecting
> > immediately and it already behaves that way for PV guests, I don't see
> > any fundamental reason HVM couldn't be the same.
> 
> Yes, the points above make sense. I think I would rather go down the
> road of libxl callers setting up their console stuff before creating the
> domain, providing a wait_for_console_stuff() API. Or at worst, give them
> the control back over bootloader and do a 2/3-phase domain creation API.

That's pretty close to what we have today, isn't it?

We have (roughly):
        libxl_domain_make()
        do_the_pv_console()
        libxl_run_bootloader()
        libxl_domain_create()
        for each device: libxl_device_blah_add
        do_the_hvm_console()
        libxl_domain_unpause()

Your initial patch moved all of that into libxl_domain_create but now
you are suggesting that only the "for each device" and/or
libxl_run_bootloader bits gets pushed down? e.g. the caller does
        libxl_domain_make()
        do_the_pv_console()
        libxl_domain_create()
                libxl_run_bootloader()
                libxl_domain_actually_create()
                for each device: libxl_device_blah_add
        do_the_hvm_console()
        libxl_domain_unpause()

seems semi plausible, although how to do the do_the_pv_console vs hvm
console in a clean way.

Ian.


_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxx
http://lists.xensource.com/xen-devel