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

[Alejandro Vallejo <alejandro.vallejo@xxxxxxxxx>]

On Thu, Apr 27, 2023 at 01:35:18PM +0100, Andrew Cooper wrote:
> 
> > +    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.

That override shouldn't be done, true. Removed on v2.

That said, looking again through the callers it appears some/many of them rely 
on
PERROR() to print the error code to stderr on failure. At the bottom of the
invocation there's xencall1(), which itself is defined in xencall.h to return 
-1 on
error and sets errno.

I think I'll also modify the error handlers that this patch touches to improve
their debug. i.e: ERROR() -> PERROR() and also print the domid that failed to be
found where it's not set. And patch 2 to define the new wrapper as returning 0 
or
-1 exclusively rather than forwarding whatever comes from below.

> 
> > 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...)

I think you meant xc_exchange_page() instead? I've also modified this one so
errno lands in the error message.

> > 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

With the code changed to assume -1/errno (so PERROR() can still be used) this
line will be correct and can be left as-is. Does this all sound ok?

Cheers,
Alejandro