Re: [PATCH v2 2/5] libxl: vcpuset: Return error values.

[Ian Campbell <ian.campbell@citrix.com>]

On Wed, 2015-03-18 at 13:06 +0000, Ian Campbell wrote:
> On Fri, 2015-03-13 at 16:26 -0400, Konrad Rzeszutek Wilk wrote:
> > The function does not return any values at all. Convert the
> > internal libxl ones (ERROR_FAIL, ..., etc) to positive values
> > and for the other cases just return standard libxl values.
> 
> It's not clear why you want to do this, in particular returning
> -ERROR_INVAL and inverting libxl error codes seems like a very strange
> thing to be doing.

BTW I know the xl error handling is horribly confused, and there are
even a small number of instances of -ERROR_* already, but I think those
are wrong and we shouldn't introduce more.

> 
> I think you should either use ERROR_INVAL (not inverted) and propagate
> libxl rc's directly or convert them into something which suits xl, i.e.
> 0 and 1.
> 
> > Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
> > ---
> >  tools/libxl/xl_cmdimpl.c | 23 +++++++++++++----------
> >  1 file changed, 13 insertions(+), 10 deletions(-)
> > 
> > diff --git a/tools/libxl/xl_cmdimpl.c b/tools/libxl/xl_cmdimpl.c
> > index 2d7145f..454a895 100644
> > --- a/tools/libxl/xl_cmdimpl.c
> > +++ b/tools/libxl/xl_cmdimpl.c
> > @@ -5013,17 +5013,18 @@ int main_vcpupin(int argc, char **argv)
> >      return rc;
> >  }
> >  
> > -static void vcpuset(uint32_t domid, const char* nr_vcpus, int check_host)
> > +static int vcpuset(uint32_t domid, const char* nr_vcpus, int check_host)
> >  {
> >      char *endptr;
> >      unsigned int max_vcpus, i;
> >      libxl_bitmap cpumap;
> > +    int rc;
> >  
> >      libxl_bitmap_init(&cpumap);
> >      max_vcpus = strtoul(nr_vcpus, &endptr, 10);
> >      if (nr_vcpus == endptr) {
> >          fprintf(stderr, "Error: Invalid argument.\n");
> > -        return;
> > +        return -ERROR_INVAL;
> >      }
> >  
> >      /*
> > @@ -5036,22 +5037,25 @@ static void vcpuset(uint32_t domid, const char* nr_vcpus, int check_host)
> >              fprintf(stderr, "You are overcommmitting! You have %d physical " \
> >                      " CPUs and want %d vCPUs! Aborting, use --ignore-host to " \
> >                      " continue\n", host_cpu, max_vcpus);
> > -            return;
> > +            return -ERROR_INVAL;
> >          }
> >          /* NB: This also limits how many are set in the bitmap */
> >          max_vcpus = (max_vcpus > host_cpu ? host_cpu : max_vcpus);
> >      }
> > -    if (libxl_cpu_bitmap_alloc(ctx, &cpumap, max_vcpus)) {
> > -        fprintf(stderr, "libxl_cpu_bitmap_alloc failed\n");
> > -        return;
> > +    rc = libxl_cpu_bitmap_alloc(ctx, &cpumap, max_vcpus);
> > +    if (rc) {
> > +        fprintf(stderr, "libxl_cpu_bitmap_alloc failed, rc: %d\n", rc);
> > +        return -rc;
> >      }
> >      for (i = 0; i < max_vcpus; i++)
> >          libxl_bitmap_set(&cpumap, i);
> >  
> > -    if (libxl_set_vcpuonline(ctx, domid, &cpumap) < 0)
> > -        fprintf(stderr, "libxl_set_vcpuonline failed domid=%d max_vcpus=%d\n", domid, max_vcpus);
> > +    rc = libxl_set_vcpuonline(ctx, domid, &cpumap);
> > +    if (rc)
> > +        fprintf(stderr, "libxl_set_vcpuonline failed domid=%d max_vcpus=%d, rc: %d\n", domid, max_vcpus, rc);
> >  
> >      libxl_bitmap_dispose(&cpumap);
> > +    return rc ? -rc : 0;
> >  }
> >  
> >  int main_vcpuset(int argc, char **argv)
> > @@ -5070,8 +5074,7 @@ int main_vcpuset(int argc, char **argv)
> >          break;
> >      }
> >  
> > -    vcpuset(find_domain(argv[optind]), argv[optind + 1], check_host);
> > -    return 0;
> > +    return vcpuset(find_domain(argv[optind]), argv[optind + 1], check_host);
> >  }
> >  
> >  static void output_xeninfo(void)
>