[libvirt] [Patch V2] Fix initialization of current vcpus in libxl driver
The cur_vcpus member of struct libxl_domain_build_info was incorrectly initialized to the number of vcpus, when it should have been interpreted as a bitmap, where bit X corresponds to online/offline status of vcpuX. To complicate matters, cur_vcpus is an int, so only 32 vcpus can be set online. Add a check to ensure vcpus does not exceed this limit. V2: Eric Blake noted a compilation pitfal when '1 << 32' on an int. Account for vcpus == 32. --- src/libxl/libxl_conf.c | 17 ++++++++++++++++- 1 files changed, 16 insertions(+), 1 deletions(-) diff --git a/src/libxl/libxl_conf.c b/src/libxl/libxl_conf.c index 3cebf41..649352d 100644 --- a/src/libxl/libxl_conf.c +++ b/src/libxl/libxl_conf.c @@ -379,11 +379,26 @@ libxlMakeDomBuildInfo(virDomainDefPtr def, libxl_domain_config *d_config) int hvm = STREQ(def->os.type, "hvm"); int i; + /* Currently, libxenlight only supports 32 vcpus per domain. + * cur_vcpus member of struct libxl_domain_build_info is defined + * as an int, but its' semantic is a bitmap of online vcpus, so + * only 32 can be represented. + */ + if (def->maxvcpus > 32 || def->vcpus > 32) { + libxlError(VIR_ERR_INTERNAL_ERROR, + _("This version of libxenlight only supports 32 " + "vcpus per domain")); + return -1; + } + libxl_init_build_info(b_info, &d_config->c_info); b_info->hvm = hvm; b_info->max_vcpus = def->maxvcpus; - b_info->cur_vcpus = def->vcpus; + if (def->vcpus == 32) + b_info->cur_vcpus = 1 << 31; + else + b_info->cur_vcpus = (1 << def->vcpus) - 1; if (def->clock.ntimers > 0 && def->clock.timers[0]->name == VIR_DOMAIN_TIMER_NAME_TSC) { switch (def->clock.timers[0]->mode) { -- 1.7.3.1
On 05/24/2011 11:22 AM, Jim Fehlig wrote:
The cur_vcpus member of struct libxl_domain_build_info was incorrectly initialized to the number of vcpus, when it should have been interpreted as a bitmap, where bit X corresponds to online/offline status of vcpuX.
To complicate matters, cur_vcpus is an int, so only 32 vcpus can be set online. Add a check to ensure vcpus does not exceed this limit.
V2: Eric Blake noted a compilation pitfal when '1 << 32' on an int.
If you keep the V2 message in your commit log, then s/pitfal/pitfall/ (personally, I tend to drop V2 messages when pushing - they are handy during review to point out what to look for, but once it is part of history, I no longer care how many versions it took to get to the final pushed version).
Account for vcpus == 32.
+ * as an int, but its' semantic is a bitmap of online vcpus, so
s/its'/its/ (That is, drop the spurious apostrophe).
b_info->hvm = hvm; b_info->max_vcpus = def->maxvcpus; - b_info->cur_vcpus = def->vcpus; + if (def->vcpus == 32) + b_info->cur_vcpus = 1 << 31;
Still wrong. You want 32 bits, not one bit: b_info->cur_vcpus = (uint32_t) -1;
+ else + b_info->cur_vcpus = (1 << def->vcpus) - 1; if (def->clock.ntimers > 0 && def->clock.timers[0]->name == VIR_DOMAIN_TIMER_NAME_TSC) { switch (def->clock.timers[0]->mode) {
ACK with those changes. -- Eric Blake eblake@redhat.com +1-801-349-2682 Libvirt virtualization library http://libvirt.org
Eric Blake wrote:
On 05/24/2011 11:22 AM, Jim Fehlig wrote:
The cur_vcpus member of struct libxl_domain_build_info was incorrectly initialized to the number of vcpus, when it should have been interpreted as a bitmap, where bit X corresponds to online/offline status of vcpuX.
To complicate matters, cur_vcpus is an int, so only 32 vcpus can be set online. Add a check to ensure vcpus does not exceed this limit.
V2: Eric Blake noted a compilation pitfal when '1 << 32' on an int.
If you keep the V2 message in your commit log, then s/pitfal/pitfall/ (personally, I tend to drop V2 messages when pushing - they are handy during review to point out what to look for, but once it is part of history, I no longer care how many versions it took to get to the final pushed version).
Account for vcpus == 32.
+ * as an int, but its' semantic is a bitmap of online vcpus, so
s/its'/its/
(That is, drop the spurious apostrophe).
b_info->hvm = hvm; b_info->max_vcpus = def->maxvcpus; - b_info->cur_vcpus = def->vcpus; + if (def->vcpus == 32) + b_info->cur_vcpus = 1 << 31;
Still wrong. You want 32 bits, not one bit:
b_info->cur_vcpus = (uint32_t) -1;
Err, right - I should address _all_ previous comments.
+ else + b_info->cur_vcpus = (1 << def->vcpus) - 1; if (def->clock.ntimers > 0 && def->clock.timers[0]->name == VIR_DOMAIN_TIMER_NAME_TSC) { switch (def->clock.timers[0]->mode) {
ACK with those changes.
And this time I have. Changes made and pushed. Thanks, Jim
participants (2)
-
Eric Blake -
Jim Fehlig