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

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

 



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




[Index of Archives]     [Virt Tools]     [Libvirt Users]     [Lib OS Info]     [Fedora Users]     [Fedora Desktop]     [Fedora SELinux]     [Big List of Linux Books]     [Yosemite News]     [KDE Users]     [Fedora Tools]