[Kimchi-devel] [PATCH] Remove static qemu URIs and add function to lookup the system URI

[Crístian Viana]

On Sex, 2014-10-03 at 14:27 -0500, bbaude at redhat.com wrote:
> +    @staticmethod
> +    def getSystemURI():
> +        """
> +        This method looks up the livbirt URI for the system instead of
> +        relying on a statically defined URI.
> +        """
> +        try:
> +            uriconn = libvirt.openReadOnly(None)
> +            return uriconn.getURI()
> +        except:
> +            err = 'Unable to connect to libvirtd to determine the URI'
> +            kimchi_log.error(err)

"uriconn" should be closed. Otherwise, for every call to "getSystemURI"
one connection object will leak.

And I think in that case you shouldn't handle an exception. If an
exception is actually raised, that function will still continue.
Whatever has called that function will get "None" as result and try to
connect with it. And given that this function already tried to connect
with the URI "None" and an exception has been raised, another exception
will be raised if the caller function tries the same thing. I mean,
handling an exception here is just going to postpone an unexpected
exception which will surely by raised later.

You can 1) not handle anything at all; if an exception is raised, no
connections will be made later (well, that makes sense as we didn't find
an URI); 2) handle the exception and return a default URI (e.g.
"qemu:///system") if something wrong happens.

> +        if (self.conn.isQemuURI(libvirt_uri)):
>              for pool_name, pool_arg in DEFAULT_POOLS.iteritems():
>                  self._default_pool_check(pool_name, pool_arg)
> 

The function "isQemuURI" is static. Even though the code above - and all
the other occurrences below - works, I'd suggest you to use
"LibvirtConnection.isQemuURI", just like you did with
"LibvirtConnection.getSystemURI".

-- 
Best regards,
Crístian.