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

[Konrad Rzeszutek Wilk <konrad.wilk@xxxxxxxxxx>]

On Wed, Mar 18, 2015 at 01:06:18PM +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.

The ERROR_INVAL are negative values. Inverting them makes them
positive - which is what the rest of xl does for error vales?
> 
> 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.

Oh, there is a lot of other code (xl <>) which return -ERROR_XYZ so that
the error values are positive.

How should 'xl' return errors? Just as 1 for failure or actually
use an -ERROR_XYZ so that folks can map them to -ERROR_XYZ?

> 
> > Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@xxxxxxxxxx>
> > ---
> >  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)
> 
> 

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel