+ try: + + for i in range(1, (timeout + 1)): + sleep(1) + cs = computersystem.get_cs_class(virt)(server, test_dom) + if cs.Name == test_dom: + to_RequestedState = cs.RequestedState + enabledState = cs.EnabledState + else: + logger.error("VS %s not found" % test_dom) + return status
You'll want to break here, otherwise the guest doesn't get destroyed properly.
+ if enabledState == expstate: + status = PASS + break
Instead of breaking here, you can just return since we've found the value we're looking for.
- + + status, to_RequestedState, enabledState = poll_for_status(options.ip, options.virt, cxml, 2) + if status != PASS or enabledState != 2:
No need to verify enabledState == 2, the poll_for_status function do this already.
+ return status + #Suspend the VS - rc = call_request_state_change(test_dom, options.ip, REQUESTED_STATE, - TIME, options.virt) + rc = call_request_state_change(test_dom, options.ip, REQUESTED_STATE, TIME, options.virt)
This change causes the test to span multiple lines. I'll work on sending out some suggested style guidelines and post them to the list to see if people agree.
- except Exception, detail: - logger.error("Exception variable: %s" % detail) - return status - + status, to_RequestedState, enabledState = poll_for_status(options.ip, options.virt, cxml, + FINAL_STATE)
Unusual formatting here. Usually, the second line of arguments for a function will line up with the first argument. Something like: status, to_RequestedState, enabledState = poll_for_status(options.ip, options.virt, cxml, FINAL_STATE) Hopefully the formatting of the email doesn't kill the illustration.
if enabledState != FINAL_STATE: logger.error("EnabledState has %i instead of %i", enabledState, FINAL_STATE) logger.error("Try to increase the timeout and run the test again")
if status != PASS: - ret = cxml.destroy(options.ip) - cxml.undefine(options.ip)
Why remove the destroy and undefine here? The guest won't get cleaned up properly.
return status
# Success: @@ -134,9 +144,8 @@ def main(): # To state == 2 # Enabled_state == RequestedState
- if from_State == START_STATE and \ - to_RequestedState == FINAL_STATE and \ - enabledState == to_RequestedState: + if from_State == START_STATE and to_RequestedState == FINAL_STATE and \ + enabledState == to_RequestedState:
This is unusual indention style. Usually, the new line starts just below the if or the first part of the statement. So you'd have: if from_State == START_STATE and to_RequestedState == FINAL_STATE and \ enabledState == to_RequestedState: or if from_State == START_STATE and to_RequestedState == FINAL_STATE and \ enabledState == to_RequestedState: My preference is the latter. You can hold off on changing this patch until we agree on a coding style. -- Kaitlin Rupert IBM Linux Technology Center kaitlin@linux.vnet.ibm.com