[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]

[Libvir] Re: [PATCH] Python bindings now generate exceptions for libvirt errors (third version)



Daniel Veillard wrote:
So this patch disables exceptions in those two functions only.

If you are 100% sure you can't pass a wrong pointer though the python binding then okay.


OK so the problem here is that virDomainGetID can return -1 to mean one of two things: Either that the domain is inactive. Or that an error occurred.

The first case happens because xm_internal.c marks inactive domains by setting ->id = -1:

virDomainPtr xenXMDomainLookupByName(virConnectPtr conn, const char *domname)
    //...

    /* Ensure its marked inactive, because may be cached
       handle to a previously active domain */
    ret->id = -1;

(and other places).

virt-manager contains code which relies on this, specifically:

    def current_memory_pretty(self):
        if self.get_id() == -1:
            return "0.00 MB"
        return self.get_memory_pretty()

The second case (error) is when domain is NULL or corrupt:

    unsigned int
    virDomainGetID(virDomainPtr domain)
    {
        if (!VIR_IS_DOMAIN(domain)) {
virLibDomainError(domain, VIR_ERR_INVALID_DOMAIN, __FUNCTION__);
            return ((unsigned int) -1);
        }
        return (domain->id);
    }

Anyway, adding code to throw an exception when get_id returns -1 (as in patch versions 1 & 2) wasn't clever because it breaks the first case. So the third version of the patch doesn't add the exception code for just this function and virDomainGetName which is similar.

We're not making things worse by not adding exception code to this function (after all - it didn't throw an exception before, and it doesn't throw one now: so no change!). But in general there is a real problem here, because we ought to throw an exception when there's an error.

>> Note that the documentation for virDomainGetID is wrong.
>
>   I don't see the error

The virDomainGetID documentation is wrong when it says:

   Returns the domain ID number or (unsigned int) -1 in case of error

It should say something like:

   Returns the domain ID number.  Returns (unsigned int) -1 either for
   error or in the Xen case if the domain is inactive.

Rich.

--
Emerging Technologies, Red Hat  http://et.redhat.com/~rjones/
64 Baker Street, London, W1U 7DF     Mobile: +44 7866 314 421
 "[Negative numbers] darken the very whole doctrines of the equations
 and make dark of the things which are in their nature excessively
 obvious and simple" (Francis Maseres FRS, mathematician, 1759)

Attachment: smime.p7s
Description: S/MIME Cryptographic Signature


[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]