Re: [PATCH v3 1/3] tools: Modify single-domid callers of xc_domain_getinfolist()

[Anthony PERARD <anthony.perard@xxxxxxxxxx>]

On Tue, May 02, 2023 at 12:13:36PM +0100, Alejandro Vallejo wrote:
> diff --git a/tools/libs/light/libxl_domain.c b/tools/libs/light/libxl_domain.c
> index 7f0986c185..5709b3e62f 100644
> --- a/tools/libs/light/libxl_domain.c
> +++ b/tools/libs/light/libxl_domain.c
> @@ -349,16 +349,12 @@ int libxl_domain_info(libxl_ctx *ctx, libxl_dominfo 
> *info_r,
>      int ret;
>      GC_INIT(ctx);
>  
> -    ret = xc_domain_getinfolist(ctx->xch, domid, 1, &xcinfo);
> +    ret = xc_domain_getinfo_single(ctx->xch, domid, &xcinfo);
>      if (ret<0) {
> -        LOGED(ERROR, domid, "Getting domain info list");
> +        LOGED(ERROR, domid, "Getting domain info");
>          GC_FREE;
>          return ERROR_FAIL;
>      }
> -    if (ret==0 || xcinfo.domain != domid) {
> -        GC_FREE;
> -        return ERROR_DOMAIN_NOTFOUND;

I kind of think we should keep returning ERROR_DOMAIN_NOTFOUND on error
as this is the most likely explanation. Also, the comment for this
function in libxl.h explain this:
    /* May be called with info_r == NULL to check for domain's existence.
     * Returns ERROR_DOMAIN_NOTFOUND if domain does not exist (used to return
     * ERROR_INVAL for this scenario). */
    int libxl_domain_info(libxl_ctx*, libxl_dominfo *info_r,
                          uint32_t domid);

Would it be possible to find out from xc_domain_getinfo_single() if it's
a domain not found scenario vs other error (like permission denied)?

> -    }
>  
>      if (info_r)
>          libxl__xcinfo2xlinfo(ctx, &xcinfo, info_r);
> @@ -1657,14 +1653,15 @@ int libxl__resolve_domid(libxl__gc *gc, const char 
> *name, uint32_t *domid)
>  libxl_vcpuinfo *libxl_list_vcpu(libxl_ctx *ctx, uint32_t domid,
>                                         int *nr_vcpus_out, int *nr_cpus_out)
>  {
> +    int rc;
>      GC_INIT(ctx);
>      libxl_vcpuinfo *ptr, *ret;
>      xc_domaininfo_t domaininfo;
>      xc_vcpuinfo_t vcpuinfo;
>      unsigned int nr_vcpus;
>  
> -    if (xc_domain_getinfolist(ctx->xch, domid, 1, &domaininfo) != 1) {
> -        LOGED(ERROR, domid, "Getting infolist");
> +    if ((rc = xc_domain_getinfo_single(ctx->xch, domid, &domaininfo)) < 0) {

The variable name "rc" is reserved for libxl return code. For syscall
and other external call, we should use the name "r". (I know that in
other part of this patch the name used is "ret", but as it is already
existing in the code base, I'm not asking for a change elsewhere)

Also, assignment to the variable should be done outside of the if(). So
the new code should look like:

    r = xc_domain_getinfolist(...);
    if (r < 0) {

(There's tools/libs/light/CODING_STYLE for all this explained)

> +        LOGED(ERROR, domid, "Getting dominfo");
>          GC_FREE;
>          return NULL;
>      }
> diff --git a/tools/libs/light/libxl_sched.c b/tools/libs/light/libxl_sched.c
> index 7c53dc60e6..21a65442c0 100644
> --- a/tools/libs/light/libxl_sched.c
> +++ b/tools/libs/light/libxl_sched.c
> @@ -219,13 +219,11 @@ static int sched_credit_domain_set(libxl__gc *gc, 
> uint32_t domid,
>      xc_domaininfo_t domaininfo;
>      int rc;
>  
> -    rc = xc_domain_getinfolist(CTX->xch, domid, 1, &domaininfo);
> +    rc = xc_domain_getinfo_single(CTX->xch, domid, &domaininfo);
>      if (rc < 0) {
> -        LOGED(ERROR, domid, "Getting domain info list");
> +        LOGED(ERROR, domid, "Getting domain info");
>          return ERROR_FAIL;
>      }
> -    if (rc != 1 || domaininfo.domain != domid)
> -        return ERROR_INVAL;

We can probably return ERROR_INVAL on error instead of ERROR_FAIL, as I
guess it's more likely that we try to change a non-existing domain
rather than having another error.

>  
>      rc = xc_sched_credit_domain_get(CTX->xch, domid, &sdom);
>      if (rc != 0) {
> @@ -426,13 +424,11 @@ static int sched_credit2_domain_set(libxl__gc *gc, 
> uint32_t domid,
>      xc_domaininfo_t info;
>      int rc;
>  
> -    rc = xc_domain_getinfolist(CTX->xch, domid, 1, &info);
> +    rc = xc_domain_getinfo_single(CTX->xch, domid, &info);
>      if (rc < 0) {
>          LOGED(ERROR, domid, "Getting domain info");
>          return ERROR_FAIL;

dito.

>      }
> -    if (rc != 1 || info.domain != domid)
> -        return ERROR_INVAL;
>  
>      rc = xc_sched_credit2_domain_get(CTX->xch, domid, &sdom);
>      if (rc != 0) {


Thanks,

-- 
Anthony PERARD