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
