On Wed, Mar 28, 2007 at 11:06:01AM +0100, Richard W.M. Jones wrote:
> 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.
I don't see how an inactive domain could have an ID which is kind
of the same as an process ID but at the hypervisor level.
> The first case happens because xm_internal.c marks inactive domains by
> setting ->id = -1:
Since it is not running that sounds reasonnable.
> 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()
I tend to think it's wrong, it should check the domain status instead.
> 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.
Hum, an inactive domain should still have a name, even if it has an
id, now that sounds broken to me.
> 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.
To me it's an error. You ask for something which clearly isn't available.
If the fact of being inactive is only represented internally by having
id == -1 then we probably made a mistake, we should have
domain->flags & DOMAIN_IS_DEFINED
bit set.
We have no small entry point to tell if a domain is just defined and
not running, so the id == -1 trick has been used as a differenciator
but that seems kind of broken in retrospect.
But let's separate the 2 issues, apply that patch, and let's try to solve
the way to detect defined domain easilly and properly.
Daniel
--
Red Hat Virtualization group http://redhat.com/virtualization/
Daniel Veillard | virtualization library http://libvirt.org/
veillard@xxxxxxxxxx | libxml GNOME XML XSLT toolkit http://xmlsoft.org/
http://veillard.com/ | Rpmfind RPM search engine http://rpmfind.net/