Re: [Xen-devel] [PATCH V2 04/11] libxl, Introduce dm-version xenstore key.

[Anthony PERARD <anthony.perard@xxxxxxxxxx>]

On Mon, Oct 24, 2011 at 10:58, Ian Campbell <Ian.Campbell@xxxxxxxxxx> wrote:
> On Thu, 2011-10-20 at 18:59 +0100, Anthony PERARD wrote:
>> The all key is /libxl/$domid/dm-version.
>>
>> The /libxl/$domid dir is created with the domain and should be only 
>> accessible
>> by the toolstack domain.
>>
>> This come with libxl__device_model_version_running helper function.
>>
>> Signed-off-by: Anthony PERARD <anthony.perard@xxxxxxxxxx>
>> ---
>> Âtools/libxl/libxl.c     Â|  Â2 ++
>> Âtools/libxl/libxl_create.c  |  28 ++++++++++++++++++++++++++++
>> Âtools/libxl/libxl_internal.c | Â 19 +++++++++++++++++++
>> Âtools/libxl/libxl_internal.h | Â Â5 +++++
>> Â4 files changed, 54 insertions(+), 0 deletions(-)
>>
>> diff --git a/tools/libxl/libxl.c b/tools/libxl/libxl.c
>> index 064fbc4..50b97c2 100644
>> --- a/tools/libxl/libxl.c
>> +++ b/tools/libxl/libxl.c
>> @@ -777,6 +777,8 @@ int libxl_domain_destroy(libxl_ctx *ctx, uint32_t domid, 
>> int force)
>> Â Â Âif (!xs_rm(ctx->xsh, XBT_NULL, dom_path))
>> Â Â Â Â ÂLIBXL__LOG_ERRNO(ctx, LIBXL__LOG_ERROR, "xs_rm failed for %s", 
>> dom_path);
>>
>> + Â Âxs_rm(ctx->xsh, XBT_NULL, libxl__sprintf(&gc, "/libxl/%d", domid));
>> +
>> Â Â Âlibxl__userdata_destroyall(&gc, domid);
>>
>> Â Â Ârc = xc_domain_destroy(ctx->xch, domid);
>> diff --git a/tools/libxl/libxl_create.c b/tools/libxl/libxl_create.c
>> index 68d0fc3..bed991c 100644
>> --- a/tools/libxl/libxl_create.c
>> +++ b/tools/libxl/libxl_create.c
>> @@ -322,6 +322,9 @@ int libxl__domain_make(libxl__gc *gc, 
>> libxl_domain_create_info *info,
>> Â Â Âxs_transaction_t t = 0;
>> Â Â Âxen_domain_handle_t handle;
>>
>> + Â Âstruct xs_permissions libxlperm[1];
>> + Â Âchar *libxl_path = NULL;
>> +
>
> The variable declaration block has little sections of perms and *_path
> -- you might as well include those there. libxlperm might be better
> called noperm (assuming that really is it's meaning, xs's permission
> scheme confuses the hell out of me).

OK, I will change that.

>> Â Â Âassert(!libxl_domid_valid_guest(*domid));
>>
>> Â Â Âuuid_string = libxl__uuid2string(gc, info->uuid);
>> @@ -368,6 +371,15 @@ int libxl__domain_make(libxl__gc *gc, 
>> libxl_domain_create_info *info,
>> Â Â Â Â Âgoto out;
>> Â Â Â}
>>
>> + Â Âlibxl_path = libxl__sprintf(gc, "/libxl/%i", *domid);
>
> This is probably worthy of a little helper, similar to
> libxl__xs_get_dom_path(). (/vm/blah ought to have one too but doesn't).

I will do it.

>> + Â Âif (!libxl_path) {
>> + Â Â Â ÂLIBXL__LOG(ctx, LIBXL__LOG_ERROR, "cannot allocate create paths");
>> + Â Â Â Ârc = ERROR_FAIL;
>> + Â Â Â Âgoto out;
>> + Â Â}
>> + Â Âlibxlperm[0].id = 0;
>> + Â Âlibxlperm[0].perms = XS_PERM_NONE;
>> +
>> Â Â Âroperm[0].id = 0;
>> Â Â Âroperm[0].perms = XS_PERM_NONE;
>> Â Â Âroperm[1].id = *domid;
>> @@ -386,6 +398,10 @@ retry_transaction:
>> Â Â Âxs_mkdir(ctx->xsh, t, vm_path);
>> Â Â Âxs_set_permissions(ctx->xsh, t, vm_path, roperm, ARRAY_SIZE(roperm));
>>
>> + Â Âxs_rm(ctx->xsh, t, libxl_path);
>> + Â Âxs_mkdir(ctx->xsh, t, libxl_path);
>> + Â Âxs_set_permissions(ctx->xsh, t, libxl_path, libxlperm, 
>> ARRAY_SIZE(libxlperm));
>> +
>> Â Â Âxs_write(ctx->xsh, t, libxl__sprintf(gc, "%s/vm", dom_path), vm_path, 
>> strlen(vm_path));
>> Â Â Ârc = libxl__domain_rename(gc, *domid, 0, info->name, t);
>> Â Â Âif (rc)
>> @@ -429,6 +445,16 @@ retry_transaction:
>> Â Â Âreturn rc;
>> Â}
>>
>> +static int store_libxl_entry(libxl__gc *gc, uint32_t domid,
>> + Â Â Â Â Â Â Â Â Â Â Â Â Â Â libxl_device_model_info *dm_info)
>> +{
>> + Â Âchar *path = NULL;
>> +
>> + Â Âpath = libxl__sprintf(gc, "/libxl/%i/dm-version", domid);
>> + Â Âreturn libxl__xs_write(gc, XBT_NULL, path, libxl__strdup(gc,
>> + Â Â Â Â
>> libxl_device_model_version_to_string(dm_info->device_model_version)));
>> +}
>> +
>> Âstatic int do_domain_create(libxl__gc *gc, libxl_domain_config *d_config,
>> Â Â Â Â Â Â Â Â Â Â Â Â Â Â Âlibxl_console_ready cb, void *priv,
>> Â Â Â Â Â Â Â Â Â Â Â Â Â Â Âuint32_t *domid_out, int restore_fd)
>> @@ -485,6 +511,8 @@ static int do_domain_create(libxl__gc *gc, 
>> libxl_domain_config *d_config,
>> Â Â Â Â Âgoto error_out;
>> Â Â Â}
>>
>> + Â Âstore_libxl_entry(gc, domid, dm_info);
>> +
>> Â Â Âfor (i = 0; i < d_config->num_disks; i++) {
>> Â Â Â Â Âret = libxl_device_disk_add(ctx, domid, &d_config->disks[i]);
>> Â Â Â Â Âif (ret) {
>> diff --git a/tools/libxl/libxl_internal.c b/tools/libxl/libxl_internal.c
>> index 3993d8e..5d0a2d4 100644
>> --- a/tools/libxl/libxl_internal.c
>> +++ b/tools/libxl/libxl_internal.c
>> @@ -319,6 +319,25 @@ int libxl__fd_set_cloexec(int fd)
>> Â Â Âreturn fcntl(fd, F_SETFD, flags | FD_CLOEXEC);
>> Â}
>>
>> +libxl_device_model_version libxl__device_model_version_running(libxl__gc 
>> *gc,
>> + Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â uint32_t 
>> domid)
>> +{
>> + Â Âchar *path = NULL;
>> + Â Âchar *dm_version = NULL;
>> + Â Âlibxl_device_model_version value;
>> +
>> + Â Âpath = libxl__sprintf(gc, "/libxl/%d/dm-version", domid);
>> + Â Âdm_version = libxl__xs_read(gc, XBT_NULL, path);
>> + Â Âif (!dm_version) {
>> + Â Â Â Âreturn LIBXL_DEVICE_MODEL_VERSION_QEMU_XEN_TRADITIONAL;
>> + Â Â}
>> +
>> + Â Âif (libxl_device_model_version_from_string(dm_version, &value) < 0) {
>
> I think this should be a fatal error, if you've managed to read
> something from this key and it isn't one of the expect values then
> something bad has happened.

Yes, I will change this to a fatal error.

>> + Â Â Â Âreturn LIBXL_DEVICE_MODEL_VERSION_QEMU_XEN_TRADITIONAL;
>> + Â Â}
>> + Â Âreturn value;
>> +}
>> +
>> Â/*
>> Â * Local variables:
>> Â * mode: C
>> diff --git a/tools/libxl/libxl_internal.h b/tools/libxl/libxl_internal.h
>> index 2e26ac6..5720b31 100644
>> --- a/tools/libxl/libxl_internal.h
>> +++ b/tools/libxl/libxl_internal.h
>> @@ -554,6 +554,11 @@ _hidden void libxl__json_object_free(libxl__gc *gc, 
>> libxl__json_object *obj);
>>
>> Â_hidden libxl__json_object *libxl__json_parse(libxl__gc *gc, const char *s);
>>
>> + Â/* Based on /local/domain/$domid/dm-version xenstore key
>> + Â * default is qemu xen traditional */
>> +_hidden libxl_device_model_version
>> +libxl__device_model_version_running(libxl__gc *gc, uint32_t domid);
>> +
>> Â#endif
>>
>> Â/*

-- 
Anthony PERARD

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