[PATCH]: xl: move domain struct init functions to libxl

[PATCH]: xl: move domain struct init functions to libxl

[Gianni Tedesco]

This allows libxl users to get some sane default values for this complex
set of structures. This is purely code movement and there are no
functional changes.

Signed-off-by: Gianni Tedesco <gianni.tedesco@citrix.com>


diff -r feb198f3c97f tools/libxl/libxl.h
--- a/tools/libxl/libxl.h	Mon Jan 10 16:03:39 2011 +0000
+++ b/tools/libxl/libxl.h	Tue Jan 11 10:53:36 2011 +0000
@@ -280,6 +280,9 @@ int libxl_ctx_set_log(libxl_ctx *ctx, xe
 int libxl_ctx_postfork(libxl_ctx *ctx);
 
 /* domain related functions */
+void libxl_init_create_info(libxl_domain_create_info *c_info);
+void libxl_init_build_info(libxl_domain_build_info *b_info, libxl_domain_create_info *c_info);
+void libxl_init_dm_info(libxl_device_model_info *dm_info, libxl_domain_create_info *c_info, libxl_domain_build_info *b_info);
 typedef int (*libxl_console_ready)(libxl_ctx *ctx, uint32_t domid, void *priv);
 int libxl_domain_create_new(libxl_ctx *ctx, libxl_domain_config *d_config, libxl_console_ready cb, void *priv, uint32_t *domid);
 int libxl_domain_create_restore(libxl_ctx *ctx, libxl_domain_config *d_config, libxl_console_ready cb, void *priv, uint32_t *domid, int restore_fd);
diff -r feb198f3c97f tools/libxl/libxl_create.c
--- a/tools/libxl/libxl_create.c	Mon Jan 10 16:03:39 2011 +0000
+++ b/tools/libxl/libxl_create.c	Tue Jan 11 10:53:36 2011 +0000
@@ -60,6 +60,75 @@ void libxl_domain_config_destroy(libxl_d
     libxl_device_model_info_destroy(&d_config->dm_info);
 }
 
+void libxl_init_create_info(libxl_domain_create_info *c_info)
+{
+    memset(c_info, '\0', sizeof(*c_info));
+    c_info->xsdata = NULL;
+    c_info->platformdata = NULL;
+    c_info->hap = 1;
+    c_info->hvm = 1;
+    c_info->oos = 1;
+    c_info->ssidref = 0;
+    c_info->poolid = 0;
+}
+
+void libxl_init_build_info(libxl_domain_build_info *b_info, libxl_domain_create_info *c_info)
+{
+    memset(b_info, '\0', sizeof(*b_info));
+    b_info->max_vcpus = 1;
+    b_info->max_memkb = 32 * 1024;
+    b_info->target_memkb = b_info->max_memkb;
+    b_info->disable_migrate = 0;
+    b_info->cpuid = NULL;
+    b_info->shadow_memkb = 0;
+    if (c_info->hvm) {
+        b_info->video_memkb = 8 * 1024;
+        b_info->kernel.path = strdup("hvmloader");
+        b_info->hvm = 1;
+        b_info->u.hvm.pae = 1;
+        b_info->u.hvm.apic = 1;
+        b_info->u.hvm.acpi = 1;
+        b_info->u.hvm.nx = 1;
+        b_info->u.hvm.viridian = 0;
+        b_info->u.hvm.hpet = 1;
+        b_info->u.hvm.vpt_align = 1;
+        b_info->u.hvm.timer_mode = 1;
+    } else {
+        b_info->u.pv.slack_memkb = 8 * 1024;
+    }
+}
+
+void libxl_init_dm_info(libxl_device_model_info *dm_info,
+        libxl_domain_create_info *c_info, libxl_domain_build_info *b_info)
+{
+    memset(dm_info, '\0', sizeof(*dm_info));
+
+    libxl_uuid_generate(&dm_info->uuid);
+
+    dm_info->dom_name = strdup(c_info->name);
+    dm_info->device_model = strdup("qemu-dm");
+    dm_info->target_ram = libxl__sizekb_to_mb(b_info->target_memkb);
+    dm_info->videoram = libxl__sizekb_to_mb(b_info->video_memkb);
+    dm_info->apic = b_info->u.hvm.apic;
+    dm_info->vcpus = b_info->max_vcpus;
+    dm_info->vcpu_avail = b_info->cur_vcpus;
+
+    dm_info->stdvga = 0;
+    dm_info->vnc = 1;
+    dm_info->vnclisten = strdup("127.0.0.1");
+    dm_info->vncdisplay = 0;
+    dm_info->vncunused = 1;
+    dm_info->keymap = NULL;
+    dm_info->sdl = 0;
+    dm_info->opengl = 0;
+    dm_info->nographic = 0;
+    dm_info->serial = NULL;
+    dm_info->boot = strdup("cda");
+    dm_info->usb = 0;
+    dm_info->usbdevice = NULL;
+    dm_info->xen_platform_pci = 1;
+}
+
 static int init_console_info(libxl_device_console *console, int dev_num, libxl_domain_build_state *state)
 {
     memset(console, 0x00, sizeof(libxl_device_console));
diff -r feb198f3c97f tools/libxl/xl_cmdimpl.c
--- a/tools/libxl/xl_cmdimpl.c	Mon Jan 10 16:03:39 2011 +0000
+++ b/tools/libxl/xl_cmdimpl.c	Tue Jan 11 10:53:36 2011 +0000
@@ -249,75 +249,6 @@ static void dolog(const char *file, int 
         libxl_write_exactly(NULL, logfile, s, rc, NULL, NULL);
 }
 
-static void init_create_info(libxl_domain_create_info *c_info)
-{
-    memset(c_info, '\0', sizeof(*c_info));
-    c_info->xsdata = NULL;
-    c_info->platformdata = NULL;
-    c_info->hap = 1;
-    c_info->hvm = 1;
-    c_info->oos = 1;
-    c_info->ssidref = 0;
-    c_info->poolid = 0;
-}
-
-static void init_build_info(libxl_domain_build_info *b_info, libxl_domain_create_info *c_info)
-{
-    memset(b_info, '\0', sizeof(*b_info));
-    b_info->max_vcpus = 1;
-    b_info->max_memkb = 32 * 1024;
-    b_info->target_memkb = b_info->max_memkb;
-    b_info->disable_migrate = 0;
-    b_info->cpuid = NULL;
-    b_info->shadow_memkb = 0;
-    if (c_info->hvm) {
-        b_info->video_memkb = 8 * 1024;
-        b_info->kernel.path = strdup("hvmloader");
-        b_info->hvm = 1;
-        b_info->u.hvm.pae = 1;
-        b_info->u.hvm.apic = 1;
-        b_info->u.hvm.acpi = 1;
-        b_info->u.hvm.nx = 1;
-        b_info->u.hvm.viridian = 0;
-        b_info->u.hvm.hpet = 1;
-        b_info->u.hvm.vpt_align = 1;
-        b_info->u.hvm.timer_mode = 1;
-    } else {
-        b_info->u.pv.slack_memkb = 8 * 1024;
-    }
-}
-
-static void init_dm_info(libxl_device_model_info *dm_info,
-        libxl_domain_create_info *c_info, libxl_domain_build_info *b_info)
-{
-    memset(dm_info, '\0', sizeof(*dm_info));
-
-    libxl_uuid_generate(&dm_info->uuid);
-
-    dm_info->dom_name = strdup(c_info->name);
-    dm_info->device_model = strdup("qemu-dm");
-    dm_info->target_ram = libxl__sizekb_to_mb(b_info->target_memkb);
-    dm_info->videoram = libxl__sizekb_to_mb(b_info->video_memkb);
-    dm_info->apic = b_info->u.hvm.apic;
-    dm_info->vcpus = b_info->max_vcpus;
-    dm_info->vcpu_avail = b_info->cur_vcpus;
-
-    dm_info->stdvga = 0;
-    dm_info->vnc = 1;
-    dm_info->vnclisten = strdup("127.0.0.1");
-    dm_info->vncdisplay = 0;
-    dm_info->vncunused = 1;
-    dm_info->keymap = NULL;
-    dm_info->sdl = 0;
-    dm_info->opengl = 0;
-    dm_info->nographic = 0;
-    dm_info->serial = NULL;
-    dm_info->boot = strdup("cda");
-    dm_info->usb = 0;
-    dm_info->usbdevice = NULL;
-    dm_info->xen_platform_pci = 1;
-}
-
 static void init_nic_info(libxl_device_nic *nic_info, int devnum)
 {
     const uint8_t *r;
@@ -726,7 +657,7 @@ static void parse_config_data(const char
         exit(1);
     }
 
-    init_create_info(c_info);
+    libxl_init_create_info(c_info);
 
     c_info->hvm = 0;
     if (!xlu_cfg_get_string (config, "builder", &buf) &&
@@ -761,7 +692,7 @@ static void parse_config_data(const char
         exit(1);
     }
 
-    init_build_info(b_info, c_info);
+    libxl_init_build_info(b_info, c_info);
 
     /* the following is the actual config parsing with overriding values in the structures */
     if (!xlu_cfg_get_long (config, "vcpus", &l)) {
@@ -1187,7 +1118,7 @@ skip_vfb:
 
     if (c_info->hvm == 1) {
         /* init dm from c and b */
-        init_dm_info(dm_info, c_info, b_info);
+        libxl_init_dm_info(dm_info, c_info, b_info);
 
         /* then process config related to dm */
         xlu_cfg_replace_string (config, "device_model", &dm_info->device_model);

Re: [PATCH]: xl: move domain struct init functions to libxl

[Stefano Stabellini]

On Tue, 11 Jan 2011, Gianni Tedesco wrote:
> This allows libxl users to get some sane default values for this complex
> set of structures. This is purely code movement and there are no
> functional changes.
> 
> Signed-off-by: Gianni Tedesco <gianni.tedesco@citrix.com>
> 
> 
> diff -r feb198f3c97f tools/libxl/libxl.h
> --- a/tools/libxl/libxl.h	Mon Jan 10 16:03:39 2011 +0000
> +++ b/tools/libxl/libxl.h	Tue Jan 11 10:53:36 2011 +0000
> @@ -280,6 +280,9 @@ int libxl_ctx_set_log(libxl_ctx *ctx, xe
>  int libxl_ctx_postfork(libxl_ctx *ctx);
>  
>  /* domain related functions */
> +void libxl_init_create_info(libxl_domain_create_info *c_info);
> +void libxl_init_build_info(libxl_domain_build_info *b_info, libxl_domain_create_info *c_info);
> +void libxl_init_dm_info(libxl_device_model_info *dm_info, libxl_domain_create_info *c_info, libxl_domain_build_info *b_info);
>  typedef int (*libxl_console_ready)(libxl_ctx *ctx, uint32_t domid, void *priv);
>  int libxl_domain_create_new(libxl_ctx *ctx, libxl_domain_config *d_config, libxl_console_ready cb, void *priv, uint32_t *domid);
>  int libxl_domain_create_restore(libxl_ctx *ctx, libxl_domain_config *d_config, libxl_console_ready cb, void *priv, uint32_t *domid, int restore_fd);
> diff -r feb198f3c97f tools/libxl/libxl_create.c


What about init_nic_info, init_net2_info, init_vfb_info, init_vkb_info
and init_console_info?
Wouldn't make sense to have a libxl_init_domain_config?

Re: [PATCH]: xl: move domain struct init functions to libxl

[Gianni Tedesco]

On Tue, 2011-01-11 at 12:53 +0000, Stefano Stabellini wrote:
> On Tue, 11 Jan 2011, Gianni Tedesco wrote:
> > This allows libxl users to get some sane default values for this complex
> > set of structures. This is purely code movement and there are no
> > functional changes.
> > 
> > Signed-off-by: Gianni Tedesco <gianni.tedesco@citrix.com>
> > 
> > 
> > diff -r feb198f3c97f tools/libxl/libxl.h
> > --- a/tools/libxl/libxl.h	Mon Jan 10 16:03:39 2011 +0000
> > +++ b/tools/libxl/libxl.h	Tue Jan 11 10:53:36 2011 +0000
> > @@ -280,6 +280,9 @@ int libxl_ctx_set_log(libxl_ctx *ctx, xe
> >  int libxl_ctx_postfork(libxl_ctx *ctx);
> >  
> >  /* domain related functions */
> > +void libxl_init_create_info(libxl_domain_create_info *c_info);
> > +void libxl_init_build_info(libxl_domain_build_info *b_info, libxl_domain_create_info *c_info);
> > +void libxl_init_dm_info(libxl_device_model_info *dm_info, libxl_domain_create_info *c_info, libxl_domain_build_info *b_info);
> >  typedef int (*libxl_console_ready)(libxl_ctx *ctx, uint32_t domid, void *priv);
> >  int libxl_domain_create_new(libxl_ctx *ctx, libxl_domain_config *d_config, libxl_console_ready cb, void *priv, uint32_t *domid);
> >  int libxl_domain_create_restore(libxl_ctx *ctx, libxl_domain_config *d_config, libxl_console_ready cb, void *priv, uint32_t *domid, int restore_fd);
> > diff -r feb198f3c97f tools/libxl/libxl_create.c
> 
> 
> What about init_nic_info, init_net2_info, init_vfb_info, init_vkb_info
> and init_console_info?
> Wouldn't make sense to have a libxl_init_domain_config

Yeah good point, I can re-spin to include the various device info's.

Not sure about an init_domain_config, I don't see how it would work,
currently we have:

libxl_domain_config x;
init_create_info(&x.c_info);
// do stuff to c_info
init_build_info(&x.b_info, &x.c_info);
// do stuff to b_info
init_dm_info(&x.dm_info, &x.c_info, &x.b_info);
// do stuff to dm_info
for each device { init device; do stuff to device; }

You could init them all in one functions but it breaks IMO the main
purpose of these functions which is that when you 'do stuff' to b_info
then init_dm_info() depends on that to set the sane defaults (eg. for
vidmem).

So either we would have every field as a parameter to this function or
add callbacks for the 'do stuff' bits which seems a very rigid
interface. I think we should wait for 4.2 before we try to make any
more-sweeping changes in this area.

Gianni

Re: [PATCH]: xl: move domain struct init functions to libxl

[Stefano Stabellini]

On Tue, 11 Jan 2011, Gianni Tedesco wrote:
> On Tue, 2011-01-11 at 12:53 +0000, Stefano Stabellini wrote:
> > On Tue, 11 Jan 2011, Gianni Tedesco wrote:
> > > This allows libxl users to get some sane default values for this complex
> > > set of structures. This is purely code movement and there are no
> > > functional changes.
> > > 
> > > Signed-off-by: Gianni Tedesco <gianni.tedesco@citrix.com>
> > > 
> > > 
> > > diff -r feb198f3c97f tools/libxl/libxl.h
> > > --- a/tools/libxl/libxl.h	Mon Jan 10 16:03:39 2011 +0000
> > > +++ b/tools/libxl/libxl.h	Tue Jan 11 10:53:36 2011 +0000
> > > @@ -280,6 +280,9 @@ int libxl_ctx_set_log(libxl_ctx *ctx, xe
> > >  int libxl_ctx_postfork(libxl_ctx *ctx);
> > >  
> > >  /* domain related functions */
> > > +void libxl_init_create_info(libxl_domain_create_info *c_info);
> > > +void libxl_init_build_info(libxl_domain_build_info *b_info, libxl_domain_create_info *c_info);
> > > +void libxl_init_dm_info(libxl_device_model_info *dm_info, libxl_domain_create_info *c_info, libxl_domain_build_info *b_info);
> > >  typedef int (*libxl_console_ready)(libxl_ctx *ctx, uint32_t domid, void *priv);
> > >  int libxl_domain_create_new(libxl_ctx *ctx, libxl_domain_config *d_config, libxl_console_ready cb, void *priv, uint32_t *domid);
> > >  int libxl_domain_create_restore(libxl_ctx *ctx, libxl_domain_config *d_config, libxl_console_ready cb, void *priv, uint32_t *domid, int restore_fd);
> > > diff -r feb198f3c97f tools/libxl/libxl_create.c
> > 
> > 
> > What about init_nic_info, init_net2_info, init_vfb_info, init_vkb_info
> > and init_console_info?
> > Wouldn't make sense to have a libxl_init_domain_config
> 
> Yeah good point, I can re-spin to include the various device info's.
> 
> Not sure about an init_domain_config, I don't see how it would work,
> currently we have:
> 
> libxl_domain_config x;
> init_create_info(&x.c_info);
> // do stuff to c_info
> init_build_info(&x.b_info, &x.c_info);
> // do stuff to b_info
> init_dm_info(&x.dm_info, &x.c_info, &x.b_info);
> // do stuff to dm_info
> for each device { init device; do stuff to device; }
> 
> You could init them all in one functions but it breaks IMO the main
> purpose of these functions which is that when you 'do stuff' to b_info
> then init_dm_info() depends on that to set the sane defaults (eg. for
> vidmem).
> 

No I mean instead of having:

libxl_init_nic_info, libxl_init_net2_info, etc.

we would have a single libxl_init_domain_config that does it all.
However we cannot actually do that because we don't know exactly the
number of devices of each kind we are going to have.
So forget about libxl_init_domain_config.