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

[Andrew Cooper <andrew.cooper3@xxxxxxxxxx>]

On 26/04/2023 3:59 pm, Alejandro Vallejo wrote:
> diff --git a/tools/libs/guest/xg_cpuid_x86.c b/tools/libs/guest/xg_cpuid_x86.c
> index bd16a87e48..6d260d2cff 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 is_hvm;

I know it makes a slightly larger diff, but simply "bool hvm" is what we
use more commonly elsewhere.

> +    xc_domaininfo_t di;
>      unsigned int nr_leaves, nr_msrs;
>      uint32_t err_leaf = -1, err_subleaf = -1, err_msr = -1;
>      /*
> @@ -291,13 +292,13 @@ 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 ( xc_domain_getinfo_single(xch, domid, &di) < 0 )
>      {
>          ERROR("Failed to obtain d%d info", domid);
>          rc = -ESRCH;

Now that xc_domain_getinfo_single() has a sane return value, you want to
set it in the if(), and not override to ESRCH here.

These two comments repeat several other times.

> diff --git a/tools/libs/guest/xg_dom_boot.c b/tools/libs/guest/xg_dom_boot.c
> index 263a3f4c85..59b4d641c9 100644
> --- a/tools/libs/guest/xg_dom_boot.c
> +++ b/tools/libs/guest/xg_dom_boot.c
> @@ -164,7 +164,7 @@ void *xc_dom_boot_domU_map(struct xc_dom_image *dom, 
> xen_pfn_t pfn,
>  
>  int xc_dom_boot_image(struct xc_dom_image *dom)
>  {
> -    xc_dominfo_t info;
> +    xc_domaininfo_t info;
>      int rc;
>  
>      DOMPRINTF_CALLED(dom->xch);
> @@ -174,21 +174,13 @@ 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);
> +    rc = xc_domain_getinfo_single(dom->xch, dom->guest_domid, &info);
>      if ( rc < 0 )
>      {
>          xc_dom_panic(dom->xch, XC_INTERNAL_ERROR,
>                       "%s: getdomaininfo failed (rc=%d)", __FUNCTION__, rc);
>          return rc;

This need to change to -1, or you've broken the error convention of this
function.  (Yes, libxc is a giant mess of error handling...)

> diff --git a/tools/libs/guest/xg_resume.c b/tools/libs/guest/xg_resume.c
> index 77e2451a3c..60d682c746 100644
> --- a/tools/libs/guest/xg_resume.c
> +++ b/tools/libs/guest/xg_resume.c
> @@ -26,28 +26,27 @@
>  static int modify_returncode(xc_interface *xch, uint32_t domid)
>  {
>      vcpu_guest_context_any_t ctxt;
> -    xc_dominfo_t info;
> +    xc_domaininfo_t info;
>      xen_capabilities_info_t caps;
>      struct domain_info_context _dinfo = {};
>      struct domain_info_context *dinfo = &_dinfo;
>      int rc;
>  
> -    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 domain info");
>          return -1;
>      }
>  
> -    if ( !info.shutdown || (info.shutdown_reason != SHUTDOWN_suspend) )
> +    if ( !dominfo_shutdown_with(&info, SHUTDOWN_suspend))

Needs a space at the end.

> diff --git a/tools/libs/guest/xg_sr_restore.c 
> b/tools/libs/guest/xg_sr_restore.c
> index 7314a24cf9..a03183f4b9 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);
> -        return -1;
> -    }
> -
> +    is_hvm = !!(ctx.dominfo.flags & XEN_DOMINF_hvm_guest);

No need for !! now you switched this to bool.

> diff --git a/tools/libs/guest/xg_sr_save.c b/tools/libs/guest/xg_sr_save.c
> index 9853d8d846..8fc8e9d3b2 100644
> --- a/tools/libs/guest/xg_sr_save.c
> +++ b/tools/libs/guest/xg_sr_save.c
> @@ -996,17 +994,13 @@ int xc_domain_save(xc_interface *xch, int io_fd, 
> uint32_t dom,
>      ctx.save.debug = !!(flags & XCFLAGS_DEBUG);
>      ctx.save.recv_fd = recv_fd;
>  
> -    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);
> -        return -1;
> -    }
> +    is_hvm = !!(ctx.dominfo.flags & XEN_DOMINF_hvm_guest);

Same here.  Can drop the !!.

> diff --git a/tools/misc/xen-lowmemd.c b/tools/misc/xen-lowmemd.c
> index a3a2741242..b483f63fdc 100644
> --- a/tools/misc/xen-lowmemd.c
> +++ b/tools/misc/xen-lowmemd.c
> @@ -38,7 +38,7 @@ void cleanup(void)
>  #define BUFSZ 512
>  void handle_low_mem(void)
>  {
> -    xc_dominfo_t  dom0_info;
> +    xc_domaininfo_t  dom0_info;

Use this opportunity to remove the double space.

> diff --git a/tools/vchan/vchan-socket-proxy.c 
> b/tools/vchan/vchan-socket-proxy.c
> index e1d959c6d1..9c4c336b03 100644
> --- a/tools/vchan/vchan-socket-proxy.c
> +++ b/tools/vchan/vchan-socket-proxy.c
> @@ -254,12 +254,12 @@ static struct libxenvchan *connect_vchan(int domid, 
> const char *path) {
>          if (ctrl)
>              break;
>  
> -        ret = xc_domain_getinfo(xc, domid, 1, &dominfo);
> +        ret = xc_domain_getinfo_single(xc, domid, &dominfo);
>          /* break the loop if domain is definitely not there anymore, but
>           * continue if it is or the call failed (like EPERM) */
>          if (ret == -1 && errno == ESRCH)

Oh wow... so this bit of vchan was written expecting sane semantics out
of xc_domain_getinfo() in the first place...

This needs adjusting too because of the -1/errno -> -errno change.

~Andrew