On 10/04/2013 07:05 AM, John Ferlan wrote:
> 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.
>
Hmm, the logic in this code is pretty intense :) In the followup patch I've added:
@@ -7078,6 +7076,8 @@ cleanup:
testDomainEventQueue(privconn, event);
if (event2)
testDomainEventQueue(privconn, event2);
+ } else {
+ virDomainEventFree(event2);
}
virObjectUnlock(vm);
testDriverUnlock(privconn);
Hopefully that covers it.
Thanks,
Cole
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list