Re: [libvirt] [PATCH v3 4/8] test: Implement snapshot create/delete/revert APIs

On 09/25/2013 03:15 PM, Cole Robinson wrote: > Again stolen from qemu_driver.c, but dropping all the unneeded bits. > This aims to copy all the current qemu validation checks since that's > the most commonly used real driver, but some of the checks are > completely artificial in the test driver. > > This only supports creation of internal snapshots for initial > simplicity. > --- > > v3: > Use STRNEQ_NULLABLE for domain_conf.c change > > src/conf/domain_conf.c | 2 +- > src/test/test_driver.c | 504 ++++++++++++++++++++++++++++++++++++++++++++++++- > 2 files changed, 504 insertions(+), 2 deletions(-) ... A RESOURCE_LEAK Coverity issue - it takes a bit to set up though... > +static int > +testDomainRevertToSnapshot(virDomainSnapshotPtr snapshot, > + unsigned int flags) > +{ > + testConnPtr privconn = snapshot->domain->conn->privateData; > + virDomainObjPtr vm = NULL; > + virDomainSnapshotObjPtr snap = NULL; > + virDomainEventPtr event = NULL; > + virDomainEventPtr event2 = NULL; > + virDomainDefPtr config = NULL; > + int ret = -1; > + > + virCheckFlags(VIR_DOMAIN_SNAPSHOT_REVERT_RUNNING | > + VIR_DOMAIN_SNAPSHOT_REVERT_PAUSED | > + VIR_DOMAIN_SNAPSHOT_REVERT_FORCE, -1); > + > + /* We have the following transitions, which create the following events: > + * 1. inactive -> inactive: none > + * 2. inactive -> running: EVENT_STARTED > + * 3. inactive -> paused: EVENT_STARTED, EVENT_PAUSED > + * 4. running -> inactive: EVENT_STOPPED > + * 5. running -> running: none > + * 6. running -> paused: EVENT_PAUSED > + * 7. paused -> inactive: EVENT_STOPPED > + * 8. paused -> running: EVENT_RESUMED > + * 9. paused -> paused: none > + * Also, several transitions occur even if we fail partway through, > + * and use of FORCE can cause multiple transitions. > + */ > + > + if (!(vm = testDomObjFromSnapshot(snapshot))) > + return -1; > + > + if (!(snap = testSnapObjFromSnapshot(vm, snapshot))) > + goto cleanup; > + > + testDriverLock(privconn); > + > + if (!vm->persistent && > + snap->def->state != VIR_DOMAIN_RUNNING && > + snap->def->state != VIR_DOMAIN_PAUSED && > + (flags & (VIR_DOMAIN_SNAPSHOT_REVERT_RUNNING | > + VIR_DOMAIN_SNAPSHOT_REVERT_PAUSED)) == 0) { > + virReportError(VIR_ERR_OPERATION_INVALID, "%s", > + _("transient domain needs to request run or pause " > + "to revert to inactive snapshot")); > + goto cleanup; > + } > + > + if (!(flags & VIR_DOMAIN_SNAPSHOT_REVERT_FORCE)) { > + if (!snap->def->dom) { > + virReportError(VIR_ERR_SNAPSHOT_REVERT_RISKY, > + _("snapshot '%s' lacks domain '%s' rollback info"), > + snap->def->name, vm->def->name); > + goto cleanup; > + } > + if (virDomainObjIsActive(vm) && > + !(snap->def->state == VIR_DOMAIN_RUNNING > + || snap->def->state == VIR_DOMAIN_PAUSED) && > + (flags & (VIR_DOMAIN_SNAPSHOT_REVERT_RUNNING | > + VIR_DOMAIN_SNAPSHOT_REVERT_PAUSED))) { > + virReportError(VIR_ERR_SNAPSHOT_REVERT_RISKY, "%s", > + _("must respawn guest to start inactive snapshot")); > + goto cleanup; > + } > + } > + > + > + if (vm->current_snapshot) { > + vm->current_snapshot->def->current = false; > + vm->current_snapshot = NULL; > + } > + > + snap->def->current = true; > + config = virDomainDefCopy(snap->def->dom, > + privconn->caps, privconn->xmlopt, true); > + if (!config) > + goto cleanup; > + 6953 goto cleanup; 6954 (20) Event cond_false: Condition "snap->def->state == VIR_DOMAIN_RUNNING", taking false branch (21) Event cond_false: Condition "snap->def->state == VIR_DOMAIN_PAUSED", taking false branch 6955 if (snap->def->state == VIR_DOMAIN_RUNNING || 6956 snap->def->state == VIR_DOMAIN_PAUSED) { > + if (snap->def->state == VIR_DOMAIN_RUNNING || > + snap->def->state == VIR_DOMAIN_PAUSED) { > + /* Transitions 2, 3, 5, 6, 8, 9 */ > + bool was_running = false; > + bool was_stopped = false; > + > + if (virDomainObjIsActive(vm)) { > + /* Transitions 5, 6, 8, 9 */ > + /* Check for ABI compatibility. */ > + if (!virDomainDefCheckABIStability(vm->def, config)) { > + virErrorPtr err = virGetLastError(); > + > + if (!(flags & VIR_DOMAIN_SNAPSHOT_REVERT_FORCE)) { > + /* Re-spawn error using correct category. */ > + if (err->code == VIR_ERR_CONFIG_UNSUPPORTED) > + virReportError(VIR_ERR_SNAPSHOT_REVERT_RISKY, "%s", > + err->str2); > + goto cleanup; > + } > + > + virResetError(err); > + testDomainShutdownState(snapshot->domain, vm, > + VIR_DOMAIN_SHUTOFF_FROM_SNAPSHOT); > + event = virDomainEventNewFromObj(vm, > + VIR_DOMAIN_EVENT_STOPPED, > + VIR_DOMAIN_EVENT_STOPPED_FROM_SNAPSHOT); > + if (event) > + testDomainEventQueue(privconn, event); > + goto load; > + } > + > + if (virDomainObjGetState(vm, NULL) == VIR_DOMAIN_RUNNING) { > + /* Transitions 5, 6 */ > + was_running = true; > + virDomainObjSetState(vm, VIR_DOMAIN_PAUSED, > + VIR_DOMAIN_PAUSED_FROM_SNAPSHOT); > + /* Create an event now in case the restore fails, so > + * that user will be alerted that they are now paused. > + * If restore later succeeds, we might replace this. */ > + event = virDomainEventNewFromObj(vm, > + VIR_DOMAIN_EVENT_SUSPENDED, > + VIR_DOMAIN_EVENT_SUSPENDED_FROM_SNAPSHOT); > + } > + virDomainObjAssignDef(vm, config, false, NULL); > + > + } else { > + /* Transitions 2, 3 */ > + load: > + was_stopped = true; > + virDomainObjAssignDef(vm, config, false, NULL); > + if (testDomainStartState(privconn, vm, > + VIR_DOMAIN_RUNNING_FROM_SNAPSHOT) < 0) > + goto cleanup; > + event = virDomainEventNewFromObj(vm, > + VIR_DOMAIN_EVENT_STARTED, > + VIR_DOMAIN_EVENT_STARTED_FROM_SNAPSHOT); > + } > + > + /* Touch up domain state. */ > + if (!(flags & VIR_DOMAIN_SNAPSHOT_REVERT_RUNNING) && > + (snap->def->state == VIR_DOMAIN_PAUSED || > + (flags & VIR_DOMAIN_SNAPSHOT_REVERT_PAUSED))) { > + /* Transitions 3, 6, 9 */ > + virDomainObjSetState(vm, VIR_DOMAIN_PAUSED, > + VIR_DOMAIN_PAUSED_FROM_SNAPSHOT); > + if (was_stopped) { > + /* Transition 3, use event as-is and add event2 */ > + event2 = virDomainEventNewFromObj(vm, > + VIR_DOMAIN_EVENT_SUSPENDED, > + VIR_DOMAIN_EVENT_SUSPENDED_FROM_SNAPSHOT); > + } /* else transition 6 and 9 use event as-is */ > + } else { > + /* Transitions 2, 5, 8 */ > + virDomainEventFree(event); > + event = NULL; > + > + if (was_stopped) { > + /* Transition 2 */ > + event = virDomainEventNewFromObj(vm, > + VIR_DOMAIN_EVENT_STARTED, > + VIR_DOMAIN_EVENT_STARTED_FROM_SNAPSHOT); > + } else if (was_running) { > + /* Transition 8 */ > + event = virDomainEventNewFromObj(vm, > + VIR_DOMAIN_EVENT_RESUMED, > + VIR_DOMAIN_EVENT_RESUMED); > + } > + } 7042 } (22) Event else_branch: Reached else branch 7043 } else { > + } else { > + /* Transitions 1, 4, 7 */ > + virDomainObjAssignDef(vm, config, false, NULL); > + (23) Event cond_false: > + if (virDomainObjIsActive(vm)) { > + /* Transitions 4, 7 */ > + testDomainShutdownState(snapshot->domain, vm, > + VIR_DOMAIN_SHUTOFF_FROM_SNAPSHOT); > + event = virDomainEventNewFromObj(vm, > + VIR_DOMAIN_EVENT_STOPPED, > + VIR_DOMAIN_EVENT_STOPPED_FROM_SNAPSHOT); (24) Event if_end: End of if statement > + } > + (25) Event cond_true: Condition "flags & (3U /* VIR_DOMAIN_SNAPSHOT_REVERT_RUNNING | VIR_DOMAIN_SNAPSHOT_REVERT_PAUSED */)", taking true branch > + if (flags & (VIR_DOMAIN_SNAPSHOT_REVERT_RUNNING | > + VIR_DOMAIN_SNAPSHOT_REVERT_PAUSED)) { > + /* Flush first event, now do transition 2 or 3 */ > + bool paused = (flags & VIR_DOMAIN_SNAPSHOT_REVERT_PAUSED) != 0; > + (26) Event cond_false: Condition "event", taking false branch > + if (event) (27) Event if_end: End of if statement > + testDomainEventQueue(privconn, event); > + event = virDomainEventNewFromObj(vm, > + VIR_DOMAIN_EVENT_STARTED, > + VIR_DOMAIN_EVENT_STARTED_FROM_SNAPSHOT); (28) Event cond_true: Condition "paused", taking true branch > + if (paused) { (29) Event alloc_fn: Storage is returned from allocation function "virDomainEventNewFromObj(virDomainObjPtr, int, int)". [details] (30) Event var_assign: Assigning: "event2" = storage returned from "virDomainEventNewFromObj(vm, 3, 5)". Also see events: [leaked_storage] > + event2 = virDomainEventNewFromObj(vm, > + VIR_DOMAIN_EVENT_SUSPENDED, > + VIR_DOMAIN_EVENT_SUSPENDED_FROM_SNAPSHOT); > + } > + } > + } > + > + vm->current_snapshot = snap; > + ret = 0; > +cleanup: > + if (event) { > + testDomainEventQueue(privconn, event); > + if (event2) > + testDomainEventQueue(privconn, event2); > + } Leads to the following Coverity issue - 7076 cleanup: (31) Event cond_false: Condition "event", taking false branch 7077 if (event) { 7078 testDomainEventQueue(privconn, event); 7079 if (event2) 7080 testDomainEventQueue(privconn, event2); (32) Event if_end: End of if statement 7081 } 7082 virObjectUnlock(vm); 7083 testDriverUnlock(privconn); 7084 (33) Event leaked_storage: Variable "event2" going out of scope leaks the storage it points to. Also see events: [alloc_fn][var_assign] 7085 return ret; 7086 } 7087 This one is a bit more "difficult" to see, but I think the complaint is that it's possible that 'event' is NULL at line 7063, but it's never checked and that 'event2' being allocated is based on 'paused' and not 'event' being non NULL, thus when we come to this point in the code the event == NULL will cause event2 to be leaked.