[PATCH 0/3] libxl stubdom API cleanup

[PATCH 0/3] libxl stubdom API cleanup

[Vincent Hanquez]

[-- Attachment #1: Type: text/plain, Size: 699 bytes --]

This patch serie cleans the stubdom API, and remove some of the policy embedded
in libxl. libxl users are expected to start explicitely a stubdomain instead of relying
on a magic value in devicemodel_create.

Vincent Hanquez (3):
  move stubdom make into a proper function
  let the user of libxenlight choose explicitely if it want to start a
    stubdom or not.
  stubdom_create returns stubdomain domid so that unpause is done by
    the user of libxenlight.

 tools/libxl/libxl.c      |   99 ++++++++++++++++++++++++++--------------------
 tools/libxl/libxl.h      |   25 +++++++++++-
 tools/libxl/xl_cmdimpl.c |   14 ++++++-
 3 files changed, 92 insertions(+), 46 deletions(-)


[-- Attachment #2: Type: text/plain, Size: 138 bytes --]

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

[PATCH 1/3] move stubdom make into a proper function

[Vincent Hanquez]

[-- Attachment #1: Type: text/plain, Size: 243 bytes --]


Signed-off-by: Vincent Hanquez <vincent.hanquez@eu.citrix.com>
---
 tools/libxl/libxl.c |   72 ++++++++++++++++++++++++++++++++------------------
 tools/libxl/libxl.h |   10 +++++++
 2 files changed, 56 insertions(+), 26 deletions(-)


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0001-move-stubdom-make-into-a-proper-function.patch --]
[-- Type: text/x-patch; name="0001-move-stubdom-make-into-a-proper-function.patch", Size: 5500 bytes --]

diff --git a/tools/libxl/libxl.c b/tools/libxl/libxl.c
index 293aaa7..bf2c7e8 100644
--- a/tools/libxl/libxl.c
+++ b/tools/libxl/libxl.c
@@ -80,7 +80,7 @@ int libxl_ctx_free(struct libxl_ctx *ctx)
 /******************************************************************************/
 
 int libxl_domain_make(struct libxl_ctx *ctx, libxl_domain_create_info *info,
-                       uint32_t *domid)
+                      uint32_t *domid)
 {
     int flags, ret, i, rc;
     char *uuid_string;
@@ -1020,6 +1020,41 @@ retry_transaction:
     return 0;
 }
 
+int libxl_stubdom_make(struct libxl_ctx *ctx, libxl_stubdom_create_info *info,
+                       libxl_domain_build_state *state, uint32_t *domid)
+{
+    int ret, i;
+    libxl_domain_create_info c_info;
+    libxl_domain_build_info b_info;
+
+    memset(&c_info, 0x00, sizeof(libxl_domain_create_info));
+    c_info.hvm = 0;
+    c_info.name = libxl_sprintf(ctx, "%s-dm", libxl_domid_to_name(ctx, info->target_domid));
+    for (i = 0; i < 16; i++)
+        c_info.uuid[i] = info->uuid[i];
+
+    memset(&b_info, 0x00, sizeof(libxl_domain_build_info));
+    b_info.max_vcpus = 1;
+    b_info.max_memkb = info->max_memkb;
+    b_info.target_memkb = b_info.max_memkb;
+    b_info.kernel = libxl_abs_path(ctx, "ioemu-stubdom.gz", libxl_xenfirmwaredir_path());
+    b_info.u.pv.cmdline = libxl_sprintf(ctx, " -d %d", info->target_domid);
+    b_info.u.pv.ramdisk = "";
+    b_info.u.pv.features = "";
+    b_info.hvm = 0;
+
+    ret = libxl_domain_make(ctx, &c_info, domid);
+    if (ret)
+        return ret;
+    ret = libxl_domain_build(ctx, &b_info, *domid, state);
+    if (ret)
+        return ret;
+
+    xc_domain_set_target(ctx->xch, *domid, info->target_domid);
+    xs_set_target(ctx->xsh, *domid, info->target_domid);
+    return 0;
+}
+
 static int libxl_create_stubdom(struct libxl_ctx *ctx,
                                 libxl_device_model_info *info,
                                 libxl_device_disk *disks, int num_disks,
@@ -1030,40 +1065,27 @@ static int libxl_create_stubdom(struct libxl_ctx *ctx,
 {
     int i, num_console = 1, ret;
     libxl_device_console *console;
-    libxl_domain_create_info c_info;
-    libxl_domain_build_info b_info;
     libxl_domain_build_state state;
     uint32_t domid;
     char **args;
     struct xs_permissions perm[2];
     xs_transaction_t t;
     libxl_device_model_starting *dm_starting = 0;
+    libxl_stubdom_create_info stubinfo;
+
+    /* initialize stubinfo create structure */
+    stubinfo.target_domid = info->domid;
+    stubinfo.max_memkb = 32 * 1024;
+    for (i = 0; i < 16; i++)
+        stubinfo.uuid[i] = info->uuid[i];
+    ret = libxl_stubdom_make(ctx, &stubinfo, &state, &domid);
+    if (ret)
+        return ret;
 
     args = libxl_build_device_model_args(ctx, info, vifs, num_vifs);
     if (!args)
         return ERROR_FAIL;
 
-    memset(&c_info, 0x00, sizeof(libxl_domain_create_info));
-    c_info.hvm = 0;
-    c_info.name = libxl_sprintf(ctx, "%s-dm", libxl_domid_to_name(ctx, info->domid));
-    for (i = 0; i < 16; i++)
-        c_info.uuid[i] = info->uuid[i];
-
-    memset(&b_info, 0x00, sizeof(libxl_domain_build_info));
-    b_info.max_vcpus = 1;
-    b_info.max_memkb = 32 * 1024;
-    b_info.target_memkb = b_info.max_memkb;
-    b_info.kernel = libxl_abs_path(ctx, "ioemu-stubdom.gz", libxl_xenfirmwaredir_path());
-    b_info.u.pv.cmdline = libxl_sprintf(ctx, " -d %d", info->domid);
-    b_info.u.pv.ramdisk = "";
-    b_info.u.pv.features = "";
-    b_info.hvm = 0;
-
-    ret = libxl_domain_make(ctx, &c_info, &domid);
-    if (ret) return ret;
-    ret = libxl_domain_build(ctx, &b_info, domid, &state);
-    if (ret) return ret;
-
     libxl_write_dmargs(ctx, domid, info->domid, args);
     libxl_xs_write(ctx, XBT_NULL,
                    libxl_sprintf(ctx, "%s/image/device-model-domid", libxl_xs_get_dompath(ctx, info->domid)),
@@ -1071,8 +1093,6 @@ static int libxl_create_stubdom(struct libxl_ctx *ctx,
     libxl_xs_write(ctx, XBT_NULL,
                    libxl_sprintf(ctx, "%s/target", libxl_xs_get_dompath(ctx, domid)),
                    "%d", info->domid);
-    xc_domain_set_target(ctx->xch, domid, info->domid);
-    xs_set_target(ctx->xsh, domid, info->domid);
 
     perm[0].id = domid;
     perm[0].perms = XS_PERM_NONE;
diff --git a/tools/libxl/libxl.h b/tools/libxl/libxl.h
index 85f8196..a0a3a1a 100644
--- a/tools/libxl/libxl.h
+++ b/tools/libxl/libxl.h
@@ -121,6 +121,13 @@ typedef struct {
 } libxl_domain_build_info;
 
 typedef struct {
+    uint8_t uuid[16];
+    char *name;
+    uint32_t max_memkb;
+    uint32_t target_domid;
+} libxl_stubdom_create_info;
+
+typedef struct {
     uint32_t store_port;
     unsigned long store_mfn;
     uint32_t console_port;
@@ -364,6 +371,9 @@ struct libxl_dominfo * libxl_list_domain(struct libxl_ctx*, int *nb_domain);
 struct libxl_poolinfo * libxl_list_pool(struct libxl_ctx*, int *nb_pool);
 struct libxl_vminfo * libxl_list_vm(struct libxl_ctx *ctx, int *nb_vm);
 
+int libxl_stubdom_make(struct libxl_ctx *ctx, libxl_stubdom_create_info *info,
+                       libxl_domain_build_state *state, uint32_t *domid);
+
 typedef struct libxl_device_model_starting libxl_device_model_starting;
 int libxl_create_device_model(struct libxl_ctx *ctx,
                               libxl_device_model_info *info,

[-- Attachment #3: Type: text/plain, Size: 138 bytes --]

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

[PATCH 2/3] let the user of libxenlight choose explicitely if it want to start a stubdom or not.

[Vincent Hanquez]

[-- Attachment #1: Type: text/plain, Size: 521 bytes --]


the same mechanism of choosing stubdomain or not, depending on the qemu-dm
filename that was embedded in the libxenlight is moved to xl so that nothing
change. also expose more functions that the caller is expected to used now.

Signed-off-by: Vincent Hanquez <vincent.hanquez@eu.citrix.com>
---
 tools/libxl/libxl.c      |   30 +++++++++++-------------------
 tools/libxl/libxl.h      |   15 ++++++++++++++-
 tools/libxl/xl_cmdimpl.c |   12 ++++++++++--
 3 files changed, 35 insertions(+), 22 deletions(-)


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0002-let-the-user-of-libxenlight-choose-explicitely-if-it.patch --]
[-- Type: text/x-patch; name="0002-let-the-user-of-libxenlight-choose-explicitely-if-it.patch", Size: 5252 bytes --]

diff --git a/tools/libxl/libxl.c b/tools/libxl/libxl.c
index bf2c7e8..811678f 100644
--- a/tools/libxl/libxl.c
+++ b/tools/libxl/libxl.c
@@ -949,10 +949,10 @@ void dm_xenstore_record_pid(void *for_spawn, pid_t innerchild)
     xs_daemon_close(xsh);
 }
 
-static int libxl_vfb_and_vkb_from_device_model_info(struct libxl_ctx *ctx,
-                                                    libxl_device_model_info *info,
-                                                    libxl_device_vfb *vfb,
-                                                    libxl_device_vkb *vkb)
+int libxl_vfb_and_vkb_from_device_model_info(struct libxl_ctx *ctx,
+                                             libxl_device_model_info *info,
+                                             libxl_device_vfb *vfb,
+                                             libxl_device_vkb *vkb)
 {
     memset(vfb, 0x00, sizeof(libxl_device_vfb));
     memset(vkb, 0x00, sizeof(libxl_device_vkb));
@@ -1055,13 +1055,13 @@ int libxl_stubdom_make(struct libxl_ctx *ctx, libxl_stubdom_create_info *info,
     return 0;
 }
 
-static int libxl_create_stubdom(struct libxl_ctx *ctx,
-                                libxl_device_model_info *info,
-                                libxl_device_disk *disks, int num_disks,
-                                libxl_device_nic *vifs, int num_vifs,
-                                libxl_device_vfb *vfb,
-                                libxl_device_vkb *vkb,
-                                libxl_device_model_starting **starting_r)
+int libxl_stubdom_create(struct libxl_ctx *ctx,
+                         libxl_device_model_info *info,
+                         libxl_device_disk *disks, int num_disks,
+                         libxl_device_nic *vifs, int num_vifs,
+                         libxl_device_vfb *vfb,
+                         libxl_device_vkb *vkb,
+                         libxl_device_model_starting **starting_r)
 {
     int i, num_console = 1, ret;
     libxl_device_console *console;
@@ -1178,14 +1178,6 @@ int libxl_create_device_model(struct libxl_ctx *ctx,
     char *vm_path;
     char **pass_stuff;
 
-    if (strstr(info->device_model, "stubdom-dm")) {
-        libxl_device_vfb vfb;
-        libxl_device_vkb vkb;
-
-        libxl_vfb_and_vkb_from_device_model_info(ctx, info, &vfb, &vkb);
-        return libxl_create_stubdom(ctx, info, disks, num_disks, vifs, num_vifs, &vfb, &vkb, starting_r);
-    }
-
     args = libxl_build_device_model_args(ctx, info, vifs, num_vifs);
     if (!args)
         return ERROR_FAIL;
diff --git a/tools/libxl/libxl.h b/tools/libxl/libxl.h
index a0a3a1a..76f9b5c 100644
--- a/tools/libxl/libxl.h
+++ b/tools/libxl/libxl.h
@@ -336,6 +336,7 @@ typedef struct {
     char *token;
 } libxl_waiter;
 
+typedef struct libxl_device_model_starting libxl_device_model_starting;
 
 int libxl_get_wait_fd(struct libxl_ctx *ctx, int *fd);
 /* waiter is allocated by the caller */
@@ -374,7 +375,19 @@ struct libxl_vminfo * libxl_list_vm(struct libxl_ctx *ctx, int *nb_vm);
 int libxl_stubdom_make(struct libxl_ctx *ctx, libxl_stubdom_create_info *info,
                        libxl_domain_build_state *state, uint32_t *domid);
 
-typedef struct libxl_device_model_starting libxl_device_model_starting;
+int libxl_stubdom_create(struct libxl_ctx *ctx,
+                         libxl_device_model_info *info,
+                         libxl_device_disk *disks, int num_disks,
+                         libxl_device_nic *vifs, int num_vifs,
+                         libxl_device_vfb *vfb,
+                         libxl_device_vkb *vkb,
+                         libxl_device_model_starting **starting_r);
+
+int libxl_vfb_and_vkb_from_device_model_info(struct libxl_ctx *ctx,
+                                             libxl_device_model_info *info,
+                                             libxl_device_vfb *vfb,
+                                             libxl_device_vkb *vkb);
+
 int libxl_create_device_model(struct libxl_ctx *ctx,
                               libxl_device_model_info *info,
                               libxl_device_disk *disk, int num_disks,
diff --git a/tools/libxl/xl_cmdimpl.c b/tools/libxl/xl_cmdimpl.c
index 208ecd6..a20e77a 100644
--- a/tools/libxl/xl_cmdimpl.c
+++ b/tools/libxl/xl_cmdimpl.c
@@ -1151,8 +1151,16 @@ start:
     }
     if (info1.hvm) {
         dm_info.domid = domid;
-        MUST( libxl_create_device_model(&ctx, &dm_info, disks, num_disks,
-                                        vifs, num_vifs, &dm_starting) );
+        if (strstr(dm_info.device_model, "stubdom-dm")) {
+            libxl_device_vfb vfb;
+            libxl_device_vkb vkb;
+
+            libxl_vfb_and_vkb_from_device_model_info(&ctx, &dm_info, &vfb, &vkb);
+            MUST( libxl_stubdom_create(&ctx, &dm_info, disks, num_disks, vifs, num_vifs, &vfb, &vkb, &dm_starting) );
+        } else {
+            MUST( libxl_create_device_model(&ctx, &dm_info, disks, num_disks,
+                                            vifs, num_vifs, &dm_starting) );
+        }
     } else {
         for (i = 0; i < num_vfbs; i++) {
             vfbs[i].domid = domid;

[-- Attachment #3: Type: text/plain, Size: 138 bytes --]

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

[PATCH 3/3] stubdom_create returns stubdomain domid so that unpause is done by the user of libxenlight.

[Vincent Hanquez]

[-- Attachment #1: Type: text/plain, Size: 242 bytes --]


Signed-off-by: Vincent Hanquez <vincent.hanquez@eu.citrix.com>
---
 tools/libxl/libxl.c      |    7 ++++---
 tools/libxl/libxl.h      |    2 +-
 tools/libxl/xl_cmdimpl.c |    4 +++-
 3 files changed, 8 insertions(+), 5 deletions(-)


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0003-stubdom_create-returns-stubdomain-domid-so-that-unpa.patch --]
[-- Type: text/x-patch; name="0003-stubdom_create-returns-stubdomain-domid-so-that-unpa.patch", Size: 2819 bytes --]

diff --git a/tools/libxl/libxl.c b/tools/libxl/libxl.c
index 811678f..29fb10f 100644
--- a/tools/libxl/libxl.c
+++ b/tools/libxl/libxl.c
@@ -1060,7 +1060,7 @@ int libxl_stubdom_create(struct libxl_ctx *ctx,
                          libxl_device_disk *disks, int num_disks,
                          libxl_device_nic *vifs, int num_vifs,
                          libxl_device_vfb *vfb,
-                         libxl_device_vkb *vkb,
+                         libxl_device_vkb *vkb, uint32_t *stubdomid,
                          libxl_device_model_starting **starting_r)
 {
     int i, num_console = 1, ret;
@@ -1141,6 +1141,9 @@ retry_transaction:
         ret = libxl_device_console_add(ctx, domid, &console[i]);
         if (ret) return ret;
     }
+
+    *stubdomid = domid;
+
     if (libxl_create_xenpv_qemu(ctx, vfb, num_console, console, &dm_starting) < 0) {
         free(args);
         return -1;
@@ -1150,8 +1153,6 @@ retry_transaction:
         return -1;
     }
 
-    libxl_domain_unpause(ctx, domid);
-
     if (starting_r) {
         *starting_r = libxl_calloc(ctx, sizeof(libxl_device_model_starting), 1);
         (*starting_r)->domid = info->domid;
diff --git a/tools/libxl/libxl.h b/tools/libxl/libxl.h
index 76f9b5c..597d91b 100644
--- a/tools/libxl/libxl.h
+++ b/tools/libxl/libxl.h
@@ -381,7 +381,7 @@ int libxl_stubdom_create(struct libxl_ctx *ctx,
                          libxl_device_nic *vifs, int num_vifs,
                          libxl_device_vfb *vfb,
                          libxl_device_vkb *vkb,
-                         libxl_device_model_starting **starting_r);
+                         uint32_t *stubdomid, libxl_device_model_starting **starting_r);
 
 int libxl_vfb_and_vkb_from_device_model_info(struct libxl_ctx *ctx,
                                              libxl_device_model_info *info,
diff --git a/tools/libxl/xl_cmdimpl.c b/tools/libxl/xl_cmdimpl.c
index a20e77a..6ed8e26 100644
--- a/tools/libxl/xl_cmdimpl.c
+++ b/tools/libxl/xl_cmdimpl.c
@@ -1154,9 +1154,11 @@ start:
         if (strstr(dm_info.device_model, "stubdom-dm")) {
             libxl_device_vfb vfb;
             libxl_device_vkb vkb;
+            uint32_t stubdomid;
 
             libxl_vfb_and_vkb_from_device_model_info(&ctx, &dm_info, &vfb, &vkb);
-            MUST( libxl_stubdom_create(&ctx, &dm_info, disks, num_disks, vifs, num_vifs, &vfb, &vkb, &dm_starting) );
+            MUST( libxl_stubdom_create(&ctx, &dm_info, disks, num_disks, vifs, num_vifs, &vfb, &vkb, &stubdomid, &dm_starting) );
+            libxl_domain_unpause(&ctx, stubdomid);
         } else {
             MUST( libxl_create_device_model(&ctx, &dm_info, disks, num_disks,
                                             vifs, num_vifs, &dm_starting) );

[-- Attachment #3: Type: text/plain, Size: 138 bytes --]

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

Re: [PATCH 0/3] libxl stubdom API cleanup

[Stefano Stabellini]

On Wed, 7 Jul 2010, Vincent Hanquez wrote:
> This patch serie cleans the stubdom API, and remove some of the policy embedded
> in libxl. libxl users are expected to start explicitely a stubdomain instead of relying
> on a magic value in devicemodel_create.
> 
> Vincent Hanquez (3):
>   move stubdom make into a proper function
>   let the user of libxenlight choose explicitely if it want to start a
>     stubdom or not.
>   stubdom_create returns stubdomain domid so that unpause is done by
>     the user of libxenlight.
> 

I though we wanted to make stubdoms transparent to libxenlight users,
why do you want to expose them now?

Re: [PATCH 0/3] libxl stubdom API cleanup

[Vincent Hanquez]

On 07/07/10 17:53, Stefano Stabellini wrote:
> I though we wanted to make stubdoms transparent to libxenlight users,
> why do you want to expose them now?
>    
 From the users yes, from the libxenlight users (aka developers) no.
It's also a good way to get the policy out of libxenlight. For example the
32mb value which might or might not change in future.

-- 
Vincent

Re: [PATCH 0/3] libxl stubdom API cleanup

[Stefano Stabellini]

On Thu, 8 Jul 2010, Vincent Hanquez wrote:
> On 07/07/10 17:53, Stefano Stabellini wrote:
> > I though we wanted to make stubdoms transparent to libxenlight users,
> > why do you want to expose them now?
> >    
>  From the users yes, from the libxenlight users (aka developers) no.
> It's also a good way to get the policy out of libxenlight. For example the
> 32mb value which might or might not change in future.
 
Fair enough.
I ack the whole series then.

Re: [PATCH 0/3] libxl stubdom API cleanup

[Ian Campbell]

On Thu, 2010-07-08 at 15:03 +0100, Stefano Stabellini wrote:
> On Thu, 8 Jul 2010, Vincent Hanquez wrote:
> > On 07/07/10 17:53, Stefano Stabellini wrote:
> > > I though we wanted to make stubdoms transparent to libxenlight users,
> > > why do you want to expose them now?
> > >    
> >  From the users yes, from the libxenlight users (aka developers) no.
> > It's also a good way to get the policy out of libxenlight. For example the
> > 32mb value which might or might not change in future.
>  
> Fair enough.
> I ack the whole series then.

Is it necessary to pull the mechanism out along with the policy though? 

Could the libxl user not specify one of nostubdom, stubdom or
libxlchooses (the default?) and let the internals of libxl take care of
actually starting it etc?

Ian.

Re: [PATCH 0/3] libxl stubdom API cleanup

[Ian Jackson]

Ian Campbell writes ("Re: [Xen-devel] [PATCH 0/3] libxl stubdom API cleanup"):
> Could the libxl user not specify one of nostubdom, stubdom or
> libxlchooses (the default?) and let the internals of libxl take care of
> actually starting it etc?

Quite so.

Ian.

Re: [PATCH 0/3] libxl stubdom API cleanup

[Vincent Hanquez]

On 08/07/10 15:18, Ian Campbell wrote:
> On Thu, 2010-07-08 at 15:03 +0100, Stefano Stabellini wrote:
>> On Thu, 8 Jul 2010, Vincent Hanquez wrote:
>>> On 07/07/10 17:53, Stefano Stabellini wrote:
>>>> I though we wanted to make stubdoms transparent to libxenlight users,
>>>> why do you want to expose them now?
>>>>
>>>    From the users yes, from the libxenlight users (aka developers) no.
>>> It's also a good way to get the policy out of libxenlight. For example the
>>> 32mb value which might or might not change in future.
>>
>> Fair enough.
>> I ack the whole series then.
>
> Is it necessary to pull the mechanism out along with the policy though?

Necessary is quite a strong word.

> Could the libxl user not specify one of nostubdom, stubdom or
> libxlchooses (the default?) and let the internals of libxl take care of
> actually starting it etc?

Starting a stubdom or not, imply 2 very different side effects (e.g. 
memory wise). Separating the API give better error reporting, more room 
for action (e.g. creating a domain without stubdom if you don't have 
those N mb to spare), and it also simplify the ocaml bindings not having 
to encode complex semantics on the ocaml side.

-- 
Vincent

Re: [PATCH 0/3] libxl stubdom API cleanup

[Ian Campbell]

On Thu, 2010-07-08 at 18:18 +0100, Vincent Hanquez wrote:
> On 08/07/10 15:18, Ian Campbell wrote:
> > On Thu, 2010-07-08 at 15:03 +0100, Stefano Stabellini wrote:
> >> On Thu, 8 Jul 2010, Vincent Hanquez wrote:
> >>> On 07/07/10 17:53, Stefano Stabellini wrote:
> >>>> I though we wanted to make stubdoms transparent to libxenlight users,
> >>>> why do you want to expose them now?
> >>>>
> >>>    From the users yes, from the libxenlight users (aka developers) no.
> >>> It's also a good way to get the policy out of libxenlight. For example the
> >>> 32mb value which might or might not change in future.
> >>
> >> Fair enough.
> >> I ack the whole series then.
> >
> > Is it necessary to pull the mechanism out along with the policy though?
> 
> Necessary is quite a strong word.
> 
> > Could the libxl user not specify one of nostubdom, stubdom or
> > libxlchooses (the default?) and let the internals of libxl take care of
> > actually starting it etc?
> 
> Starting a stubdom or not, imply 2 very different side effects (e.g. 
> memory wise). Separating the API give better error reporting, more room 
> for action (e.g. creating a domain without stubdom if you don't have 
> those N mb to spare), and it also simplify the ocaml bindings not having 
> to encode complex semantics on the ocaml side.

Fair enough.

Re: [PATCH 0/3] libxl stubdom API cleanup

[Tim Deegan]

At 15:18 +0100 on 08 Jul (1278602309), Ian Campbell wrote:
> On Thu, 2010-07-08 at 15:03 +0100, Stefano Stabellini wrote:
> > On Thu, 8 Jul 2010, Vincent Hanquez wrote:
> > > On 07/07/10 17:53, Stefano Stabellini wrote:
> > > > I though we wanted to make stubdoms transparent to libxenlight users,
> > > > why do you want to expose them now?
> > > >    
> > >  From the users yes, from the libxenlight users (aka developers) no.
> > > It's also a good way to get the policy out of libxenlight. For example the
> > > 32mb value which might or might not change in future.
> >  
> > Fair enough.
> > I ack the whole series then.
> 
> Is it necessary to pull the mechanism out along with the policy though? 

Or, if we're taking some mechanism out, couldn't we take _all_ the
mechanism out?  The idea of a stub domain doesn't seem like one that
libxl necessarily needs to know about.

Tim.

-- 
Tim Deegan <Tim.Deegan@citrix.com>
Principal Software Engineer, XenServer Engineering
Citrix Systems UK Ltd.  (Company #02937203, SL9 0BG)

Re: [PATCH 0/3] libxl stubdom API cleanup

[Vincent Hanquez]

On 09/07/10 09:17, Tim Deegan wrote:
>> Is it necessary to pull the mechanism out along with the policy though?
>>      
> Or, if we're taking some mechanism out, couldn't we take _all_ the
> mechanism out?

Which one do you have in minds ?

> The idea of a stub domain doesn't seem like one that
> libxl necessarily needs to know about.
>    
yes, indeed. the stubdom create could move as a xl helper.
On the ocaml side reimplementing stubdom create is a trivial composition 
of smaller libxl function (create/build/add devs) which are already bound.

-- 
Vincent

Re: [PATCH 0/3] libxl stubdom API cleanup

[Tim Deegan]

At 11:44 +0100 on 09 Jul (1278675850), Vincent Hanquez wrote:
> On 09/07/10 09:17, Tim Deegan wrote:
> >> Is it necessary to pull the mechanism out along with the policy though?
> >>      
> > Or, if we're taking some mechanism out, couldn't we take _all_ the
> > mechanism out?
> 
> Which one do you have in minds ?

It looks like your patch leaves some "create a stubdom" functions in the
libxl API.  I'd have thought libxl should either handle stubdoms
entirely or not at all.  (Unless stubdom creation needs some low-level
grunge that will uglify the libxl API if it's exposed that far up - I
can't think of any except PRIV_FOR though).

> > The idea of a stub domain doesn't seem like one that
> > libxl necessarily needs to know about.
> >    
> yes, indeed. the stubdom create could move as a xl helper.
> On the ocaml side reimplementing stubdom create is a trivial composition 
> of smaller libxl function (create/build/add devs) which are already bound.

That sounds cleaner to me.

Cheers,

Tim.

-- 
Tim Deegan <Tim.Deegan@citrix.com>
Principal Software Engineer, XenServer Engineering
Citrix Systems UK Ltd.  (Company #02937203, SL9 0BG)

Re: [PATCH 0/3] libxl stubdom API cleanup

[Stefano Stabellini]

On Fri, 9 Jul 2010, Tim Deegan wrote:
> At 11:44 +0100 on 09 Jul (1278675850), Vincent Hanquez wrote:
> > On 09/07/10 09:17, Tim Deegan wrote:
> > >> Is it necessary to pull the mechanism out along with the policy though?
> > >>      
> > > Or, if we're taking some mechanism out, couldn't we take _all_ the
> > > mechanism out?
> > 
> > Which one do you have in minds ?
> 
> It looks like your patch leaves some "create a stubdom" functions in the
> libxl API.  I'd have thought libxl should either handle stubdoms
> entirely or not at all.  (Unless stubdom creation needs some low-level
> grunge that will uglify the libxl API if it's exposed that far up - I
> can't think of any except PRIV_FOR though).
> 
> > > The idea of a stub domain doesn't seem like one that
> > > libxl necessarily needs to know about.
> > >    
> > yes, indeed. the stubdom create could move as a xl helper.
> > On the ocaml side reimplementing stubdom create is a trivial composition 
> > of smaller libxl function (create/build/add devs) which are already bound.
> 
> That sounds cleaner to me.
> 

I am not so sure about this: a stubdom is not just a normal PV guest, it
also needs some special plumbings in xenstore.
By design libxl clients are not required to know about xenstore,
therefore we cannot have stubdoms entirely built by libxl clients.
I think that working around this would be worse than Vincent's current
patch series.

Re: [PATCH 0/3] libxl stubdom API cleanup

[Vincent Hanquez]

On 09/07/10 11:51, Tim Deegan wrote:
> At 11:44 +0100 on 09 Jul (1278675850), Vincent Hanquez wrote:
>    
>> On 09/07/10 09:17, Tim Deegan wrote:
>>      
>>>> Is it necessary to pull the mechanism out along with the policy though?
>>>>
>>>>          
>>> Or, if we're taking some mechanism out, couldn't we take _all_ the
>>> mechanism out?
>>>        
>> Which one do you have in minds ?
>>      
> It looks like your patch leaves some "create a stubdom" functions in the
> libxl API.  I'd have thought libxl should either handle stubdoms
> entirely or not at all.  (Unless stubdom creation needs some low-level
> grunge that will uglify the libxl API if it's exposed that far up - I
> can't think of any except PRIV_FOR though).
>    
I think that either is fine from my point of view; as long as I don't 
have to capture two very different semantics (starting a program | 
starting a domain) in one call.

-- 
Vincent

Re: [PATCH 0/3] libxl stubdom API cleanup

[Tim Deegan]

At 11:59 +0100 on 09 Jul (1278676763), Stefano Stabellini wrote:
> I am not so sure about this: a stubdom is not just a normal PV guest, it
> also needs some special plumbings in xenstore.
> By design libxl clients are not required to know about xenstore,
> therefore we cannot have stubdoms entirely built by libxl clients.
> I think that working around this would be worse than Vincent's current
> patch series.

Fair enough.

Tim.

-- 
Tim Deegan <Tim.Deegan@citrix.com>
Principal Software Engineer, XenServer Engineering
Citrix Systems UK Ltd.  (Company #02937203, SL9 0BG)

Re: [PATCH 0/3] libxl stubdom API cleanup

[Ian Jackson]

Vincent Hanquez writes ("Re: [Xen-devel] [PATCH 0/3] libxl stubdom API cleanup"):
> I think that either is fine from my point of view; as long as I don't 
> have to capture two very different semantics (starting a program | 
> starting a domain) in one call.

I still disagree.  I think it would be better to hide this distinction
as much as possible.

Your key motive seems to be some problem with the ocaml bindings.
Perhaps you could explain that in more detail ?

Ian.

Re: [PATCH 0/3] libxl stubdom API cleanup

[Stefano Stabellini]

On Fri, 9 Jul 2010, Ian Jackson wrote:
> Vincent Hanquez writes ("Re: [Xen-devel] [PATCH 0/3] libxl stubdom API cleanup"):
> > I think that either is fine from my point of view; as long as I don't 
> > have to capture two very different semantics (starting a program | 
> > starting a domain) in one call.
> 
> I still disagree.  I think it would be better to hide this distinction
> as much as possible.
> 
> Your key motive seems to be some problem with the ocaml bindings.
> Perhaps you could explain that in more detail ?
 
I think Vincent wanted a different API to make memory accounting easier.

What about extending the current create_device_model API with a
more explicit stubdom flag, and a way to return the stubdom domid to the
caller?

Also the caller should be able to know in advance the amount of memory
used for the stubdom: another libxl function could be added for that
purpose.

Would that interface be flexible enough for you?

Re: [PATCH 0/3] libxl stubdom API cleanup

[Vincent Hanquez]

On 09/07/10 18:04, Ian Jackson wrote:
> Vincent Hanquez writes ("Re: [Xen-devel] [PATCH 0/3] libxl stubdom API cleanup"):
>> I think that either is fine from my point of view; as long as I don't
>> have to capture two very different semantics (starting a program |
>> starting a domain) in one call.
>
> I still disagree.  I think it would be better to hide this distinction
> as much as possible.
>
> Your key motive seems to be some problem with the ocaml bindings.
> Perhaps you could explain that in more detail ?

This has nothing to do with the ocaml bindings, but this has to do with 
who is using those bindings (i.e. a fully featured toolstack).

-- 
Vincent

Re: [PATCH 0/3] libxl stubdom API cleanup

[Ian Jackson]

--text follows this line--
Stefano Stabellini writes ("Re: [Xen-devel] [PATCH 0/3] libxl stubdom API cleanup"):
> I think Vincent wanted a different API to make memory accounting easier.

Right.

> What about extending the current create_device_model API with a
> more explicit stubdom flag, and a way to return the stubdom domid to the
> caller?
> 
> Also the caller should be able to know in advance the amount of memory
> used for the stubdom: another libxl function could be added for that
> purpose.
> 
> Would that interface be flexible enough for you?

That would address my concerns.

Ian.