VirtualBox

Ticket #5182 (closed defect: invalid)

Opened 5 years ago

Last modified 4 years ago

'Discard Current Snapshot and State' feature is broken

Reported by: MarkCranness Owned by:
Priority: major Component: virtual disk
Version: VirtualBox 3.0.8 Keywords: Snapshot, Discard, Revert
Cc: Guest type: other
Host type: other

Description

The VirtualBox Snapshot 'Discard Current Snapshot and State' feature is broken.

1) To see the problem, perform the following steps:

  • Create a new snapshot named: '1 January'
  • Boot the VM and create a file called 'Work.txt', with this contents:

January February March (This file represents changes done over 3 months since the 1st January snapshot.)

  • Shutdown the VM. Suppose on 1 April the user wants to test an experimental change.
  • Create a new snapshot named: 'Before 1 April Experiment', intended to protect the 'Work' while the experiment is done.
  • Boot the VM and edit file 'Work.txt', adding 'Experimental change'.
  • Shutdown the VM.

Suppose the user decides that the experiment was not wanted, and they want to undo the change. They want to Revert the current state to what it was when the 'Before 1 April Experiment' snapshot was taken, and then delete that snapshot.

The VirtualBox UI provides Snapshot management buttons that can help the user unwind the change. One such button is 'Discard Current Snapshot and State'. Although the label on this button is ambigious, it seems that it will do what they want.

Hovering the mouse over this button displays the following information in the VirtualBox status bar: 'Discard the current snapshot and revert the machine to the state it had before the snapshot was taken'

Assuming the the 'current snapshot' is the most recent 1st April snapshot, then reverting the machine to the state it had before the 'Before 1 April Experiment' snapshot was taken is exactly what is required. Just before the 1st April snapshot was taken, file 'Work.txt' had January/February/March in it, representing 3 months of work.

  • Click the 'Discard Current Snapshot and State' button.
  • Boot the VM and examine the contents of file 'Work.txt'. File WORK.TXT is not present!

The 'Discard Current Snapshot and State' button has not only reverted changes since the 1st April snapshot, and removed the 1st April snapshot, but it has ALSO reverted all changes since the 1st January snapshot, and erased 3 months of work!

This is a severe bug. If this is not a bug (it is) then this is a severe design error.

(To people that might say: 'this is working as designed', I say: Rubbish! A feature that reverts back TWO snapshots is just silly and mostly useless. Why not go back 3 or more? Where is the useful feature that just goes back 1 snapshot and deletes that snapshot?)

NOTE: The 'Discard Current Snapshot and State' button acts differently depending upon if there are is ONLY ONE snapshot when it is run, or of there are 2 or more snapshots when it is run. If the above steps are repeated, but excluding the creation of the '1 January' snapshot (start from a proper root hard disk) and ensure that the ONLY snapshot present is the 1st April one, then 'Discard Current Snapshot and State' performs as expected according to the status bar information text.

2) This different behaviour can be seen in the source code (see below).

'Revert to Current Snapshot' and 'Discard Current Snapshot and State' share some source code. The (pseudo) code looks like so:

// http://www.virtualbox.org/browser/trunk/src/VBox/Main/MachineImpl.cpp?rev=13762#L8646
DiscardCurrentState() { // Implements 'Revert to Current Snapshot'
    ...
    DiscardCurrentStateHandler(false); // discardCurSnapshot=false
    ...
}

// http://www.virtualbox.org/browser/trunk/src/VBox/Main/MachineImpl.cpp?rev=13762#L8710
DiscardCurrentSnapshotAndState() { // Implements 'Discard Current Snapshot and State'
    ...
    DiscardCurrentStateHandler(true); // discardCurSnapshot=true
    ...
}

// http://www.virtualbox.org/browser/trunk/src/VBox/Main/MachineImpl.cpp?rev=13762#L9902
DiscardCurrentStateHandler(bool discardCurrentSnapshot) {
    ...

    bool isLastSnapshot = CurrentSnapshot->parent().isNull(); // Is there ONE ONLY 'last' snapshot?

    if (discardCurrentSnapshot && !isLastSnapshot) {
        DiscardSnapshotHandler(CurrentSnapshot);
    }

    RevertToCurrentSnapShot(); // inline code to do so, not a function call...

    if (discardCurrentSnapshot && isLastSnapshot) {
        DiscardSnapshotHandler(CurrentSnapshot);
    }

    ...
}

This is how the code above works...

  • 'Revert to Current Snapshot' calls DiscardCurrentStateHandler(false), so that all that happens is:

RevertToCurrentSnapShot();

  • 'Discard Current Snapshot and State', when the current snapshot has NO Parent, in other words there is ONLY one snapshot, does this:

RevertToCurrentSnapShot(); DiscardSnapshotHandler(CurrentSnapshot);

  • 'Discard Current Snapshot and State', when the current snapshot has a Parent, in other words there are two or more snapshots, does this:

DiscardSnapshotHandler(CurrentSnapshot); RevertToCurrentSnapShot();

This last behaviour is broken and unwanted and silly and inefficient.

Using the first bug example given above, which has two snaphots named '1 January' and 'Before 1 April Experiment', the call to DiscardSnapshotHandler(CurrentSnapshot) Discards 'Before 1 April Experiment', merging its changes (the 1st April experiment) with the changes stored in '1 January' (the work January/February/March). '1 January' is now the Current snapshot. Then the call to RevertToCurrentSnapShot() throws away the merged changes stored in '1 January', throwing away the experiment, BUT ALSO throwing away all of the work January/February/March, which is a severe problem. (and also, why bother merging the changes if they are only going to be thrown away?)

'Discard Current Snapshot and State', when there are 2 or more snapshots, should function exactly the same as when there is only one snapshot, and call RevertToCurrentSnapShot() first, and DiscardSnapshotHandler() second. That behaviour is useful and sensible.

3) The behaviour suggested above is useful, whereas the current 'Go back two snapshots' behaviour is mostly useless.

The suggested 'Revert to Last Snapshot then Discard Snapshot' function is useful in situations where the current state must be protected while an experimental configuration or change is done. If the experiment is successful and should be retained, the user uses 'Discard Snapshot'. If the experiment needs several iterations to get right, the user uses 'Revert to Snapshot' and tries again. If the experiment is not successful and needs to be abandoned, the user uses 'Revert to Last Snapshot then Discard Snapshot'.

Change History

comment:1 Changed 5 years ago by MarkCranness

comment:2 Changed 5 years ago by MarkCranness

'Discard Current Snapshot and State' feature is even more broken.

'Discard Current Snapshot and State' sometimes copies the current state into a base hard disk, Confirming/Commiting/Saving the current state, rather than Reverting/Discarding the current state.

1) To see this behaviour, perform the following steps:

  • Create a new empty VDI file 'DiskB', attach it to a VM, partition it, format it and detach it again (or use an available VDI file that is not currently attached to any VM, AND was never attached to any VM at the time any existing snapshot was made - this is important).
  • Choose or create a VM to perform the test. Ensure that the 'DiskB' VDI is not attached.
  • Take a new Snapshot on the VM 'Snapshot 1'.
  • Attach 'DiskB' to the VM, ensuring that there is NO blue cog icon to the left of the disk name (it will be attached as a Base disk and not via a differencing disk).
  • Boot the VM and create a file named 'Change 1.5' on both DiskB and another disk on the VM. (At this point, file 'Change 1.5' is in the base VDI for DiskB, but in the current state differencing disk for the other VM disk.)
  • Shutdown the VM.
  • Take another new Snapshot 'Snapshot 2'.
  • Boot the VM and create a file 'Change 2.5' on both DiskB and the other disk.
  • Shutdown the VM.
  • Select 'Current State' in the tree and mouse hover over 'Discard Current Snapshot and State' button. Note Status Bar explanatory text says: 'Discard the current snapshot and revert the machine to the state it had before the snapshot was taken'
  • Click 'Discard Current Snapshot and State' button. According to the status bar text, this should rollback the 'Change 2.5' change, but leave the 'Change 1.5' file, on both disks.
  • Re-attach DiskB to the VM and boot the VM.
  • Examine the contents of the disks.

You will note that the other VM disk is missing both 'Change 1.5' and 'Change 2.5' files, whereas it should still have 'Change 1.5' (which was the original problem reported at the head of this ticket).

You will also note that DiskB still has 'Change 2.5' on it, as it has been commited/merged into it. DiskB should only have 'Change 1.5' on it. This is a severe error.

(Please change the severity of this ticket.)

2) The cause of this problem is very closely related to the original ticket problem.

The root cause is that SessionMachine::discardCurrentStateHandler should never in any circumstances first Discard the current snapshot and then Revert the current state, as it currently does when there is more than one snaphot. It should always Revert first and Discard afterwards (see argument in original ticket).

The if statement, and its contained code, starting:

if (aTask.discardCurrentSnapshot && !isLastSnapshot)

... (lines 9958-9979) should be deleted.

The code inside the if statement starting:

if (aTask.discardCurrentSnapshot && isLastSnapshot)

... (lines 10079-10126) should always be executed (the 'if' test deleted, leaving the code in place).

(Possibly also some other detail changes: I have not tested these code changes.)

3) Looking at the code, one can see an earlier attempt to fix this problem, focusing around the 'isLastSnapshot' variable (set in line 9942).

That code makes the assumption that if we are discarding the topmost snapshot [having ->parent().isNull()], then the hard disk for this snapshot must be a base hard disk, and therefore do not Discard then Revert, because that will merge the changes into the base hard disk. In fact it is not if the snapshot has a parent that determines if the hard disk is a base hard disk, it is if the hard disk has a parent, and when a snapshot has more than one attached hard disk, one disk might be a base disk and one might be a differencing disk.

comment:3 Changed 5 years ago by sandervl73

  • Status changed from new to closed
  • Resolution set to invalid

Thanks for the report, but we're going to redesign the whole thing as the current implementation is, ehm, not ideal. Therefor it doesn't make any sense to open tickets anymore. Please wait for the next major release.

In the new design there will be two operations:

- delete snapshot
- restore snapshot (any in the tree)

comment:4 Changed 4 years ago by MarkCranness

Re new status 'closed', OK and good (I have since decided that the existing behaviour is *not* broken when considered in a branched tree - I live and learn... another topic).

However, the bug I added in my 2nd comment is an actual live bug: "'Discard Current Snapshot and State' sometimes copies the current state into a base hard disk, Confirming/Commiting/Saving the current state, rather than Reverting/Discarding the current state."

Should I create another ticket for that? (I think so.)

Note: See TracTickets for help on using tickets.

www.oracle.com
ContactPrivacy policyTerms of Use