[PATCH v1] Misc fixes to libxl (v1)

[PATCH v1] Misc fixes to libxl (v1)

[Konrad Rzeszutek Wilk]

Hey,

These two patches came about as I was debugging other things and I figured
they would be useful.

Please take a look at your convience.


 tools/libxl/libxl_pci.c  |  4 ++--
 tools/libxl/xl_cmdimpl.c | 20 +++++++++++++-------
 2 files changed, 15 insertions(+), 9 deletions(-)

Konrad Rzeszutek Wilk (2):
      libxl: give pciback a chance to do its teardown before we reset the device.
      libxl: vcpu-set - allow to decrease vcpu count on overcommitted guests (v2)

[PATCH v1 1/2] libxl: give pciback a chance to do its teardown before we reset the device.

[Konrad Rzeszutek Wilk]

We try to reset the device before we signal the pciback that
the device is no longer to be used by the guest. As such
we should give the pciback (or QEMU) a chance first to
some cleanup before we deploy the sledghammer approach
of resetting the device.

Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
---
 tools/libxl/libxl_pci.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/tools/libxl/libxl_pci.c b/tools/libxl/libxl_pci.c
index 44d0453..5a7cc9e 100644
--- a/tools/libxl/libxl_pci.c
+++ b/tools/libxl/libxl_pci.c
@@ -1260,6 +1260,8 @@ skip1:
         abort();
     }
 out:
+    libxl__device_pci_remove_xenstore(gc, domid, pcidev);
+
     /* don't do multiple resets while some functions are still passed through */
     if ( (pcidev->vdevfn & 0x7) == 0 ) {
         libxl__device_pci_reset(gc, pcidev->domain, pcidev->bus, pcidev->dev, pcidev->func);
@@ -1277,8 +1279,6 @@ out:
         libxl__device_pci_remove_common(gc, stubdomid, &pcidev_s, force);
     }
 
-    libxl__device_pci_remove_xenstore(gc, domid, pcidev);
-
     rc = 0;
 out_fail:
     free(assigned);
-- 
1.9.3

[PATCH v1 2/2] libxl: vcpu-set - allow to decrease vcpu count on overcommitted guests (v2)

[Konrad Rzeszutek Wilk]

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 while at it, remove crud code.

Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
[v2: Remove crud code as spotted by Boris]
---
 tools/libxl/xl_cmdimpl.c | 20 +++++++++++++-------
 1 file changed, 13 insertions(+), 7 deletions(-)

diff --git a/tools/libxl/xl_cmdimpl.c b/tools/libxl/xl_cmdimpl.c
index 5195914..5b27bd8 100644
--- a/tools/libxl/xl_cmdimpl.c
+++ b/tools/libxl/xl_cmdimpl.c
@@ -4754,15 +4754,21 @@ static void vcpuset(uint32_t domid, const char* nr_vcpus, int check_host)
      * by the host's amount of pCPUs.
      */
     if (check_host) {
+        libxl_dominfo dominfo;
+
         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 " \
-                    " continue\n", host_cpu, max_vcpus);
-            return;
+
+        if (libxl_domain_info(ctx, &dominfo, domid) != 0)
+            dominfo.vcpu_online = host_cpu;
+
+        if (max_vcpus > dominfo.vcpu_online) {
+            if ((max_vcpus > host_cpu)) {
+                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;
+            }
         }
-        /* 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");
-- 
1.9.3

Re: [PATCH v1 1/2] libxl: give pciback a chance to do its teardown before we reset the device.

[Ian Campbell]

On Wed, 2014-06-04 at 09:33 -0400, Konrad Rzeszutek Wilk wrote:
> We try to reset the device before we signal the pciback that
> the device is no longer to be used by the guest. As such
> we should give the pciback (or QEMU) a chance first to
> some cleanup before we deploy the sledghammer approach
> of resetting the device.

What does "a chance" correspond to here? Do we wait for some defined
time for the state node to hit closed or are we now relying in winning a
race more often than before?

I think we would obviously prefer the former, meaning there should be a
call to libxl__wait_for_backend("6") in here somewhere I think.

Ian.

Re: [PATCH v1 2/2] libxl: vcpu-set - allow to decrease vcpu count on overcommitted guests (v2)

[Ian Campbell]

On Wed, 2014-06-04 at 09:33 -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 while at it, remove crud code.
> 
> Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>

Contrary to $SUBJECT this is an xl patch not a libxl one. Also there is
a spurious "(v2)" in the subject.

> [v2: Remove crud code as spotted by Boris]
> ---
>  tools/libxl/xl_cmdimpl.c | 20 +++++++++++++-------
>  1 file changed, 13 insertions(+), 7 deletions(-)
> 
> diff --git a/tools/libxl/xl_cmdimpl.c b/tools/libxl/xl_cmdimpl.c
> index 5195914..5b27bd8 100644
> --- a/tools/libxl/xl_cmdimpl.c
> +++ b/tools/libxl/xl_cmdimpl.c
> @@ -4754,15 +4754,21 @@ static void vcpuset(uint32_t domid, const char* nr_vcpus, int check_host)
>       * by the host's amount of pCPUs.
>       */
>      if (check_host) {
> +        libxl_dominfo dominfo;
> +
>          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 " \
> -                    " continue\n", host_cpu, max_vcpus);
> -            return;
> +
> +        if (libxl_domain_info(ctx, &dominfo, domid) != 0)
> +            dominfo.vcpu_online = host_cpu;
> +
> +        if (max_vcpus > dominfo.vcpu_online) {
> +            if ((max_vcpus > host_cpu)) {

I think this is 
        if (max_vcpus > dominfo.vcpu_online && max_vcpus > host_cpu) {

and if not then the second one has a spurious set of ()s.

> +                fprintf(stderr, "You are overcommmitting! You have %d physical" \

You've carried over the typo here (unless you intended to overcommit on
the number of m's ;-)). Might as well fix while you are here..

> +                        " CPUs and want %d vCPUs! Aborting, use --ignore-host to" \
> +                        " continue\n", host_cpu, max_vcpus);
> +                return;
> +            }
>          }
> -        /* NB: This also limits how many are set in the bitmap */
> -        max_vcpus = (max_vcpus > host_cpu ? host_cpu : max_vcpus);

Where did this go?

>      }
>      if (libxl_cpu_bitmap_alloc(ctx, &cpumap, max_vcpus)) {
>          fprintf(stderr, "libxl_cpu_bitmap_alloc failed\n");

Re: [PATCH v1 1/2] libxl: give pciback a chance to do its teardown before we reset the device.

[Konrad Rzeszutek Wilk]

On Thu, Jun 05, 2014 at 11:58:42AM +0100, Ian Campbell wrote:
> On Wed, 2014-06-04 at 09:33 -0400, Konrad Rzeszutek Wilk wrote:
> > We try to reset the device before we signal the pciback that
> > the device is no longer to be used by the guest. As such
> > we should give the pciback (or QEMU) a chance first to
> > some cleanup before we deploy the sledghammer approach
> > of resetting the device.
> 
> What does "a chance" correspond to here? Do we wait for some defined
> time for the state node to hit closed or are we now relying in winning a
> race more often than before?
> 
> I think we would obviously prefer the former, meaning there should be a
> call to libxl__wait_for_backend("6") in here somewhere I think.

Yeah, that would be smarter.
> 
> Ian.
> 

Re: [PATCH v1 2/2] libxl: vcpu-set - allow to decrease vcpu count on overcommitted guests (v2)

[Konrad Rzeszutek Wilk]

On Thu, Jun 05, 2014 at 12:02:57PM +0100, Ian Campbell wrote:
> On Wed, 2014-06-04 at 09:33 -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 while at it, remove crud code.
> > 
> > Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
> 
> Contrary to $SUBJECT this is an xl patch not a libxl one. Also there is
> a spurious "(v2)" in the subject.
> 
> > [v2: Remove crud code as spotted by Boris]
> > ---
> >  tools/libxl/xl_cmdimpl.c | 20 +++++++++++++-------
> >  1 file changed, 13 insertions(+), 7 deletions(-)
> > 
> > diff --git a/tools/libxl/xl_cmdimpl.c b/tools/libxl/xl_cmdimpl.c
> > index 5195914..5b27bd8 100644
> > --- a/tools/libxl/xl_cmdimpl.c
> > +++ b/tools/libxl/xl_cmdimpl.c
> > @@ -4754,15 +4754,21 @@ static void vcpuset(uint32_t domid, const char* nr_vcpus, int check_host)
> >       * by the host's amount of pCPUs.
> >       */
> >      if (check_host) {
> > +        libxl_dominfo dominfo;
> > +
> >          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 " \
> > -                    " continue\n", host_cpu, max_vcpus);
> > -            return;
> > +
> > +        if (libxl_domain_info(ctx, &dominfo, domid) != 0)
> > +            dominfo.vcpu_online = host_cpu;
> > +
> > +        if (max_vcpus > dominfo.vcpu_online) {
> > +            if ((max_vcpus > host_cpu)) {
> 
> I think this is 
>         if (max_vcpus > dominfo.vcpu_online && max_vcpus > host_cpu) {
> 
> and if not then the second one has a spurious set of ()s.
> 
> > +                fprintf(stderr, "You are overcommmitting! You have %d physical" \
> 
> You've carried over the typo here (unless you intended to overcommit on
> the number of m's ;-)). Might as well fix while you are here..

Mmmmm.. You are riggggghhhhhhhhhhttttttttt.
> 
> > +                        " CPUs and want %d vCPUs! Aborting, use --ignore-host to" \
> > +                        " continue\n", host_cpu, max_vcpus);
> > +                return;
> > +            }
> >          }
> > -        /* NB: This also limits how many are set in the bitmap */
> > -        max_vcpus = (max_vcpus > host_cpu ? host_cpu : max_vcpus);
> 
> Where did this go?

No need for it actually. As we already do the action if 'max_vcpus >
host_cpu' - which is that we return. So in essence that code will set max_vcpus
to max_vcpus.

> 
> >      }
> >      if (libxl_cpu_bitmap_alloc(ctx, &cpumap, max_vcpus)) {
> >          fprintf(stderr, "libxl_cpu_bitmap_alloc failed\n");
> 
> 

Re: [PATCH v1 1/2] libxl: give pciback a chance to do its teardown before we reset the device.

[Ian Jackson]

Konrad Rzeszutek Wilk writes ("Re: [PATCH v1 1/2] libxl: give pciback a chance to do its teardown before we reset the device."):
> On Thu, Jun 05, 2014 at 11:58:42AM +0100, Ian Campbell wrote:
> > I think we would obviously prefer the former, meaning there should be a
> > call to libxl__wait_for_backend("6") in here somewhere I think.
> 
> Yeah, that would be smarter.

No new callers of libxl__wait_for_backend should be introduced.  It's
a synchronous sleeper.

Ian.

Re: [PATCH v1 2/2] libxl: vcpu-set - allow to decrease vcpu count on overcommitted guests (v2)

[Ian Campbell]

On Thu, 2014-06-05 at 13:44 -0400, Konrad Rzeszutek Wilk wrote:
> > > -        /* NB: This also limits how many are set in the bitmap */
> > > -        max_vcpus = (max_vcpus > host_cpu ? host_cpu : max_vcpus);
> > 
> > Where did this go?
> 
> No need for it actually. As we already do the action if 'max_vcpus >
> host_cpu' - which is that we return. So in essence that code will set max_vcpus
> to max_vcpus.

What about if dominfo.vcpu_online > max_vcpus? iN that case the
max_vcpus > host_cpu check doesn't occur.

You could be in this state if someone had previously forced overcommit I
think.

> 
> > 
> > >      }
> > >      if (libxl_cpu_bitmap_alloc(ctx, &cpumap, max_vcpus)) {
> > >          fprintf(stderr, "libxl_cpu_bitmap_alloc failed\n");
> > 
> > 

Re: [PATCH v1 1/2] libxl: give pciback a chance to do its teardown before we reset the device.

[Ian Campbell]

On Thu, 2014-06-05 at 18:56 +0100, Ian Jackson wrote:
> Konrad Rzeszutek Wilk writes ("Re: [PATCH v1 1/2] libxl: give pciback a chance to do its teardown before we reset the device."):
> > On Thu, Jun 05, 2014 at 11:58:42AM +0100, Ian Campbell wrote:
> > > I think we would obviously prefer the former, meaning there should be a
> > > call to libxl__wait_for_backend("6") in here somewhere I think.
> > 
> > Yeah, that would be smarter.
> 
> No new callers of libxl__wait_for_backend should be introduced.  It's
> a synchronous sleeper.

What's the proper mechanism then?

Re: [PATCH v1 2/2] libxl: vcpu-set - allow to decrease vcpu count on overcommitted guests (v2)

[Konrad Rzeszutek Wilk]

On Fri, Jun 06, 2014 at 10:07:37AM +0100, Ian Campbell wrote:
> On Thu, 2014-06-05 at 13:44 -0400, Konrad Rzeszutek Wilk wrote:
> > > > -        /* NB: This also limits how many are set in the bitmap */
> > > > -        max_vcpus = (max_vcpus > host_cpu ? host_cpu : max_vcpus);
> > > 
> > > Where did this go?
> > 
> > No need for it actually. As we already do the action if 'max_vcpus >
> > host_cpu' - which is that we return. So in essence that code will set max_vcpus
> > to max_vcpus.
> 
> What about if dominfo.vcpu_online > max_vcpus? iN that case the
> max_vcpus > host_cpu check doesn't occur.

Let me split that change out to a different patch. But in case you
do remember this conversation - that is the purpose of this patch - to
bypass the check.
> 
> You could be in this state if someone had previously forced overcommit I
> think.

Right, or the guest was constructed with values greater than pCPU.

Since I am sure you don't remember the context of this patch, I am
resending them here (they grew to four patches)

Let me rehash what we had in set in stone way back in 4.4:
 - The guest config ('maxvcpus') is permitted to be greater than the pCPUs.
   Ditto for the initially allocated ('vcpus') amounts. It is also
   OK to be different - 'vcpus' < 'maxvcpus', etc.

 - If the 'vcpus' < pCPUs and we want to increase it above pCPUs we should
   error out and print out a warning telling them to use --ignore-host.
   Regardless of the dominfo.max_vcpu_id - so if the max_vpcu_id is
   greater than pCPU and 'vcpu' < pCPU, we should still warn the user
   when increasing.

 - If the 'vcpus' > pCPUs and we want to decrease to be below pCPUs then
   we should do that without the warning.
   (this is what the patch was fixing).

[PATCH 1/4] libxl: vcpuset: Return error values.

[Konrad Rzeszutek Wilk]

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.

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 b7eac29..378ede1 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)
-- 
1.8.4.2

[PATCH 2/4] libxl: vcpuset: Check max_vcpus argument against the maximum number of vCPUs the guest has set.

[Konrad Rzeszutek Wilk]

The maximum number of VCPUs the guest can have is determined during
domain creation and is set by 'maxvcpus' parameter (in the guest
config). Trying to set the amount of vCPUs above said value
in vcpuset will result in an error - and we can catch it here
(instead of later in the function) and print a nice warning to the user.

Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
---
 tools/libxl/xl_cmdimpl.c | 16 +++++++++++++++-
 1 file changed, 15 insertions(+), 1 deletion(-)

diff --git a/tools/libxl/xl_cmdimpl.c b/tools/libxl/xl_cmdimpl.c
index 378ede1..d22ba85 100644
--- a/tools/libxl/xl_cmdimpl.c
+++ b/tools/libxl/xl_cmdimpl.c
@@ -5018,6 +5018,7 @@ static int vcpuset(uint32_t domid, const char* nr_vcpus, int check_host)
     char *endptr;
     unsigned int max_vcpus, i;
     libxl_bitmap cpumap;
+    libxl_dominfo dominfo;
     int rc;
 
     libxl_bitmap_init(&cpumap);
@@ -5026,7 +5027,20 @@ static int vcpuset(uint32_t domid, const char* nr_vcpus, int check_host)
         fprintf(stderr, "Error: Invalid argument.\n");
         return -ERROR_INVAL;
     }
-
+    rc = libxl_domain_info(ctx, &dominfo, domid);
+    if (rc == ERROR_INVAL) {
+        fprintf(stderr, "Error: Domain %u does not exist.\n", domid);
+        return -rc;
+    }
+    if (rc) {
+        fprintf(stderr, "libxl_domain_info failed (code %d).\n", rc);
+        return -rc;
+    }
+    if (max_vcpus > dominfo.vcpu_max_id + 1) {
+        fprintf(stderr, "You have a max of %d vCPUs and you want %d vCPUs!\n",
+                dominfo.vcpu_max_id + 1, max_vcpus);
+        return -ERROR_INVAL;
+    }
     /*
      * Maximum amount of vCPUS the guest is allowed to set is limited
      * by the host's amount of pCPUs.
-- 
1.8.4.2

[PATCH 3/4] libxl: vcpuset: Remove useless limit on max_vcpus.

[Konrad Rzeszutek Wilk]

The check is superflous. If the 'max_vcpus' (argument
value) is greater than  pCPU and --ignore-host has not
been supplied we would print an warning and return
and not call this code.

If the --ignore-host parameter had been used we would
never end up in this condition and enforce 'max_vcpus'.

The only time it would be invoked is if max_vcpus < host_cpu
in which case it would set max_vcpus to max_vcpus.

In short - it is dead code.

Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
---
 tools/libxl/xl_cmdimpl.c | 2 --
 1 file changed, 2 deletions(-)

diff --git a/tools/libxl/xl_cmdimpl.c b/tools/libxl/xl_cmdimpl.c
index d22ba85..9e2d1b2 100644
--- a/tools/libxl/xl_cmdimpl.c
+++ b/tools/libxl/xl_cmdimpl.c
@@ -5053,8 +5053,6 @@ static int vcpuset(uint32_t domid, const char* nr_vcpus, int check_host)
                     " continue\n", host_cpu, max_vcpus);
             return -ERROR_INVAL;
         }
-        /* NB: This also limits how many are set in the bitmap */
-        max_vcpus = (max_vcpus > host_cpu ? host_cpu : max_vcpus);
     }
     rc = libxl_cpu_bitmap_alloc(ctx, &cpumap, max_vcpus);
     if (rc) {
-- 
1.8.4.2

[PATCH 4/4] libxl: vcpu-set - allow to decrease vcpu count on overcommitted guests (v3)

[Konrad Rzeszutek Wilk]

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 | 7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)

diff --git a/tools/libxl/xl_cmdimpl.c b/tools/libxl/xl_cmdimpl.c
index 9e2d1b2..3d89e3e 100644
--- a/tools/libxl/xl_cmdimpl.c
+++ b/tools/libxl/xl_cmdimpl.c
@@ -5047,9 +5047,10 @@ 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 " \
+
+        if (max_vcpus > dominfo.vcpu_online && max_vcpus > host_cpu) {
+            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 -ERROR_INVAL;
         }
-- 
1.8.4.2

Re: [PATCH 1/4] libxl: vcpuset: Return error values.

[Ian Jackson]

Konrad Rzeszutek Wilk writes ("[PATCH 1/4] libxl: vcpuset: Return error values."):
> 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.

This makes no sense to me, I'm afraid.  What is the return value type
of this function ?  Normally functions in libxl and many in xl return
a libxl error code.

I can't see any reason for ever writing -ERROR_FOO.

Ian.

Re: [PATCH 2/4] libxl: vcpuset: Check max_vcpus argument against the maximum number of vCPUs the guest has set.

[Ian Jackson]

Konrad Rzeszutek Wilk writes ("[PATCH 2/4] libxl: vcpuset: Check max_vcpus argument against the maximum number of vCPUs the guest has set."):
> The maximum number of VCPUs the guest can have is determined during
> domain creation and is set by 'maxvcpus' parameter (in the guest
> config). Trying to set the amount of vCPUs above said value
> in vcpuset will result in an error - and we can catch it here
> (instead of later in the function) and print a nice warning to the user.
...
> +    rc = libxl_domain_info(ctx, &dominfo, domid);
> +    if (rc == ERROR_INVAL) {
> +        fprintf(stderr, "Error: Domain %u does not exist.\n", domid);
> +        return -rc;

Do we really return ERROR_INVAL for this ?  ...   Looks like we do.

OK then, although we are definitely going to have to change that at
some point.  How tiresome.

Thanks,
Ian.

Re: [PATCH 2/4] libxl: vcpuset: Check max_vcpus argument against the maximum number of vCPUs the guest has set.

[Konrad Rzeszutek Wilk]

On Tue, Feb 03, 2015 at 03:11:11PM +0000, Ian Jackson wrote:
> Konrad Rzeszutek Wilk writes ("[PATCH 2/4] libxl: vcpuset: Check max_vcpus argument against the maximum number of vCPUs the guest has set."):
> > The maximum number of VCPUs the guest can have is determined during
> > domain creation and is set by 'maxvcpus' parameter (in the guest
> > config). Trying to set the amount of vCPUs above said value
> > in vcpuset will result in an error - and we can catch it here
> > (instead of later in the function) and print a nice warning to the user.
> ...
> > +    rc = libxl_domain_info(ctx, &dominfo, domid);
> > +    if (rc == ERROR_INVAL) {
> > +        fprintf(stderr, "Error: Domain %u does not exist.\n", domid);
> > +        return -rc;
> 
> Do we really return ERROR_INVAL for this ?  ...   Looks like we do.
> 
> OK then, although we are definitely going to have to change that at
> some point.  How tiresome.

I could add a new type - ERROR_NOTFOUND (of course as a seperate patch)
and change all of the libxl_domain_info users to take advantage of that
if you would like?

> 
> Thanks,
> Ian.

Re: [PATCH 2/4] libxl: vcpuset: Check max_vcpus argument against the maximum number of vCPUs the guest has set.

[Ian Campbell]

On Tue, 2015-02-03 at 10:45 -0500, Konrad Rzeszutek Wilk wrote:
> On Tue, Feb 03, 2015 at 03:11:11PM +0000, Ian Jackson wrote:
> > Konrad Rzeszutek Wilk writes ("[PATCH 2/4] libxl: vcpuset: Check max_vcpus argument against the maximum number of vCPUs the guest has set."):
> > > The maximum number of VCPUs the guest can have is determined during
> > > domain creation and is set by 'maxvcpus' parameter (in the guest
> > > config). Trying to set the amount of vCPUs above said value
> > > in vcpuset will result in an error - and we can catch it here
> > > (instead of later in the function) and print a nice warning to the user.
> > ...
> > > +    rc = libxl_domain_info(ctx, &dominfo, domid);
> > > +    if (rc == ERROR_INVAL) {
> > > +        fprintf(stderr, "Error: Domain %u does not exist.\n", domid);
> > > +        return -rc;
> > 
> > Do we really return ERROR_INVAL for this ?  ...   Looks like we do.
> > 
> > OK then, although we are definitely going to have to change that at
> > some point.  How tiresome.
> 
> I could add a new type - ERROR_NOTFOUND (of course as a seperate patch)
> and change all of the libxl_domain_info users to take advantage of that
> if you would like?

Sounds good (subject to anyone who wants to bikeshed the name). In
general we would like to move away from the catchall errors like
ERROR_INVAL (or, worse, ERROR_FAIL) towards more specific errors.

Ian.