Re: [libvirt] [PATCH] Domain Events Python Bindings
Comments inline below
Daniel Veillard wrote on 10/29/2008 01:09 PM:
...
> +def myDomainEventCallback1 (conn, dom, event, opaque):
> + print "myDomainEventCallback1 EVENT: Domain %s(%s) %s" % (dom.name(),
dom.ID(), eventToString(event))
> +
> +def myDomainEventCallback2 (conn, dom, event, opaque):
> + print "myDomainEventCallback2 EVENT: Domain %s(%s) %s" % (dom.name(),
dom.ID(), eventToString(event))
Thinking out loud, maybe we should allow dom to be NULL/None in
examples, if we extend the API later to add node related events dom
would be NULL, isn't it
I was under the impression that re-use of this API was undesired, and that the events API
would be extended to call out each event class explicitly (IIRC, Daniel B suggested this)
If we were to extend the API, there would be a
virConnectNodeEventRegister/Deregister
and associated callbacks, with their own signatures.
So - we would not be mxing NodeEventCallbacks, and DomainEventCallbacks with the same
python code.
[...]
> +##########################################
> +# Main
> +##########################################
> +
> +def usage():
> + print "usage: "+os.path.basename(sys.argv[0])+" [uri]"
> + print " uri will default to qemu:///system"
ideally for regression testing it would be nice to be able to provide
a test driver definition, but augmented with an emulated scenario,
things like:
<scenario>
<event type="start">
<domain type='test2'>
<name>test2</name>
<memory>512000</memory>
<currentMemory>512000</currentMemory>
<vcpu>2</vcpu>
<os>
<type arch='i686'>hvm</type>
<boot dev='hd'/>
</os>
</domain>
</event>
<event type="pause">
<domain name="test"/>
</event>
<event type="resume">
<domain name="test"/>
</event>
<event type="destroy">
<domain name="test2"/>
</event>
</scenario>
I really have no knowledge of how the test driver works, or how to design a test of this
code. Ad discussed in an earlier thread - since this event code depends on OS state, I
made no effort to design tests to exercise the code, beyond the example app. While I agree
that a regression test would be desirable, I really don't know how that would be
accomplished.
...
> +static PyObject *
> +libvirt_virEventRegisterImpl(ATTRIBUTE_UNUSED PyObject * self,
> + PyObject * args)
> +{
> + Py_XDECREF(addHandleObj);
> + Py_XDECREF(updateHandleObj);
> + Py_XDECREF(removeHandleObj);
> + Py_XDECREF(addTimeoutObj);
> + Py_XDECREF(updateTimeoutObj);
> + Py_XDECREF(removeTimeoutObj);
hum, maybe the ParseTuple should be done before onto temporary
variables and then only DECREF, right now if the parse fails, then
the next event may lead to a crash due to garbage collected code :-)
hmmm...maybe I'm misunderstanding how XDECREF works...I thought it would not dec the
ref if the object was NULL
Other than the above comments, I think the other suggestions are good ones. I'll take
a look.
Ben