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

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


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