Vboxwrapper - handling of outdated vboxmanage command options

[computezrmle]

Certain Vbox tasks from LHC@home recently failed with log entries like this:

2024-04-13 16:06:31 (7532): Detected: VirtualBox VboxManage Interface (Version: Unknown)
.
.
.
2024-04-13 16:06:43 (7532): Error in add storage controller (fixed disk) for VM: -108
Command:
VBoxManage -q storagectl "boinc_2f92bf95dc688693" --name "Hard Disk Controller" --add "sata" --controller "IntelAHCI" --hostiocache off --sataportcount 3
Output:
Oracle VM VirtualBox Command Line Management Interface Version 7.0.8
Copyright (C) 2005-2023 Oracle and/or its affiliates

VBoxManage.exe: error: Unknown option: --sataportcount
Usage - Manage a storage controller:

  VBoxManage storagectl <uuid | vmname> <--name=controller-name> [--add= floppy
      | ide | pcie | sas | sata | scsi | usb ] [--controller= BusLogic | I82078
      | ICH6 | IntelAhci | LSILogic | LSILogicSAS | NVMe | PIIX3 | PIIX4 | USB
      | VirtIO ] [--bootable= on | off ] [--hostiocache= on | off ]
      [--portcount=count] [--remove] [--rename=new-controller-name]

2024-04-13 16:06:43 (7532): Could not create VM
2024-04-13 16:06:43 (7532): ERROR: VM failed to start

The root cause is that (for an unknown reason) the VirtualBox version could not be detected.
Vboxwrapper then used --sataportcount which is set since is_virtual_box_version_newer() returns false as default.
The log also shows that the VirtualBox version used on this computer is 7.0.8.

Suggestion
To give a task a chance to run is_virtual_box_version_newer() should return true as default which would set recent vboxmanage command options as default.

[AenBleidd]

I don't like this solution simply because this breaks the whole function logic.
Instead I'd investigate more why it was not possible to detect the VBox version.
Otherwise this will break the logic, and would make vboxwrapper to act wrong in the case when it works perfectly fine right now.
I'd suggest to add additional logging to the is_virtualbox_version_newer function to show the input string, to identify more accurately, why it was not possible to detect the correct VBox version

[computezrmle]

As mentioned, the reason why the check fails is unknown.
The log is not from my own computer and the computer in question does not always fail the version check.
Could be a broken VirtualBox installation, but that's without evidence.

Below please find a complete list where is_virtual_box_version_newer() is called.
The first location is the only critical one as it causes the task to fail, but it wouldn't do any harm to give the task a chance.

All other locations also cause no harm if true is returned, especially since all checks (but 1) are against very old VirtualBox versions that are not in use any more for years.

As for more logging:
Agreed, but it may be done in get_version_information() if the version details can't be identified.

boinc/samples/vboxwrapper/vbox_vboxmanage.cpp

Lines 452 to 458 in 627637d

	if ((vm_disk_controller_type == "sata") || (vm_disk_controller_type == "SATA")) {
	if (is_virtualbox_version_newer(4, 3, 0)) {
	command += "--portcount 3";
	} else {
	command += "--sataportcount 3";
	}
	}

boinc/samples/vboxwrapper/vbox_vboxmanage.cpp

Lines 352 to 360 in 627637d

	// Tweak the VM's Audio Support
	//
	vboxlog_msg("Disabling Audio Support for VM.");
	command = "modifyvm \"" + vm_name + "\" ";
	if (is_virtualbox_version_newer(7, 0, 4)) {
	command += "--audio-enabled off ";
	} else {
	command += "--audio none ";
	}

boinc/samples/vboxwrapper/vbox_vboxmanage.cpp

Lines 735 to 738 in 627637d

	// Add network bandwidth throttle group
	//
	if (is_virtualbox_version_newer(4, 2, 0)) {
	vboxlog_msg("Adding network bandwidth throttle group to VM. (Defaulting to 1024GB)");

boinc/samples/vboxwrapper/vbox_vboxmanage.cpp

Lines 874 to 877 in 627637d

	// Delete network bandwidth throttle group
	//
	if (is_virtualbox_version_newer(4, 2, 0)) {
	vboxlog_msg("Removing network bandwidth throttle group from VM.");

boinc/samples/vboxwrapper/vbox_vboxmanage.cpp

Lines 1456 to 1458 in 627637d

	int VBOX_VM::capture_screenshot() {
	if (enable_screenshots_on_error) {
	if (is_virtualbox_version_newer(5, 0, 0)) {

boinc/samples/vboxwrapper/vbox_vboxmanage.cpp

Lines 2140 to 2143 in 627637d

	if (is_virtualbox_version_newer(4, 2, 0)) {
	// Update bandwidth group limits
	//

[AenBleidd]

Even if now there is no harm to do the change you proposed, that doesn't mean that this won't make an issue in future.
I suggest to do the next:

1. Extend logging (get_version_information() indeed is the best place for that)
2. Release a new version of vboxwrapper and wait for more details about this issue.
   I prefer to fix the root cause of the issue, and not just patch the aftermaths.
   Thank you for understanding.

[computezrmle]

As for (1.):
done

As for (2.):
It does harm now since it forces a task to fail without any reason to do so.
I understand this as a logical error in the current vboxwrapper that needs to be corrected.

[davidpanderson]

My feeling is: we should try to figure out why version detection fails.

But if it does fail, we should assume it's the current version.