Re: [PATCH v2 6/7] tools: Use new xc function for some xc_domain_getinfo() calls

[Andrew Cooper <andrew.cooper3@xxxxxxxxxx>]

On 28/04/2023 11:41 am, Alejandro Vallejo wrote:
> diff --git a/tools/libs/ctrl/xc_domain.c b/tools/libs/ctrl/xc_domain.c
> index 6b11775d4c..533e3c1314 100644
> --- a/tools/libs/ctrl/xc_domain.c
> +++ b/tools/libs/ctrl/xc_domain.c
> @@ -1959,15 +1959,14 @@ int xc_domain_memory_mapping(
>      uint32_t add_mapping)
>  {
>      DECLARE_DOMCTL;
> -    xc_dominfo_t info;
> +    xc_domaininfo_t info;
>      int ret = 0, rc;
>      unsigned long done = 0, nr, max_batch_sz;
>  
> -    if ( xc_domain_getinfo(xch, domid, 1, &info) != 1 ||
> -         info.domid != domid )
> +    if ( xc_domain_getinfo_single(xch, domid, &info) < 0 )
>      {
> -        PERROR("Could not get info for domain");
> -        return -EINVAL;
> +        PERROR("Could not get info for dom%u", domid);
> +        return -1;

I think this needs to be "return -errno" to have the same semantics as
before.

> diff --git a/tools/libs/ctrl/xc_private.c b/tools/libs/ctrl/xc_private.c
> index 2f99a7d2cf..8dcebad401 100644
> --- a/tools/libs/ctrl/xc_private.c
> +++ b/tools/libs/ctrl/xc_private.c
> @@ -441,11 +441,10 @@ int xc_machphys_mfn_list(xc_interface *xch,
>  
>  long xc_get_tot_pages(xc_interface *xch, uint32_t domid)
>  {
> -    xc_dominfo_t info;
> -    if ( (xc_domain_getinfo(xch, domid, 1, &info) != 1) ||
> -         (info.domid != domid) )
> +    xc_domaininfo_t info;
> +    if ( xc_domain_getinfo_single(xch, domid, &info) < 0 )
>          return -1;
> -    return info.nr_pages;
> +    return info.tot_pages;

As we're modifying every line in the function, take the opportunity to
add two extra blank lines to improve readability.

> diff --git a/tools/libs/guest/xg_cpuid_x86.c b/tools/libs/guest/xg_cpuid_x86.c
> index bd16a87e48..1519b5d556 100644
> --- a/tools/libs/guest/xg_cpuid_x86.c
> +++ b/tools/libs/guest/xg_cpuid_x86.c
> @@ -281,7 +281,8 @@ static int xc_cpuid_xend_policy(
>      xc_interface *xch, uint32_t domid, const struct xc_xend_cpuid *xend)
>  {
>      int rc;
> -    xc_dominfo_t di;
> +    bool hvm;
> +    xc_domaininfo_t di;
>      unsigned int nr_leaves, nr_msrs;
>      uint32_t err_leaf = -1, err_subleaf = -1, err_msr = -1;
>      /*
> @@ -291,13 +292,12 @@ static int xc_cpuid_xend_policy(
>      xen_cpuid_leaf_t *host = NULL, *def = NULL, *cur = NULL;
>      unsigned int nr_host, nr_def, nr_cur;
>  
> -    if ( xc_domain_getinfo(xch, domid, 1, &di) != 1 ||
> -         di.domid != domid )
> +    if ( (rc = xc_domain_getinfo_single(xch, domid, &di)) < 0 )
>      {
> -        ERROR("Failed to obtain d%d info", domid);
> -        rc = -ESRCH;
> +        PERROR("Failed to obtain d%d info", domid);
>          goto fail;

Sorry, I gave you bad advice last time around.  We need rc = -errno to
maintain behaviour here, and that is a pattern used out of context too.

Same in related hunks too.

> @@ -330,12 +330,12 @@ static int xc_cpuid_xend_policy(
>      /* Get the domain type's default policy. */
>      nr_msrs = 0;
>      nr_def = nr_leaves;
> -    rc = get_system_cpu_policy(xch, di.hvm ? 
> XEN_SYSCTL_cpu_policy_hvm_default
> +    rc = get_system_cpu_policy(xch, hvm ? XEN_SYSCTL_cpu_policy_hvm_default
>                                             : 
> XEN_SYSCTL_cpu_policy_pv_default,

We like to keep the ? and : vertically aligned if possible.

> diff --git a/tools/libs/guest/xg_dom_boot.c b/tools/libs/guest/xg_dom_boot.c
> index 263a3f4c85..1dea534bba 100644
> --- a/tools/libs/guest/xg_dom_boot.c
> +++ b/tools/libs/guest/xg_dom_boot.c
> @@ -174,19 +174,11 @@ int xc_dom_boot_image(struct xc_dom_image *dom)
>          return rc;
>  
>      /* collect some info */
> -    rc = xc_domain_getinfo(dom->xch, dom->guest_domid, 1, &info);
> -    if ( rc < 0 )
> -    {
> -        xc_dom_panic(dom->xch, XC_INTERNAL_ERROR,
> -                     "%s: getdomaininfo failed (rc=%d)", __FUNCTION__, rc);
> -        return rc;
> -    }
> -    if ( rc == 0 || info.domid != dom->guest_domid )
> +    if ( xc_domain_getinfo_single(dom->xch, dom->guest_domid, &info) < 0 )
>      {
>          xc_dom_panic(dom->xch, XC_INTERNAL_ERROR,
> -                     "%s: Huh? No domains found (nr_domains=%d) "
> -                     "or domid mismatch (%d != %d)", __FUNCTION__,
> -                     rc, info.domid, dom->guest_domid);
> +                     "%s: getdomaininfo failed (errno=%d)",
> +                     __FUNCTION__, rc, errno);

Ah yes, this is where your stray hunk from patch 7 wants to live.

> diff --git a/tools/libs/guest/xg_sr_restore.c 
> b/tools/libs/guest/xg_sr_restore.c
> index 7314a24cf9..6767c9f5cc 100644
> --- a/tools/libs/guest/xg_sr_restore.c
> +++ b/tools/libs/guest/xg_sr_restore.c
> @@ -887,20 +888,15 @@ int xc_domain_restore(xc_interface *xch, int io_fd, 
> uint32_t dom,
>          break;
>      }
>  
> -    if ( xc_domain_getinfo(xch, dom, 1, &ctx.dominfo) != 1 )
> +    if ( xc_domain_getinfo_single(xch, dom, &ctx.dominfo) < 0 )
>      {
> -        PERROR("Failed to get domain info");
> -        return -1;
> -    }
> -
> -    if ( ctx.dominfo.domid != dom )
> -    {
> -        ERROR("Domain %u does not exist", dom);
> +        PERROR("Failed to get info for dom%u", dom);

This is somewhat ambiguous, because "info" could be anything.  "dominfo"
would be better.

> diff --git a/tools/libs/guest/xg_sr_save.c b/tools/libs/guest/xg_sr_save.c
> index 9853d8d846..b0b30b4bc2 100644
> --- a/tools/libs/guest/xg_sr_save.c
> +++ b/tools/libs/guest/xg_sr_save.c
> @@ -336,19 +336,17 @@ static int suspend_domain(struct xc_sr_context *ctx)
>      }
>  
>      /* Refresh domain information. */
> -    if ( (xc_domain_getinfo(xch, ctx->domid, 1, &ctx->dominfo) != 1) ||
> -         (ctx->dominfo.domid != ctx->domid) )
> +    if ( xc_domain_getinfo_single(xch, ctx->domid, &ctx->dominfo) < 0 )
>      {
>          PERROR("Unable to refresh domain information");
>          return -1;
>      }
>  
>      /* Confirm the domain has actually been paused. */
> -    if ( !ctx->dominfo.shutdown ||
> -         (ctx->dominfo.shutdown_reason != SHUTDOWN_suspend) )
> +    if ( !dominfo_shutdown_with(&ctx->dominfo, SHUTDOWN_suspend) )
>      {
>          ERROR("Domain has not been suspended: shutdown %d, reason %d",
> -              ctx->dominfo.shutdown, ctx->dominfo.shutdown_reason);
> +              ctx->dominfo.flags & XEN_DOMINF_shutdown, 
> dominfo_shutdown_reason(&ctx->dominfo));

This likely wants wrapping onto the next line.

> diff --git a/tools/libs/light/libxl_dom.c b/tools/libs/light/libxl_dom.c
> index bd5d823581..94fef37401 100644
> --- a/tools/libs/light/libxl_dom.c
> +++ b/tools/libs/light/libxl_dom.c
> @@ -34,7 +34,7 @@ libxl_domain_type libxl__domain_type(libxl__gc *gc, 
> uint32_t domid)
>  
>      ret = xc_domain_getinfo_single(ctx->xch, domid, &info);
>      if (ret < 0) {
> -        LOG(ERROR, "unable to get domain type for domid=%"PRIu32, domid);
> +        LOGED(ERROR, domid, "unable to get dominfo");
>          return LIBXL_DOMAIN_TYPE_INVALID;
>      }

Ah, this is the answer to my review on patch 5.

Quite a few of the following hunks look like want to be in patch 5 too.

Everything else looks good, best as I can tell.

~Andrew