Ian vs Ian, round 0 Was:Re: [PATCH v3 7/7] libxl/vcpu-set - allow to decrease vcpu count on overcommitted guests (v3)

[Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>]

On Tue, Mar 24, 2015 at 03:41:46PM +0000, Ian Campbell wrote:
> On Mon, 2015-03-23 at 14:21 -0400, Konrad Rzeszutek Wilk wrote:
> > We have a check to warn the user if they are overcommitting.
> > But the check only checks the hosts CPU amount and does
> > not take into account the case when the user is trying to fix
> > the overcommit. That is - they want to limit the amount of
> > online VCPUs.
> > 
> > This fix allows the user to offline vCPUs without any
> > warnings when they are running an overcommitted guest.
> > 
> > Also fix the extra space in the message.
> > 
> > Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
> > 
> > ---
> > [v2: Remove crud code as spotted by Boris]
> > [v3: Moved crud code removal to another patch]
> > ---
> >  tools/libxl/xl_cmdimpl.c | 15 ++++++++++++---
> >  1 file changed, 12 insertions(+), 3 deletions(-)
> > 
> > diff --git a/tools/libxl/xl_cmdimpl.c b/tools/libxl/xl_cmdimpl.c
> > index b121d75..d7bd5d5 100644
> > --- a/tools/libxl/xl_cmdimpl.c
> > +++ b/tools/libxl/xl_cmdimpl.c
> > @@ -5238,9 +5238,18 @@ static int vcpuset(uint32_t domid, const char* nr_vcpus, int check_host)
> >       */
> >      if (check_host) {
> >          unsigned int host_cpu = libxl_get_max_cpus(ctx);
> > -        if (max_vcpus > host_cpu) {
> > -            fprintf(stderr, "You are overcommmitting! You have %d physical " \
> > -                    " CPUs and want %d vCPUs! Aborting, use --ignore-host to " \
> > +        libxl_dominfo dominfo;
> > +
> > +        rc = libxl_domain_info(ctx, &dominfo, domid);
> > +        if (rc)
> > +        {
> > +             if (rc == ERROR_DOMAIN_NOTFOUND)
> > +                fprintf(stderr, "Domain %u does not exist.\n", domid);
> > +            return 1;
> 
> Indentation is wrong on the if.

Right!
> 
> More generally all of these rc == ERROR_DOMAIN_NOTFOUND check and prints
> are going to get rather tiresome, especially if/when there are other
> interesting error codes. Perhaps we could arrange for something further
> down the stack on libxl to log this sort of thing, such that xl can rely
> on it already having been mentioned?
> 
> e.g. libxl_domain_info could do it perhaps?

That would be the easiest way. Should I drop

libxl: Add ERROR_DOMAIN_NOTFOUND for libxl_domain_info when it cannot find the domain

and just add an LIBXL__LOG_ERRNO and keep on returning the old error value
(ERROR_INVAL)?

Ian J recommended to add this new ERROR_DOMAIN_NOTFOUND in libxl_domain_info
so I think I will let you two fight it out! (changing the title to catch
Ian J's attention)

> 
> Or perhaps a helper (in xl?) which produces some common logging for
> common errors which could be called?

Nah, I am all for simplicity and just need these errors to be printed out.

We could also just leave the 
libxl: Add ERROR_DOMAIN_NOTFOUND for libxl_domain_info when it cannot find the domain

and I can add another patch that just adds an nice LIBXL__LOG_ERRNO in
the libxl_domain_info?

>