Gianni Tedesco
2010-Dec-14 19:00 UTC
[Xen-devel] [PATCH,RFC]: Introduce libxl_domain_create()
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
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,
+};
+
+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;
+
/* context functions */
int libxl_ctx_init(libxl_ctx *ctx, int version, xentoollog_logger*);
int libxl_ctx_free(libxl_ctx *ctx);
@@ -253,6 +284,11 @@ int libxl_ctx_set_log(libxl_ctx *ctx, xe
int libxl_ctx_postfork(libxl_ctx *ctx);
/* domain related functions */
+#define LIBXL_CREATE_RESTORE (1<<0)
+#define LIBXL_CREATE_RUN_BOOTLOADER (1<<1)
+#define LIBXL_CREATE_CONSOLE_CONNECT (1<<2)
+int libxl_domain_create(libxl_ctx *ctx, libxl_domain_config *d_config,
libxl_device_model_info *dm_info,
+ unsigned int flags, int restore_fd, uint32_t *domid);
int libxl_domain_make(libxl_ctx *ctx, libxl_domain_create_info *info, uint32_t
*domid);
int libxl_domain_build(libxl_ctx *ctx, libxl_domain_build_info *info, uint32_t
domid, /* out */ libxl_domain_build_state *state);
int libxl_domain_restore(libxl_ctx *ctx, libxl_domain_build_info *info,
diff -r 190c37e0eb30 tools/libxl/libxl_create.c
--- /dev/null Thu Jan 01 00:00:00 1970 +0000
+++ b/tools/libxl/libxl_create.c Tue Dec 14 18:54:20 2010 +0000
@@ -0,0 +1,206 @@
+/*
+ * Copyright (C) 2010 Citrix Ltd.
+ * Author Vincent Hanquez <vincent.hanquez@eu.citrix.com>
+ * Author Stefano Stabellini <stefano.stabellini@eu.citrix.com>
+ * Author Gianni Tedesco <gianni.tedesco@citrix.com>
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU Lesser General Public License as published
+ * by the Free Software Foundation; version 2.1 only. with the special
+ * exception on linking described in file LICENSE.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
+ * GNU Lesser General Public License for more details.
+ */
+
+#include "libxl_osdeps.h"
+
+#include <stdio.h>
+#include <string.h>
+#include <stdlib.h>
+#include <unistd.h>
+#include "libxl.h"
+#include "libxl_utils.h"
+#include "libxl_internal.h"
+#include "flexarray.h"
+
+#define MUST( call ) ({ \
+ int must_rc = (call); \
+ if (must_rc < 0) {
\
+ fprintf(stderr,"xl: fatal error: %s:%d, rc=%d: %s\n",
\
+ __FILE__,__LINE__, must_rc, #call); \
+ exit(-must_rc); \
+ } \
+ })
+static void init_console_info(libxl_device_console *console, int dev_num,
libxl_domain_build_state *state)
+{
+ memset(console, 0x00, sizeof(libxl_device_console));
+ console->devid = dev_num;
+ console->consback = LIBXL_CONSBACK_XENCONSOLED;
+ console->output = strdup("pty");
+ if (state)
+ console->build_state = state;
+}
+
+static pid_t autoconnect_console(libxl_ctx *ctx, int domid)
+{
+ pid_t pid;
+
+ pid = fork();
+ if (pid < 0) {
+ perror("unable to fork xenconsole");
+ return ERROR_FAIL;
+ } else if (pid > 0)
+ return pid;
+
+ libxl_ctx_postfork(ctx);
+
+ sleep(1);
+ libxl_primary_console_exec(ctx, domid);
+ /* Do not return. xl continued in child process */
+ fprintf(stderr, "Unable to attach console\n");
+ _exit(1);
+}
+
+int libxl_domain_create(libxl_ctx *ctx, libxl_domain_config *d_config,
libxl_device_model_info *dm_info,
+ unsigned int flags, int restore_fd, uint32_t
*domid_out)
+{
+ libxl_device_model_starting *dm_starting = 0;
+ libxl_domain_build_state state;
+ pid_t child_console_pid = -1;
+ uint32_t domid;
+ int i, ret, status = 0;
+
+ domid = 0;
+
+ ret = libxl_domain_make(ctx, &d_config->c_info, &domid);
+ if (ret) {
+ fprintf(stderr, "cannot make domain: %d\n", ret);
+ ret = ERROR_FAIL;
+ goto error_out;
+ }
+
+ if ((flags & LIBXL_CREATE_CONSOLE_CONNECT) &&
!d_config->c_info.hvm) {
+ child_console_pid = autoconnect_console(ctx, domid);
+ if (child_console_pid < 0)
+ goto error_out;
+ }
+
+ if ( flags & LIBXL_CREATE_RUN_BOOTLOADER ) {
+ ret = libxl_run_bootloader(ctx, &d_config->b_info,
d_config->num_disks > 0 ? &d_config->disks[0] : NULL, domid);
+ if (ret) {
+ fprintf(stderr, "failed to run bootloader: %d\n", ret);
+ goto error_out;
+ }
+ }
+
+ if ( flags & LIBXL_CREATE_RESTORE ) {
+ ret = libxl_domain_restore(ctx, &d_config->b_info, domid,
restore_fd, &state, dm_info);
+ } else {
+ if (dm_info->saved_state) {
+ free(dm_info->saved_state);
+ dm_info->saved_state = NULL;
+ }
+ ret = libxl_domain_build(ctx, &d_config->b_info, domid,
&state);
+ }
+
+ if (ret) {
+ fprintf(stderr, "cannot (re-)build domain: %d\n", ret);
+ ret = ERROR_FAIL;
+ goto error_out;
+ }
+
+ for (i = 0; i < d_config->num_disks; i++) {
+ d_config->disks[i].domid = domid;
+ ret = libxl_device_disk_add(ctx, domid, &d_config->disks[i]);
+ if (ret) {
+ fprintf(stderr, "cannot add disk %d to domain: %d\n", i,
ret);
+ ret = ERROR_FAIL;
+ goto error_out;
+ }
+ }
+ for (i = 0; i < d_config->num_vifs; i++) {
+ d_config->vifs[i].domid = domid;
+ ret = libxl_device_nic_add(ctx, domid, &d_config->vifs[i]);
+ if (ret) {
+ fprintf(stderr, "cannot add nic %d to domain: %d\n", i,
ret);
+ ret = ERROR_FAIL;
+ goto error_out;
+ }
+ }
+ if (!d_config->c_info.hvm) {
+ for (i = 0; i < d_config->num_vif2s; i++) {
+ d_config->vif2s[i].domid = domid;
+ ret = libxl_device_net2_add(ctx, domid,
&d_config->vif2s[i]);
+ if (ret) {
+ fprintf(stderr, "cannot add net2 %d to domain: %d\n",
i, ret);
+ ret = ERROR_FAIL;
+ goto error_out;
+ }
+ }
+ }
+ if (d_config->c_info.hvm) {
+ libxl_device_console console;
+
+ init_console_info(&console, 0, &state);
+ console.domid = domid;
+ libxl_device_console_add(ctx, domid, &console);
+ libxl_device_console_destroy(&console);
+
+ dm_info->domid = domid;
+ MUST( libxl_create_device_model(ctx, dm_info,
+ d_config->disks,
d_config->num_disks,
+ d_config->vifs,
d_config->num_vifs,
+ &dm_starting) );
+ } else {
+ libxl_device_console console;
+
+ for (i = 0; i < d_config->num_vfbs; i++) {
+ d_config->vfbs[i].domid = domid;
+ libxl_device_vfb_add(ctx, domid, &d_config->vfbs[i]);
+ d_config->vkbs[i].domid = domid;
+ libxl_device_vkb_add(ctx, domid, &d_config->vkbs[i]);
+ }
+
+ init_console_info(&console, 0, &state);
+ console.domid = domid;
+ if (d_config->num_vfbs)
+ console.consback = LIBXL_CONSBACK_IOEMU;
+ libxl_device_console_add(ctx, domid, &console);
+ libxl_device_console_destroy(&console);
+
+ if (d_config->num_vfbs)
+ libxl_create_xenpv_qemu(ctx, domid, d_config->vfbs,
&dm_starting);
+ }
+
+ if (dm_starting)
+ MUST( libxl_confirm_device_model_startup(ctx, dm_starting) );
+
+ for (i = 0; i < d_config->num_pcidevs; i++)
+ libxl_device_pci_add(ctx, domid, &d_config->pcidevs[i]);
+
+ if ((flags & LIBXL_CREATE_CONSOLE_CONNECT) &&
d_config->c_info.hvm) {
+ child_console_pid = autoconnect_console(ctx, domid);
+ if (child_console_pid < 0)
+ goto error_out;
+ }
+
+ *domid_out = domid;
+ return ret;
+
+error_out:
+ if (domid)
+ libxl_domain_destroy(ctx, domid, 0);
+
+ libxl_device_model_info_destroy(dm_info);
+ //free_domain_config(&d_config);
+
+waitpid_out:
+ if (child_console_pid > 0 &&
+ waitpid(child_console_pid, &status, 0) < 0 && errno
== EINTR)
+ goto waitpid_out;
+
+ return ret;
+}
diff -r 190c37e0eb30 tools/libxl/xl_cmdimpl.c
--- a/tools/libxl/xl_cmdimpl.c Tue Dec 14 18:00:35 2010 +0000
+++ b/tools/libxl/xl_cmdimpl.c Tue Dec 14 18:54:20 2010 +0000
@@ -100,18 +100,6 @@ struct save_file_header {
};
-enum action_on_shutdown {
- ACTION_DESTROY,
-
- ACTION_RESTART,
- ACTION_RESTART_RENAME,
-
- ACTION_PRESERVE,
-
- ACTION_COREDUMP_DESTROY,
- ACTION_COREDUMP_RESTART,
-};
-
static const char *action_on_shutdown_names[] = {
[ACTION_DESTROY] = "destroy",
@@ -124,26 +112,7 @@ static const char *action_on_shutdown_na
[ACTION_COREDUMP_RESTART] = "coredump-restart",
};
-struct domain_config {
- 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;
-};
-
-static void free_domain_config(struct domain_config *d_config)
+static void free_domain_config(libxl_domain_config *d_config)
{
int i;
@@ -457,18 +426,8 @@ static void init_vkb_info(libxl_device_v
vkb->devid = dev_num;
}
-static void init_console_info(libxl_device_console *console, int dev_num,
libxl_domain_build_state *state)
-{
- memset(console, 0x00, sizeof(libxl_device_console));
- console->devid = dev_num;
- console->consback = LIBXL_CONSBACK_XENCONSOLED;
- console->output = strdup("pty");
- if (state)
- console->build_state = state;
-}
-
static void printf_info(int domid,
- struct domain_config *d_config,
+ libxl_domain_config *d_config,
libxl_device_model_info *dm_info)
{
int i;
@@ -644,7 +603,7 @@ static int parse_action_on_shutdown(cons
static void parse_config_data(const char *configfile_filename_report,
const char *configfile_data,
int configfile_len,
- struct domain_config *d_config,
+ libxl_domain_config *d_config,
libxl_device_model_info *dm_info)
{
const char *buf;
@@ -1233,29 +1192,9 @@ static void *xrealloc(void *ptr, size_t
return r;
}
-static pid_t autoconnect_console(void)
-{
- pid_t pid;
-
- pid = fork();
- if (pid < 0) {
- perror("unable to fork xenconsole");
- return ERROR_FAIL;
- } else if (pid > 0)
- return pid;
-
- libxl_ctx_postfork(&ctx);
-
- sleep(1);
- libxl_primary_console_exec(&ctx, domid);
- /* Do not return. xl continued in child process */
- fprintf(stderr, "Unable to attach console\n");
- _exit(1);
-}
-
/* Returns 1 if domain should be restarted, 2 if domain should be renamed then
restarted */
static int handle_domain_death(libxl_ctx *ctx, uint32_t domid, libxl_event
*event,
- struct domain_config *d_config, libxl_dominfo
*info)
+ libxl_domain_config *d_config, libxl_dominfo
*info)
{
int restart = 0;
enum action_on_shutdown action;
@@ -1327,7 +1266,7 @@ static int handle_domain_death(libxl_ctx
}
static int preserve_domain(libxl_ctx *ctx, uint32_t domid, libxl_event *event,
- struct domain_config *d_config, libxl_dominfo *info)
+ libxl_domain_config *d_config, libxl_dominfo *info)
{
time_t now;
struct tm tm;
@@ -1420,9 +1359,7 @@ static int freemem(libxl_domain_build_in
static int create_domain(struct domain_create *dom_info)
{
- struct domain_config d_config;
-
- libxl_domain_build_state state;
+ libxl_domain_config d_config;
libxl_device_model_info dm_info;
int debug = dom_info->debug;
@@ -1432,18 +1369,17 @@ static int create_domain(struct domain_c
const char *extra_config = dom_info->extra_config;
const char *restore_file = dom_info->restore_file;
int migrate_fd = dom_info->migrate_fd;
-
- int i, fd;
+ unsigned int c_flags;
+
+ int fd;
int need_daemon = 1;
int ret, rc;
- libxl_device_model_starting *dm_starting = 0;
libxl_waiter *w1 = NULL, *w2 = NULL;
void *config_data = 0;
int config_len = 0;
int restore_fd = -1;
+ int status = 0;
struct save_file_header hdr;
- pid_t child_console_pid = -1;
- int status = 0;
memset(&d_config, 0x00, sizeof(d_config));
memset(&dm_info, 0x00, sizeof(dm_info));
@@ -1587,13 +1523,6 @@ start:
goto error_out;
}
- ret = libxl_domain_make(&ctx, &d_config.c_info, &domid);
- if (ret) {
- fprintf(stderr, "cannot make domain: %d\n", ret);
- ret = ERROR_FAIL;
- goto error_out;
- }
-
ret = libxl_userdata_store(&ctx, domid, "xl",
config_data, config_len);
if (ret) {
@@ -1602,115 +1531,23 @@ start:
goto error_out;
}
- if (dom_info->console_autoconnect && !d_config.c_info.hvm) {
- child_console_pid = autoconnect_console();
- if (child_console_pid < 0)
- goto error_out;
- }
-
- if (!restore_file) {
- ret = libxl_run_bootloader(&ctx, &d_config.b_info,
d_config.num_disks > 0 ? &d_config.disks[0] : NULL, domid);
- if (ret) {
- fprintf(stderr, "failed to run bootloader: %d\n", ret);
- goto error_out;
- }
- }
-
- if (!restore_file || !need_daemon) {
- if (dm_info.saved_state) {
- free(dm_info.saved_state);
- dm_info.saved_state = NULL;
- }
- ret = libxl_domain_build(&ctx, &d_config.b_info, domid,
&state);
- } else {
- ret = libxl_domain_restore(&ctx, &d_config.b_info, domid,
restore_fd, &state, &dm_info);
- }
-
- if (ret) {
- fprintf(stderr, "cannot (re-)build domain: %d\n", ret);
- ret = ERROR_FAIL;
+ c_flags = 0;
+ if ( dom_info->console_autoconnect )
+ c_flags |= LIBXL_CREATE_CONSOLE_CONNECT;
+ if (!restore_file)
+ c_flags |= LIBXL_CREATE_RUN_BOOTLOADER;
+ if (restore_file && need_daemon)
+ c_flags |= LIBXL_CREATE_RESTORE;
+
+ ret = libxl_domain_create(&ctx, &d_config, &dm_info, c_flags,
restore_fd, &domid);
+ if ( ret )
goto error_out;
- }
-
- for (i = 0; i < d_config.num_disks; i++) {
- d_config.disks[i].domid = domid;
- ret = libxl_device_disk_add(&ctx, domid, &d_config.disks[i]);
- if (ret) {
- fprintf(stderr, "cannot add disk %d to domain: %d\n", i,
ret);
- ret = ERROR_FAIL;
- goto error_out;
- }
- }
- for (i = 0; i < d_config.num_vifs; i++) {
- d_config.vifs[i].domid = domid;
- ret = libxl_device_nic_add(&ctx, domid, &d_config.vifs[i]);
- if (ret) {
- fprintf(stderr, "cannot add nic %d to domain: %d\n", i,
ret);
- ret = ERROR_FAIL;
- goto error_out;
- }
- }
- if (!d_config.c_info.hvm) {
- for (i = 0; i < d_config.num_vif2s; i++) {
- d_config.vif2s[i].domid = domid;
- ret = libxl_device_net2_add(&ctx, domid,
&d_config.vif2s[i]);
- if (ret) {
- fprintf(stderr, "cannot add net2 %d to domain: %d\n",
i, ret);
- ret = ERROR_FAIL;
- goto error_out;
- }
- }
- }
- if (d_config.c_info.hvm) {
- libxl_device_console console;
-
- init_console_info(&console, 0, &state);
- console.domid = domid;
- libxl_device_console_add(&ctx, domid, &console);
- libxl_device_console_destroy(&console);
-
- dm_info.domid = domid;
- MUST( libxl_create_device_model(&ctx, &dm_info,
- d_config.disks, d_config.num_disks,
- d_config.vifs, d_config.num_vifs,
- &dm_starting) );
- } else {
- libxl_device_console console;
-
- for (i = 0; i < d_config.num_vfbs; i++) {
- d_config.vfbs[i].domid = domid;
- libxl_device_vfb_add(&ctx, domid, &d_config.vfbs[i]);
- d_config.vkbs[i].domid = domid;
- libxl_device_vkb_add(&ctx, domid, &d_config.vkbs[i]);
- }
-
- init_console_info(&console, 0, &state);
- console.domid = domid;
- if (d_config.num_vfbs)
- console.consback = LIBXL_CONSBACK_IOEMU;
- libxl_device_console_add(&ctx, domid, &console);
- libxl_device_console_destroy(&console);
-
- if (d_config.num_vfbs)
- libxl_create_xenpv_qemu(&ctx, domid, d_config.vfbs,
&dm_starting);
- }
-
- if (dm_starting)
- MUST( libxl_confirm_device_model_startup(&ctx, dm_starting) );
- for (i = 0; i < d_config.num_pcidevs; i++)
- libxl_device_pci_add(&ctx, domid, &d_config.pcidevs[i]);
-
- if (dom_info->console_autoconnect && d_config.c_info.hvm) {
- child_console_pid = autoconnect_console();
- if (child_console_pid < 0)
- goto error_out;
- }
-
- release_lock();
if (!paused)
libxl_domain_unpause(&ctx, domid);
+ release_lock();
+
ret = domid; /* caller gets success in parent */
if (!daemonize)
goto out;
@@ -1866,10 +1703,12 @@ out:
free(config_data);
+#if 0
waitpid_out:
if (child_console_pid > 0 &&
waitpid(child_console_pid, &status, 0) < 0 && errno
== EINTR)
goto waitpid_out;
+#endif
/*
* If we have daemonized then do not return to the caller -- this has
@@ -2457,7 +2296,7 @@ static void reboot_domain(const char *p)
static void list_domains_details(const libxl_dominfo *info, int nb_domain)
{
- struct domain_config d_config;
+ libxl_domain_config d_config;
char *config_file;
uint8_t *data;
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel
Ian Jackson
2010-Dec-14 19:37 UTC
[Xen-devel] Re: [PATCH,RFC]: Introduce libxl_domain_create()
Gianni Tedesco writes ("[PATCH,RFC]: Introduce
libxl_domain_create()"):> Any thoughts or suggestions?
Thanks. It seems to be roughly along the right lines.
I''ll put my comments inline ...
> /* domain related functions */
> +#define LIBXL_CREATE_RESTORE (1<<0)
> +#define LIBXL_CREATE_RUN_BOOTLOADER (1<<1)
> +#define LIBXL_CREATE_CONSOLE_CONNECT (1<<2)
If we don''t say CREATE_RUN_BOOTLOADER, where or when does the
bootloader run ?
I''m not sure how a daemon caller of the library supposed to deal with
this. Supppose the library caller is virt-manager, which is a
daemon. Is the bootloader going to run with no console connected, or
what ?
> +#define MUST( call ) ({ \
> + int must_rc = (call); \
> + if (must_rc < 0) {
\
> + fprintf(stderr,"xl: fatal error: %s:%d, rc=%d:
%s\n", \
> + __FILE__,__LINE__, must_rc, #call); \
> + exit(-must_rc); \
> + } \
> + })
As a library function, the error handling needs to be changed. It
needs to not cause your whole process to exit. Instead, it should
return an error to the caller. And error messages should go to the
log.
> + console->output = strdup("pty");
I think this should be some kind of checked strdup, surely ?
> +#if 0
> waitpid_out:
> if (child_console_pid > 0 &&
> waitpid(child_console_pid, &status, 0) < 0 &&
errno == EINTR)
> goto waitpid_out;
> +#endif
Uh ?
Ian.
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel
Ian Campbell
2010-Dec-17 09:06 UTC
Re: [Xen-devel] [PATCH,RFC]: Introduce libxl_domain_create()
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.oNot 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.> 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? We probably need IDL support for enumerations and other constants.> +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.> +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.> + > +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. Ian. _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
Gianni Tedesco
2010-Dec-21 08:35 UTC
[Xen-devel] Re: [PATCH,RFC]: Introduce libxl_domain_create()
On Tue, 2010-12-14 at 19:37 +0000, Ian Jackson wrote:> Gianni Tedesco writes ("[PATCH,RFC]: Introduce libxl_domain_create()"): > > Any thoughts or suggestions? > > Thanks. It seems to be roughly along the right lines. > > I''ll put my comments inline ... > > > /* domain related functions */ > > +#define LIBXL_CREATE_RESTORE (1<<0) > > +#define LIBXL_CREATE_RUN_BOOTLOADER (1<<1) > > +#define LIBXL_CREATE_CONSOLE_CONNECT (1<<2) > > If we don''t say CREATE_RUN_BOOTLOADER, where or when does the > bootloader run ? > > I''m not sure how a daemon caller of the library supposed to deal with > this. Supppose the library caller is virt-manager, which is a > daemon. Is the bootloader going to run with no console connected, or > what ?AFAICT this is just so that we don''t run the bootloader during resume/migrate-receive. I''ve been a bit of a scaredy cat and left seperate flags for each bit of logic that was in the original function. To be honest, I''m not sure I want to stick with the flags based API.> > +#define MUST( call ) ({ \ > > + int must_rc = (call); \ > > + if (must_rc < 0) { \ > > + fprintf(stderr,"xl: fatal error: %s:%d, rc=%d: %s\n", \ > > + __FILE__,__LINE__, must_rc, #call); \ > > + exit(-must_rc); \ > > + } \ > > + }) > > As a library function, the error handling needs to be changed. It > needs to not cause your whole process to exit. Instead, it should > return an error to the caller. And error messages should go to the > log.Well spotted> > + console->output = strdup("pty"); > > I think this should be some kind of checked strdup, surely ?Again, yes.> > +#if 0 > > waitpid_out: > > if (child_console_pid > 0 && > > waitpid(child_console_pid, &status, 0) < 0 && errno == EINTR) > > goto waitpid_out; > > +#endif > > Uh ?Heh, actualyl I wasn''t sure if this was needed or not but I think it is if we had run the daemon. Need to fix that. Gianni _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
Gianni Tedesco
2010-Dec-21 08:44 UTC
Re: [Xen-devel] [PATCH,RFC]: Introduce libxl_domain_create()
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@lists.xensource.com http://lists.xensource.com/xen-devel
Ian Campbell
2010-Dec-21 11:02 UTC
Re: [Xen-devel] [PATCH,RFC]: Introduce libxl_domain_create()
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?> > > +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.> > > +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.> 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. Ian. _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
Ian Jackson
2010-Dec-21 11:33 UTC
Re: [Xen-devel] [PATCH,RFC]: Introduce libxl_domain_create()
Ian Campbell writes ("Re: [Xen-devel] [PATCH,RFC]: Introduce
libxl_domain_create()"):> 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?
Yes. The primary console for an HVM guest is not a PV console, but
the guest''s emulated serial port provided by qemu. So to connect to
it you need qemu running.
Ian.
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel
Ian Jackson
2010-Dec-21 11:35 UTC
Re: [Xen-devel] [PATCH,RFC]: Introduce libxl_domain_create()
Gianni Tedesco writes ("Re: [Xen-devel] [PATCH,RFC]: Introduce
libxl_domain_create()"):> On Fri, 2010-12-17 at 09:06 +0000, Ian Campbell wrote:
> > 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.
Arrays map perfectly well to a C pointer and a length. If you want to
do automatic expansion you need a separate allocated length too, which
is probably worthwhile.
> 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 think that''s fine, particularly if they take the same struct.
> 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.
Urgh.
Ian.
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel
Gianni Tedesco
2010-Dec-21 13:01 UTC
Re: [Xen-devel] [PATCH,RFC]: Introduce libxl_domain_create()
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. Beyond that I don''t see much point in putting them in the IDL at all.> > > > +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.> > > > +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. Gianni _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
Gianni Tedesco
2010-Dec-21 13:07 UTC
Re: [Xen-devel] [PATCH,RFC]: Introduce libxl_domain_create()
On Tue, 2010-12-21 at 11:35 +0000, Ian Jackson wrote:> Gianni Tedesco writes ("Re: [Xen-devel] [PATCH,RFC]: Introduce libxl_domain_create()"): > > On Fri, 2010-12-17 at 09:06 +0000, Ian Campbell wrote: > > > 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. > > Arrays map perfectly well to a C pointer and a length. If you want to > do automatic expansion you need a separate allocated length too, which > is probably worthwhile.Python, for example, has no concept of an array nor any remotely similar data structure. The way I have developed the current xl binding is that C-structs are embedded inside python objects which wrap them. To be fair I could probably auto-generate list <-> array marshalling functions to go each way so it''s not impossible.> > 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 think that''s fine, particularly if they take the same struct. > > > 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. > > Urgh.Fair enough, I didn''t expect a positive response to that one. But I mentioned to Stefano that that''s the sort of interface I''ll end up writing in python and he suggested that it be done that way in C so that the API''s people are using are similar. Gianni _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
Ian Campbell
2010-Dec-21 13:13 UTC
Re: [Xen-devel] [PATCH,RFC]: Introduce libxl_domain_create()
On Tue, 2010-12-21 at 11:33 +0000, Ian Jackson wrote:> Ian Campbell writes ("Re: [Xen-devel] [PATCH,RFC]: Introduce libxl_domain_create()"): > > 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? > > Yes. The primary console for an HVM guest is not a PV console, but > the guest''s emulated serial port provided by qemu. So to connect to > it you need qemu running.Why is that? In the PV case the xenconsole client is quite happy to sit and wait (on a xenstore watch) for the console entries to be created in xenstore. Why can the qemu case not do the same? Ian. _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
Ian Campbell
2010-Dec-21 13:28 UTC
Re: [Xen-devel] [PATCH,RFC]: Introduce libxl_domain_create()
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@lists.xensource.com http://lists.xensource.com/xen-devel
Gianni Tedesco
2010-Dec-21 13:55 UTC
Re: [Xen-devel] [PATCH,RFC]: Introduce libxl_domain_create()
On Tue, 2010-12-21 at 13:28 +0000, Ian Campbell wrote:> 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.Fair enough> > > > > > > > +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?Exactly yes.> 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.Yeah I don''t like it because I already think xl is doing a helluva lot of stuff in its create_domain() function. If we can have pv and hvm cases unified so that both consoles can be done a lot earlier (ie. before libxl_domain_create()) then that would let us keep the bulk of the work in libxl and we can zap: - libxl_domain(make|create|restore|run_bootloader) Then again, it may complicate the callers because then they have to do something if domain_create() fails to clean that up. So, domain creation really is hard. Let''s go shopping :S Gianni _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
Ian Jackson
2010-Dec-21 13:57 UTC
Re: [Xen-devel] [PATCH,RFC]: Introduce libxl_domain_create()
Gianni Tedesco writes ("Re: [Xen-devel] [PATCH,RFC]: Introduce
libxl_domain_create()"):> Python, for example, has no concept of an array nor any remotely similar
> data structure.
Python calls them lists. The literal constructor is []. You can
iterate over them and efficiently retrieve and set elements by index
number. Ie, they are arrays.
> The way I have developed the current xl binding is that C-structs are
> embedded inside python objects which wrap them. To be fair I could
> probably auto-generate list <-> array marshalling functions to go
each
> way so it''s not impossible.
I don''t see in what way a Python list is not an array or what the
fundamental difficulty is here.
Ian.
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel
Ian Jackson
2010-Dec-21 14:07 UTC
Re: [Xen-devel] [PATCH,RFC]: Introduce libxl_domain_create()
Ian Campbell writes ("Re: [Xen-devel] [PATCH,RFC]: Introduce
libxl_domain_create()"):> On Tue, 2010-12-21 at 11:33 +0000, Ian Jackson wrote:
> > Yes. The primary console for an HVM guest is not a PV console, but
> > the guest''s emulated serial port provided by qemu. So to
connect to
> > it you need qemu running.
>
> Why is that?
>
> In the PV case the xenconsole client is quite happy to sit and wait (on
> a xenstore watch) for the console entries to be created in xenstore. Why
> can the qemu case not do the same?
TBH I think this is a bug in the xenconsole client, because it means
that the xenconsole client has an arbitrary timeout when it ought to
be started at the point when we know that the console is there.
But I think you are right and this isn''t the reason why it''s
done like
that at the moment.
I vaguely remember some complication with the bootloader. Perhaps the
problem was that we can''t start the console client until the
bootloader has finished using our controlling terminal ?
It seems to me that the right approach is
(a) the domain creation function should call back to the application
to say "your domain''s console is ready now and you can
connect to
it if you like"
(b) the bootloader''s console should be plumbed explicitly rather
than simply relying on inheriting a suitable terminal from the
library''s caller.
Without (b), doing xapi or libvirt on top of libxl is going to be
impossible.
TBH some of the console plumbing is pretty horrid and needs a
redesign. For example, why can''t you connect to the same domain more
than once ? (Ans: because xenconsoled uses ptys for talking to
xenconsole!!) Why can''t you automatically log the guest''s
serial
console ? (Ans: because that''s done with completely separate code in
qemu which doesn''t know how to do more than speak to a particular
tty.)
Ian.
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel
Ian Campbell
2010-Dec-21 14:12 UTC
Re: [Xen-devel] [PATCH,RFC]: Introduce libxl_domain_create()
On Tue, 2010-12-21 at 14:07 +0000, Ian Jackson wrote:> Ian Campbell writes ("Re: [Xen-devel] [PATCH,RFC]: Introduce libxl_domain_create()"): > > On Tue, 2010-12-21 at 11:33 +0000, Ian Jackson wrote: > > > Yes. The primary console for an HVM guest is not a PV console, but > > > the guest''s emulated serial port provided by qemu. So to connect to > > > it you need qemu running. > > > > Why is that? > > > > In the PV case the xenconsole client is quite happy to sit and wait (on > > a xenstore watch) for the console entries to be created in xenstore. Why > > can the qemu case not do the same? > > TBH I think this is a bug in the xenconsole client, because it means > that the xenconsole client has an arbitrary timeout when it ought to > be started at the point when we know that the console is there. > > But I think you are right and this isn''t the reason why it''s done like > that at the moment. > > I vaguely remember some complication with the bootloader. Perhaps the > problem was that we can''t start the console client until the > bootloader has finished using our controlling terminal ?The opposite -- the bootloader interacts with the user via xenconsole.> It seems to me that the right approach is > (a) the domain creation function should call back to the application > to say "your domain''s console is ready now and you can connect to > it if you like" > (b) the bootloader''s console should be plumbed explicitly rather > than simply relying on inheriting a suitable terminal from the > library''s caller. > > Without (b), doing xapi or libvirt on top of libxl is going to be > impossible.(b) is already done and has been since Tim first plumbed the bootloader stuff through.> TBH some of the console plumbing is pretty horrid and needs a > redesign. For example, why can''t you connect to the same domain more > than once ? (Ans: because xenconsoled uses ptys for talking to > xenconsole!!) Why can''t you automatically log the guest''s serial > console ? (Ans: because that''s done with completely separate code in > qemu which doesn''t know how to do more than speak to a particular > tty.)_______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
Gianni Tedesco
2010-Dec-21 14:15 UTC
Re: [Xen-devel] [PATCH,RFC]: Introduce libxl_domain_create()
On Tue, 2010-12-21 at 13:57 +0000, Ian Jackson wrote:> Gianni Tedesco writes ("Re: [Xen-devel] [PATCH,RFC]: Introduce libxl_domain_create()"): > > Python, for example, has no concept of an array nor any remotely similar > > data structure. > > Python calls them lists. The literal constructor is []. You can > iterate over them and efficiently retrieve and set elements by index > number. Ie, they are arrays.Hmm, they are lists :) They behave entirely like lists but look like arrays.> > The way I have developed the current xl binding is that C-structs are > > embedded inside python objects which wrap them. To be fair I could > > probably auto-generate list <-> array marshalling functions to go each > > way so it''s not impossible. > > I don''t see in what way a Python list is not an array or what the > fundamental difficulty is here.Well, when I have a list of PyFoo then I can''t back that with an array of Foo that''s all.> Ian._______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
Ian Jackson
2010-Dec-21 14:21 UTC
Re: [Xen-devel] [PATCH,RFC]: Introduce libxl_domain_create()
Ian Campbell writes ("Re: [Xen-devel] [PATCH,RFC]: Introduce
libxl_domain_create()"):> The opposite -- the bootloader interacts with the user via xenconsole.
So it does. I hadn''t spotted that going in. In that case I think not
setting up the console earlier is just historical.
Ian.
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel