[libvirt] [libvirt-glib 2/2] conn-test: Fix gvir_connection_get_version error check

[Christophe Fergeau]

Hey,

On Wed, Nov 14, 2012 at 03:45:49PM +0100, Michal Privoznik wrote:
> On 14.11.2012 15:16, Christophe Fergeau wrote:
> > This method returns a version number, which can be 0, so we cannot
> > check the return value for 0 to know if an error happened. Test if
> > the GError is set instead to detect errors.
> > ---
> >  examples/conn-test.c | 3 ++-
> >  1 file changed, 2 insertions(+), 1 deletion(-)
> > 
> > diff --git a/examples/conn-test.c b/examples/conn-test.c
> > index b98d115..3e0e148 100644
> > --- a/examples/conn-test.c
> > +++ b/examples/conn-test.c
> > @@ -47,7 +47,8 @@ do_connection_open(GObject *source,
> >  
> >      g_print("Hypervisor name: %s\n", hv_name);
> >  
> > -    if (!(hv_version = gvir_connection_get_version(conn, &err))) {
> > +    hv_version = gvir_connection_get_version(conn, &err);
> > +    if (err != NULL) {
> >          g_error("%s", err->message);
> >      }
> >  
> > 
> 
> ACK

Looking a bit more at this, the situation is a bit more complicated.
virConnectGetVersion documentation says:
"* Returns -1 in case of error, 0 otherwise. if the version can't be
 *    extracted by lack of capacities returns 0 and @hvVer is 0, otherwise
 *    @hvVer value is major * 1,000,000 + minor * 1,000 + release
 *"

virConnectGetVersion returning 0 indicates some kind of error, so it would
probably be better for gvir_connection_get_version to set a GError in this
case rather than just returning 0. Ie something like the patch below may be
more correct:

diff --git a/libvirt-gobject/libvirt-gobject-connection.c
b/libvirt-gobject/libvirt-gobject-connection.c
index 3157a66..500de78 100644
--- a/libvirt-gobject/libvirt-gobject-connection.c
+++ b/libvirt-gobject/libvirt-gobject-connection.c
@@ -1108,6 +1108,11 @@ gvir_connection_get_version(GVirConnection *conn,
     if (virConnectGetVersion(priv->conn, &ret) < 0) {
         gvir_set_error_literal(err, GVIR_CONNECTION_ERROR, 0,
                                "Unable to get hypervisor version");
+        ret = 0;
+    } else if (ret == 0) {
+        /* From virConnectGetVersion doc: "if the version can't be
+         * extracted by lack of capacities returns 0 and @hvVer is 0" */
+        g_set_error_literal(err, GVIR_CONNECTION_ERROR, 0, "XXXX");
     }

 cleanup:

As a side note, I get virConnectGetVersion to return 0 by running libvirtd
1.0.0 as a user, and then running virsh version with virsh 0.10.2. It seems
to me that in this case virConnectGetVersion should either work or return -1,
but not 0. I had a quick look at the code, but stopped when I saw there
were remote calls involved...

Any thoughts on all of this?

Christophe
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 198 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/libvir-list/attachments/20121115/d0b06c24/attachment-0001.sig>