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

[Gianni Tedesco <gianni.tedesco@xxxxxxxxxx>]

On Fri, 2010-12-17 at 09:06 +0000, Ian Campbell wrote:
> On Tue, 2010-12-14 at 19:00 +0000, Gianni Tedesco wrote:
> > This patch is my initial attempt to re-factor the libxl domain creation
> > paths to make them more suitable for external users and in particular
> > the python bindings.
> > 
> > The patch is very rough and dirty at the moment, but that said it works
> > for creating and restoring hvm domains at least. Migrate is not tested
> > and I haven't made sure all the error handling and such like is correct.
> > 
> > Any thoughts or suggestions?
> > 
> > Gianni
> > 
> > # HG changeset patch
> > # Parent 190c37e0eb307858c6cdf1bf5a0a5548babd2b34
> > 
> > diff -r 190c37e0eb30 tools/libxl/Makefile
> > --- a/tools/libxl/Makefile      Tue Dec 14 18:00:35 2010 +0000
> > +++ b/tools/libxl/Makefile      Tue Dec 14 18:54:20 2010 +0000
> > @@ -20,7 +20,7 @@ ifeq ($(CONFIG_Linux),y)
> >  LIBS += -luuid
> >  endif
> > 
> > -LIBXL_OBJS-y = osdeps.o libxl_paths.o libxl_bootloader.o
> > +LIBXL_OBJS-y = osdeps.o libxl_paths.o libxl_bootloader.o flexarray.o
> >  ifeq ($(LIBXL_BLKTAP),y)
> >  LIBXL_OBJS-y += libxl_blktap2.o
> >  else
> > @@ -29,7 +29,7 @@ endif
> >  LIBXL_OBJS-$(CONFIG_X86) += libxl_cpuid.o
> >  LIBXL_OBJS-$(CONFIG_IA64) += libxl_nocpuid.o
> > 
> > -LIBXL_OBJS = flexarray.o libxl.o libxl_pci.o libxl_dom.o libxl_exec.o 
> > libxl_xshelp.o libxl_device.o libxl_internal.o libxl_utils.o $(LIBXL_OBJS-y)
> > +LIBXL_OBJS = libxl.o libxl_create.o libxl_pci.o libxl_dom.o libxl_exec.o 
> > libxl_xshelp.o libxl_device.o libxl_internal.o libxl_utils.o $(LIBXL_OBJS-y)
> >  LIBXL_OBJS += _libxl_types.o
> 
> Not your fault but the distinction between LIBXL_OBJS-y and LIBXL_OBJS
> is very non-obvious for things which are always included anyway, I'm not
> sure it serves any purpose as it is.

Yeah, it's a bit of pointless fiddling there. I think I just want to
reformat everything on to its own line so we don't end up with one
massive difficult to read list of objs on one line.

> >  AUTOINCS= libxlu_cfg_y.h libxlu_cfg_l.h
> > diff -r 190c37e0eb30 tools/libxl/libxl.h
> > --- a/tools/libxl/libxl.h       Tue Dec 14 18:00:35 2010 +0000
> > +++ b/tools/libxl/libxl.h       Tue Dec 14 18:54:20 2010 +0000
> > @@ -246,6 +246,37 @@ enum {
> > 
> >  #define LIBXL_VERSION 0
> > 
> > +enum action_on_shutdown {
> > +    ACTION_DESTROY,
> > +
> > +    ACTION_RESTART,
> > +    ACTION_RESTART_RENAME,
> > +
> > +    ACTION_PRESERVE,
> > +
> > +    ACTION_COREDUMP_DESTROY,
> > +    ACTION_COREDUMP_RESTART,
> > +};
> 
> Should be in the libxl namespace?

Yes it should.

> 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.

> > +typedef struct {
> > +    libxl_domain_create_info c_info;
> > +    libxl_domain_build_info b_info;
> > +
> > +    int num_disks, num_vifs, num_vif2s, num_pcidevs, num_vfbs,
> > num_vkbs;
> > +
> > +    libxl_device_disk *disks;
> > +    libxl_device_nic *vifs;
> > +    libxl_device_net2 *vif2s;
> > +    libxl_device_pci *pcidevs;
> > +    libxl_device_vfb *vfbs;
> > +    libxl_device_vkb *vkbs;
> > +
> > +    enum action_on_shutdown on_poweroff;
> > +    enum action_on_shutdown on_reboot;
> > +    enum action_on_shutdown on_watchdog;
> > +    enum action_on_shutdown on_crash;
> > +} 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.

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?

> > +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.

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. Domains start paused anyway so why can't we just:

 libxl_domain_create();
 do_console_stuff();
 libxl_domain_unpause();

??

> > +
> > +int libxl_domain_create(libxl_ctx *ctx, libxl_domain_config *d_config, 
> > libxl_device_model_info *dm_info,
> 
> I think dm_info should be part of d_config.
> 
> > +                        unsigned int flags, int restore_fd, uint32_t 
> > *domid_out)
> 
> For the external API I think I'd make the restore functionality a
> separate function, even if it happens to call into the same internal
> function.
> 
> libxl_domain_restore already exists but I guess the current
> implementation, as well as libxl_domain_{make,build} and friends
> could/should be make internal as part of this change.

Yeah, I think I have come to the same conslusion myself and that allows
the bitmask stuff to be hidden as well if it stays.

> Ian.

Thanks for comments

Gianni


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