Bug in xm config parsing code

Bug in xm config parsing code

[Tim Evers]

Hi,

I think I've found a bug in xm/create.py:

http://lists.xensource.com/archives/html/xen-changelog/2009-11/msg00132.html

introduces the "maxvcpus" parameter. It states that

"this is patch to add maxvcpus support to xen xm command. It's using
vcpu_avail bitmask and sets the number of vcpus to maxvcpus if
present.  If it's not present, old behavior is preserved."

The last part is untrue as far as I understand. If maxvcpus is unset,
vcpu_avail is not read at all. The patch removes it from the parameter
list given to add_conf without adding it in another place. As a result
vcpu_avail remains unset and is filled with the default value of 255 (?)
later.

This patch to fixes the issue for me:

--- create.py	2010-09-08 15:52:18.000000000 +0200
+++ create.py.af	2010-09-13 16:59:48.000000000 +0200
@@ -1120,8 +1120,10 @@

         # For case we don't have maxvcpus set but we have vcpus we preserve
         # old behaviour
+	# te@artfiles.de: need to read vcpu_avail here since it isn't done later.
         if not maxvcpus and vcpus:
             config.append(['vcpus', vcpus])
+            config.append(['vcpu_avail', getattr(vals, 'vcpu_avail'])

     def add_conf(n):
         if hasattr(vals, n):

I would have filed a bug report for this but I have not found Xen4 on
Xen Bugzilla. Any hints on this?

Regards

Tim Evers
Artfiles GmbH

Re: Bug in xm config parsing code

[Ian Jackson]

Tim Evers writes ("[Xen-devel] Bug in xm config parsing code"):
> I think I've found a bug in xm/create.py:
> http://lists.xensource.com/archives/html/xen-changelog/2009-11/msg00132.html
> introduces the "maxvcpus" parameter. It states that

Your explanation is plausible.  I have CC'd the author of the patch
you're talking about (changeset dc121a3c3a92).

Michal: would you like to have an opinion ?  I'm inclined to apply
Tim's patch.

> +	# te@artfiles.de: need to read vcpu_avail here since it isn't done later.

Tim: FYI, there is no for that kind of comment, as our revision
control system will keep track of these kind of things.  The prose
part of your email will make a good checkin comment and allow future
readers to see exactly what was done why.

Also, you should normally provide a Signed-off-by.  You do it like
this:
   Signed-off-by: Ian Jackson <ian.jackson@eu.citrix.com>
except with your name and email address.  This indicates that you are
certifying the code as suitable (copyright-wise and so on) for
inclusion in Xen.  You can find the precise meaning in the Linux
upstream kernel tree (Documentation/SubmittingPatches, copy below).

However for this trivial one-line change I'd be prepared to apply your
change without it, so there is no need for you to resend or sign off
now.

Thanks,
Ian.

>From Documentation/SubmittingPatches:

       Developer's Certificate of Origin 1.1

       By making a contribution to this project, I certify that:

       (a) The contribution was created in whole or in part by me and I
           have the right to submit it under the open source license
           indicated in the file; or

       (b) The contribution is based upon previous work that, to the best
           of my knowledge, is covered under an appropriate open source
           license and I have the right under that license to submit that
           work with modifications, whether created in whole or in part
           by me, under the same open source license (unless I am
           permitted to submit under a different license), as indicated
           in the file; or

       (c) The contribution was provided directly to me by some other
           person who certified (a), (b) or (c) and I have not modified
           it.

       (d) I understand and agree that this project and the contribution
           are public and that a record of the contribution (including all
           personal information I submit with it, including my sign-off) is
           maintained indefinitely and may be redistributed consistent with
           this project or the open source license(s) involved.

--