diff mbox series

[v2,01/21] libxl: don't ignore the return value from xc_cpuid_apply_policy

Message ID 20210413140140.73690-2-roger.pau@citrix.com (mailing list archive)
State Superseded
Headers show
Series libs/guest: new CPUID/MSR interface | expand

Commit Message

Roger Pau Monne April 13, 2021, 2:01 p.m. UTC 
Also change libxl__cpuid_legacy to propagate the error from
xc_cpuid_apply_policy into callers.

Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
Reviewed-by: Jan Beulich <jbeulich@suse.com>
---
Changes since 1:
 - Return ERROR_FAIL on error.
---
 tools/libs/light/libxl_cpuid.c    | 15 +++++++++++----
 tools/libs/light/libxl_create.c   |  5 +++--
 tools/libs/light/libxl_dom.c      |  2 +-
 tools/libs/light/libxl_internal.h |  4 ++--
 tools/libs/light/libxl_nocpuid.c  |  5 +++--
 5 files changed, 20 insertions(+), 11 deletions(-)

Comments

Anthony PERARD April 28, 2021, 3:13 p.m. UTC | #1 
On Tue, Apr 13, 2021 at 04:01:19PM +0200, Roger Pau Monne wrote:
> Also change libxl__cpuid_legacy to propagate the error from
> xc_cpuid_apply_policy into callers.
> 
> Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
> Reviewed-by: Jan Beulich <jbeulich@suse.com>
> ---
> Changes since 1:
>  - Return ERROR_FAIL on error.
> ---
>  tools/libs/light/libxl_cpuid.c    | 15 +++++++++++----
>  tools/libs/light/libxl_create.c   |  5 +++--
>  tools/libs/light/libxl_dom.c      |  2 +-
>  tools/libs/light/libxl_internal.h |  4 ++--
>  tools/libs/light/libxl_nocpuid.c  |  5 +++--
>  5 files changed, 20 insertions(+), 11 deletions(-)
> 
> diff --git a/tools/libs/light/libxl_cpuid.c b/tools/libs/light/libxl_cpuid.c
> index 289c59c7420..539fc4869e6 100644
> --- a/tools/libs/light/libxl_cpuid.c
> +++ b/tools/libs/light/libxl_cpuid.c
> @@ -419,11 +419,13 @@ int libxl_cpuid_parse_config_xend(libxl_cpuid_policy_list *cpuid,
>      return 0;
>  }
>  
> -void libxl__cpuid_legacy(libxl_ctx *ctx, uint32_t domid, bool restore,
> -                         libxl_domain_build_info *info)
> +int libxl__cpuid_legacy(libxl_ctx *ctx, uint32_t domid, bool restore,
> +                        libxl_domain_build_info *info)
>  {
> +    GC_INIT(ctx);
>      bool pae = true;
>      bool itsc;
> +    int rc;
>  
>      /*
>       * Gross hack.  Using libxl_defbool_val() here causes libvirt to crash in
> @@ -462,8 +464,13 @@ void libxl__cpuid_legacy(libxl_ctx *ctx, uint32_t domid, bool restore,
>      itsc = (libxl_defbool_val(info->disable_migrate) ||
>              info->tsc_mode == LIBXL_TSC_MODE_ALWAYS_EMULATE);
>  
> -    xc_cpuid_apply_policy(ctx->xch, domid, restore, NULL, 0,
> -                          pae, itsc, nested_virt, info->cpuid);
> +    rc = xc_cpuid_apply_policy(ctx->xch, domid, restore, NULL, 0,
> +                               pae, itsc, nested_virt, info->cpuid);
> +    if (rc)
> +        LOGE(ERROR, "Failed to apply CPUID policy");

Is logging `errno` is going to be accurate enough ? The
xc_cpuid_apply_policy() seems to only set `rc` and never change `errno`
directly. It would often return "-errno" or an error code. There's one
instance where we have "rc = -EOPNOTSUPP" but `errno` doesn't seems to
be change to the same value (when checking "featureset).

So maybe having "LOGEV(ERROR, -rc, ...)" would be better?

And it should be `r` instead of `rc`. The latter is for libxl error
code, the former for system call/libxc.

> +
> +    GC_FREE;
> +    return rc ? ERROR_FAIL : 0;

The rest looks good.

Thanks,
diff mbox series

Patch

diff --git a/tools/libs/light/libxl_cpuid.c b/tools/libs/light/libxl_cpuid.c
index 289c59c7420..539fc4869e6 100644
--- a/tools/libs/light/libxl_cpuid.c
+++ b/tools/libs/light/libxl_cpuid.c
@@ -419,11 +419,13 @@  int libxl_cpuid_parse_config_xend(libxl_cpuid_policy_list *cpuid,
     return 0;
 }
 
-void libxl__cpuid_legacy(libxl_ctx *ctx, uint32_t domid, bool restore,
-                         libxl_domain_build_info *info)
+int libxl__cpuid_legacy(libxl_ctx *ctx, uint32_t domid, bool restore,
+                        libxl_domain_build_info *info)
 {
+    GC_INIT(ctx);
     bool pae = true;
     bool itsc;
+    int rc;
 
     /*
      * Gross hack.  Using libxl_defbool_val() here causes libvirt to crash in
@@ -462,8 +464,13 @@  void libxl__cpuid_legacy(libxl_ctx *ctx, uint32_t domid, bool restore,
     itsc = (libxl_defbool_val(info->disable_migrate) ||
             info->tsc_mode == LIBXL_TSC_MODE_ALWAYS_EMULATE);
 
-    xc_cpuid_apply_policy(ctx->xch, domid, restore, NULL, 0,
-                          pae, itsc, nested_virt, info->cpuid);
+    rc = xc_cpuid_apply_policy(ctx->xch, domid, restore, NULL, 0,
+                               pae, itsc, nested_virt, info->cpuid);
+    if (rc)
+        LOGE(ERROR, "Failed to apply CPUID policy");
+
+    GC_FREE;
+    return rc ? ERROR_FAIL : 0;
 }
 
 static const char *input_names[2] = { "leaf", "subleaf" };
diff --git a/tools/libs/light/libxl_create.c b/tools/libs/light/libxl_create.c
index 0c64268f66d..2faa96d9c68 100644
--- a/tools/libs/light/libxl_create.c
+++ b/tools/libs/light/libxl_create.c
@@ -1443,6 +1443,7 @@  int libxl__srm_callout_callback_static_data_done(unsigned int missing,
 
     libxl_domain_config *d_config = dcs->guest_config;
     libxl_domain_build_info *info = &d_config->b_info;
+    int rc = 0;
 
     /*
      * CPUID/MSR information is not present in pre Xen-4.14 streams.
@@ -1452,9 +1453,9 @@  int libxl__srm_callout_callback_static_data_done(unsigned int missing,
      * stream doesn't contain any CPUID data.
      */
     if (missing & XGR_SDD_MISSING_CPUID)
-        libxl__cpuid_legacy(ctx, dcs->guest_domid, true, info);
+        rc = libxl__cpuid_legacy(ctx, dcs->guest_domid, true, info);
 
-    return 0;
+    return rc;
 }
 
 void libxl__srm_callout_callback_restore_results(xen_pfn_t store_mfn,
diff --git a/tools/libs/light/libxl_dom.c b/tools/libs/light/libxl_dom.c
index 842a51c86cb..e9f58ee4b2b 100644
--- a/tools/libs/light/libxl_dom.c
+++ b/tools/libs/light/libxl_dom.c
@@ -384,7 +384,7 @@  int libxl__build_pre(libxl__gc *gc, uint32_t domid,
      * being migrated-in/restored have CPUID handled during the
      * static_data_done() callback. */
     if (!state->restore)
-        libxl__cpuid_legacy(ctx, domid, false, info);
+        rc = libxl__cpuid_legacy(ctx, domid, false, info);
 
     return rc;
 }
diff --git a/tools/libs/light/libxl_internal.h b/tools/libs/light/libxl_internal.h
index c6a4a187f5b..44a2f3c8fe3 100644
--- a/tools/libs/light/libxl_internal.h
+++ b/tools/libs/light/libxl_internal.h
@@ -2052,8 +2052,8 @@  typedef yajl_gen_status (*libxl__gen_json_callback)(yajl_gen hand, void *);
 _hidden char *libxl__object_to_json(libxl_ctx *ctx, const char *type,
                                     libxl__gen_json_callback gen, void *p);
 
-_hidden void libxl__cpuid_legacy(libxl_ctx *ctx, uint32_t domid, bool retore,
-                                 libxl_domain_build_info *info);
+_hidden int libxl__cpuid_legacy(libxl_ctx *ctx, uint32_t domid, bool retore,
+                                libxl_domain_build_info *info);
 
 /* Calls poll() again - useful to check whether a signaled condition
  * is still true.  Cannot fail.  Returns currently-true revents. */
diff --git a/tools/libs/light/libxl_nocpuid.c b/tools/libs/light/libxl_nocpuid.c
index f47336565b9..0630959e760 100644
--- a/tools/libs/light/libxl_nocpuid.c
+++ b/tools/libs/light/libxl_nocpuid.c
@@ -34,9 +34,10 @@  int libxl_cpuid_parse_config_xend(libxl_cpuid_policy_list *cpuid,
     return 0;
 }
 
-void libxl__cpuid_legacy(libxl_ctx *ctx, uint32_t domid, bool restore,
-                         libxl_domain_build_info *info)
+int libxl__cpuid_legacy(libxl_ctx *ctx, uint32_t domid, bool restore,
+                        libxl_domain_build_info *info)
 {
+    return 0;
 }
 
 yajl_gen_status libxl_cpuid_policy_list_gen_json(yajl_gen hand,