[Libvirt-cim] [PATCH] Make sure provider revision is a number

[John Ferlan]

On 09/17/2013 08:54 AM, Viktor Mihajlovski wrote:
> Commit 4a7fae9 changed the revision returned by get_provider_version
> to a string. However, the revision has to be a number because (since
> git is used) it's basically the number of commits in HEAD.
> Comparing a string against an int in Python 2.x will always result in
> the string value being greater than the int value. Since there
> are a lot of compares against revision numbers all over the test
> scripts, this change is probably the most economic.
> 
> Signed-off-by: Viktor Mihajlovski <mihajlov at linux.vnet.ibm.com>
> ---
> Not being a python guy I got wacked myself when I was testing
> my (soon to come) cimtest changes against a pre-console libvirt-cim.
> '1272' > 1276 silently returned True leading to epic failures :-( 
> 
>  suites/libvirt-cim/lib/XenKvmLib/const.py     |    4 +++-
>  suites/libvirt-cim/lib/XenKvmLib/reporting.py |    2 +-
>  2 files changed, 4 insertions(+), 2 deletions(-)
> 

I guess this is a mea culpa and me too moment all wrapped into one.
Python isn't necessarily my strength and I probably should have looked
more deeply into all the callers.  I guess I was too hyperfocused on the
error I got that resulted in the bad changes I made...

> diff --git a/suites/libvirt-cim/lib/XenKvmLib/const.py b/suites/libvirt-cim/lib/XenKvmLib/const.py
> index a8d0254..a454b2f 100755
> --- a/suites/libvirt-cim/lib/XenKvmLib/const.py
> +++ b/suites/libvirt-cim/lib/XenKvmLib/const.py
> @@ -179,7 +179,9 @@ def get_provider_version(virt, ip):
>      revision = revision.strip("+")
>      if revision.isdigit():
>          revision = int(revision)
> +    else:
> +        raise Exception("revision %s is not a digit", revision)
>  
> -    return str(revision), str(changeset)
> +    return revision, changeset
>  
>  
> diff --git a/suites/libvirt-cim/lib/XenKvmLib/reporting.py b/suites/libvirt-cim/lib/XenKvmLib/reporting.py
> index 67ec974..88375b0 100644
> --- a/suites/libvirt-cim/lib/XenKvmLib/reporting.py
> +++ b/suites/libvirt-cim/lib/XenKvmLib/reporting.py
> @@ -101,7 +101,7 @@ def get_env_data(ip, virt):
>      rev, changeset = get_provider_version(virt, ip)
>      cimtest_revision, cimtest_changeset = get_cimtest_version()
>  
> -    lc_ver = "Libvirt-cim revision: %s\nLibvirt-cim changeset: %s\n" % \
> +    lc_ver = "Libvirt-cim revision: %d\nLibvirt-cim changeset: %s\n" % \

Still not quite right - 'changeset' is more of a hexadecimal value, but
printing as such (either %x or %d) would result in a traceback and the
error message "a number is required, not unicode".

If I print 'Revision' and 'Changeset' in 'get_provider_version' right
after they are fetched from 'inst', I get:

1262
eb776e2

To "fix" I changed the "(rev, changeset)" to "(rev, str(changeset))"

I adjusted and pushed.

Of course I did a bit more digging just to be sure...

'changeset' is ever compared against is 'sles11_changeset'

It's initially generated in the libvirt-cim build as a "'git rev-parse
--short HEAD' > .changeset" by autoconfiscate.sh.  If unavailable it's
set to 'Unknown' in the configure script as part of a build environment
variable "LIBVIRT_CIM_CS".

While at it - I checked the generation of "revision" too.  In this case,
it's the result of the number of commits from a "git rev-list HEAD | wc
-l > .revision". It's then saved/used as the build environment variable
"LIBVIRT_CIM_RV". In my example, it shows as 1262, although to be
technically correct it should be 1279.  If it's not possible to get the
value it defaults to '0' (zero).  So printing/using as an 'int' is
certainly the right thing to do.

I'm not a "build guru", but I think the libvirt-cim build probably needs
to generate the values more frequently than just in autoconfiscate.sh
seeing as that's 'perhaps' run less frequently than  someone making
changes to the git repository and rebuilding...

John
>               (rev, changeset)
>      cimtest_ver = "Cimtest revision: %s\nCimtest changeset: %s\n" % \
>                    (cimtest_revision, cimtest_changeset)
>