diff mbox

[v3,3/5] libxl: initialise the build info before calling prepare_config

Message ID 1453291044-83976-4-git-send-email-roger.pau@citrix.com
State New, archived
Headers show

Commit Message

Roger Pau Monné Jan. 20, 2016, 11:57 a.m. UTC
libxl__arch_domain_prepare_config has access to the libxl_domain_build_info
struct, so make sure it's properly initialised.

Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
---
Cc: Ian Jackson <ian.jackson@eu.citrix.com>
Cc: Ian Campbell <ian.campbell@citrix.com>
Cc: Wei Liu <wei.liu2@citrix.com>
---
NB: libxl__arch_domain_prepare_config is called from libxl__domain_make.
---
 tools/libxl/libxl_create.c | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

Comments

Ian Campbell Jan. 20, 2016, 12:46 p.m. UTC | #1
On Wed, 2016-01-20 at 12:57 +0100, Roger Pau Monne wrote:
> libxl__arch_domain_prepare_config has access to the
> libxl_domain_build_info
> struct, so make sure it's properly initialised.
> 
> Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
> ---
> Cc: Ian Jackson <ian.jackson@eu.citrix.com>
> Cc: Ian Campbell <ian.campbell@citrix.com>
> Cc: Wei Liu <wei.liu2@citrix.com>
> ---
> NB: libxl__arch_domain_prepare_config is called from libxl__domain_make.

I think this is worth moving into the actual commit message. Plus it would
be useful to clarify that while prepare_config has access to b_info it
doesn't touch it right now (but presumably you are about to make it do so).

If it does touch it then that is currently a bug which should be mentioned
in the commit message and tagged for backport etc.

I suspect the reason for the ordering today is that domain_make is intended
to consume create_info, not build_info. However that distinction seems to
me to be an artefact of a much older API structure which doesn't seem to
make much sense now (but we are stuck with it :-()

> ---
>  tools/libxl/libxl_create.c | 12 ++++++------
>  1 file changed, 6 insertions(+), 6 deletions(-)
> 
> diff --git a/tools/libxl/libxl_create.c b/tools/libxl/libxl_create.c
> index 61a4001..ba4c9e8 100644
> --- a/tools/libxl/libxl_create.c
> +++ b/tools/libxl/libxl_create.c
> @@ -892,6 +892,12 @@ static void initiate_domain_create(libxl__egc *egc,
>          goto error_out;
>      }
>  
> +    ret = libxl__domain_build_info_setdefault(gc, &d_config->b_info);
> +    if (ret) {
> +        LOG(ERROR, "Unable to set domain build info defaults");
> +        goto error_out;
> +    }
> +
>      ret = libxl__domain_make(gc, d_config, &domid, &state->config);
>      if (ret) {
>          LOG(ERROR, "cannot make domain: %d", ret);
> @@ -903,12 +909,6 @@ static void initiate_domain_create(libxl__egc *egc,
>      dcs->guest_domid = domid;
>      dcs->dmss.dm.guest_domid = 0; /* means we haven't spawned */
>  
> -    ret = libxl__domain_build_info_setdefault(gc, &d_config->b_info);
> -    if (ret) {
> -        LOG(ERROR, "Unable to set domain build info defaults");
> -        goto error_out;
> -    }
> -
>      if (d_config->c_info.type == LIBXL_DOMAIN_TYPE_HVM &&
>          (libxl_defbool_val(d_config->b_info.u.hvm.nested_hvm) &&
>           libxl_defbool_val(d_config->b_info.u.hvm.altp2m))) {
Roger Pau Monné Jan. 20, 2016, 3:32 p.m. UTC | #2
El 20/01/16 a les 13.46, Ian Campbell ha escrit:
> On Wed, 2016-01-20 at 12:57 +0100, Roger Pau Monne wrote:
>> libxl__arch_domain_prepare_config has access to the
>> libxl_domain_build_info
>> struct, so make sure it's properly initialised.
>>
>> Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
>> ---
>> Cc: Ian Jackson <ian.jackson@eu.citrix.com>
>> Cc: Ian Campbell <ian.campbell@citrix.com>
>> Cc: Wei Liu <wei.liu2@citrix.com>
>> ---
>> NB: libxl__arch_domain_prepare_config is called from libxl__domain_make.
> 
> I think this is worth moving into the actual commit message. Plus it would
> be useful to clarify that while prepare_config has access to b_info it
> doesn't touch it right now (but presumably you are about to make it do so).

Right, see 5/5.

> If it does touch it then that is currently a bug which should be mentioned
> in the commit message and tagged for backport etc.

No, it doesn't touch it ATM, so not a bug.

> I suspect the reason for the ordering today is that domain_make is intended
> to consume create_info, not build_info. However that distinction seems to
> me to be an artefact of a much older API structure which doesn't seem to
> make much sense now (but we are stuck with it :-()

That's right, this create_info vs build_info split seems quite
arbitrary, but I don't see myself shaving that yak right now (also
changing this is going to be a pain regarding API compatibility).

Roger.
Ian Campbell Jan. 20, 2016, 3:37 p.m. UTC | #3
On Wed, 2016-01-20 at 16:32 +0100, Roger Pau Monné wrote:
> > I suspect the reason for the ordering today is that domain_make is intended
> > to consume create_info, not build_info. However that distinction seems to
> > me to be an artefact of a much older API structure which doesn't seem to
> > make much sense now (but we are stuck with it :-()
> 
> That's right, this create_info vs build_info split seems quite
> arbitrary, but I don't see myself shaving that yak right now (also
> changing this is going to be a pain regarding API compatibility).

Yeah, we can't change that in any sane way I don't think.

Ian.
diff mbox

Patch

diff --git a/tools/libxl/libxl_create.c b/tools/libxl/libxl_create.c
index 61a4001..ba4c9e8 100644
--- a/tools/libxl/libxl_create.c
+++ b/tools/libxl/libxl_create.c
@@ -892,6 +892,12 @@  static void initiate_domain_create(libxl__egc *egc,
         goto error_out;
     }
 
+    ret = libxl__domain_build_info_setdefault(gc, &d_config->b_info);
+    if (ret) {
+        LOG(ERROR, "Unable to set domain build info defaults");
+        goto error_out;
+    }
+
     ret = libxl__domain_make(gc, d_config, &domid, &state->config);
     if (ret) {
         LOG(ERROR, "cannot make domain: %d", ret);
@@ -903,12 +909,6 @@  static void initiate_domain_create(libxl__egc *egc,
     dcs->guest_domid = domid;
     dcs->dmss.dm.guest_domid = 0; /* means we haven't spawned */
 
-    ret = libxl__domain_build_info_setdefault(gc, &d_config->b_info);
-    if (ret) {
-        LOG(ERROR, "Unable to set domain build info defaults");
-        goto error_out;
-    }
-
     if (d_config->c_info.type == LIBXL_DOMAIN_TYPE_HVM &&
         (libxl_defbool_val(d_config->b_info.u.hvm.nested_hvm) &&
          libxl_defbool_val(d_config->b_info.u.hvm.altp2m))) {