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 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.

John


> +    virObjectUnlock(vm);
> +    testDriverUnlock(privconn);
> +
> +    return ret;
> +}
> +
> +

--
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]