RamSize wrong value in VBox.log

[Socratis]

Create a VM. Any will do. Set the memory to a non nice, binary, "round" number, for example 3043 MB (I just saw it on a test case, don't blame me). Here's what VBox.log has to say about it:

RamSize <integer> = 0x00000000be300000 (3 190 816 768, 2 GB)

Although the RAM is set to 2.97 GB, it is identified as 2 GB. Yes, it is correctly identified to the byte, but it is not correctly identified in the "human readable" version. Some sort of lower(), int() conversion? This should be trivial to fix.

And since we're at it, it would make sense to report that value in MB, the same that it is reported everywhere else, like the total host memory or the available host memory.

BTW, I left the VBox version as empty, because this is way older behavior than the current 5.1.6

[Frank Mehnert]

Sorry, this human readable form is only there to make it better readable and to provide an estimate. We will not fix this.

[Socratis]

"provide an estimate"? That rough estimate is about 50% off. And you know how many times moderators have been bitten by this piece of information? Come on frank, this should be really trivial to fix. Instead of GB, report it in MB, that's it. And it will prevent many, many errors in the forums.

If you insist on not fixing it yourselves, would an MIT licensed patch do it? I'm not a C person, but I'm really confident I can track this down and fix it within an hour (of free time that is ;) ).

At least leave the ticket open for now?

[Frank Mehnert]

As long as we don't use point values the deviation can be very big of course. The runtime library does not contain support for floating point values. Yes, I know how to do the arithmetics but still, VBox.log is a debugging aid and nothing more. Please, there are more than 2,000 tickets open in this bugtracker, please don't open such tickets.

I'm not saying that we will not fix this eventually. But please, no ticket for requesting changes in VBox.log.

[mpack]

I'm afraid I have to agree with Socratis. In the example given above the reported information is plain wrong. The amount of memory allocated is not 2GB, it's 2.97GB, or near as anything 3GB. It's also inconvenient that it reports the number in different units than available RAM, so I'd agree that the easiest fix is to report both in MB. Finally, to write the log off as an unimportant convenience is IMHO a mistake: the user overcommitting host RAM - or getting close to doing so - is one of the most common errors users make. Helping the volunteers spot this as easily as possible seems to me like a very good move, rather then have them raising unnecessary tickets for the mistakes we don't spot.

[Socratis]

Replying to mpack:

I'm afraid I have to agree with Socratis.

Come on Don, it's not that difficult to agree with me... :D

But, I got the go ahead from Frank (in a PM) and I'm going to try and discover the problem myself. Of course Frank also gave me the exact function name to look for, which feels like cheating, but I'd be more than happy to hunt it down and submit a patch.

Stay tuned...

[Socratis]

OK, got it! I'm not sure how to submit a patch that contains the +++ and --- lines (the proper way), but I guess you can figure it out ;). If I do (which I plan to) I will post again, just for completion.

As Frank pointed out to me, the problem is in "VMM/VMMR3/CFGM.cpp", function "cfgmR3Dump()". Change the following lines, from:

if (pLeaf->Value.Integer.u64 > _2G)
	pHlp->pfnPrintf(pHlp, ", %'lld GB", pLeaf->Value.Integer.u64 / _1G);
else if (pLeaf->Value.Integer.u64 > _2M)
	pHlp->pfnPrintf(pHlp, ", %'lld MB", pLeaf->Value.Integer.u64 / _1M);
else if (pLeaf->Value.Integer.u64 > _2K)
	pHlp->pfnPrintf(pHlp, ", %'lld KB", pLeaf->Value.Integer.u64 / _1K);

to

if (pLeaf->Value.Integer.u64 > _2M)
	pHlp->pfnPrintf(pHlp, ", %'lld MB", pLeaf->Value.Integer.u64 / _1M);
else if (pLeaf->Value.Integer.u64 > _2K)
	pHlp->pfnPrintf(pHlp, ", %'lld KB", pLeaf->Value.Integer.u64 / _1K);

Pretty much delete two lines. That way the values reported are always in MB, as both me and mpack thought it would be more appropriate. Which BTW is what is reported in the "VBoxManage showvminfo".

Funny side effect; I never realized that you can assign less than 1 MB to a VM.

Oh, BTW, the patch is submitted under the MIT license.

[Socratis]

Man, I thought that a simple 'diff' would do it. That's what happens when you don't know how to use a tool. Finally, I think got it. So comparison between the 5.1.6 "CFGM.cpp" and my modification yields:

diff -U 3 /Users/Shared/VirtualBox/Source/VirtualBox-5.1.6/src/VBox/VMM/VMMR3/CFGM.cpp /Users/Shared/VirtualBox/Source/Socratis/src/VBox/VMM/VMMR3/CFGM.cpp 
--- /Users/Shared/VirtualBox/Source/VirtualBox-5.1.6/src/VBox/VMM/VMMR3/CFGM.cpp	2016-09-12 19:19:20.000000000 +0300
+++ /Users/Shared/VirtualBox/Source/Socratis/src/VBox/VMM/VMMR3/CFGM.cpp	2016-09-28 20:35:48.000000000 +0300
@@ -3243,9 +3243,7 @@
                     || (   pLeaf->cchName >= 2
                         && !RTStrNCmp(pLeaf->szName, "cb", 2)) )
                 {
-                    if (pLeaf->Value.Integer.u64 > _2G)
-                        pHlp->pfnPrintf(pHlp, ", %'lld GB", pLeaf->Value.Integer.u64 / _1G);
-                    else if (pLeaf->Value.Integer.u64 > _2M)
+                    if (pLeaf->Value.Integer.u64 > _2M)
                         pHlp->pfnPrintf(pHlp, ", %'lld MB", pLeaf->Value.Integer.u64 / _1M);
                     else if (pLeaf->Value.Integer.u64 > _2K)
                         pHlp->pfnPrintf(pHlp, ", %'lld KB", pLeaf->Value.Integer.u64 / _1K);

[mpack]

I don't believe MIT license is appropriate for this patch. I believe the main constraint on MIT is that they have to maintain your copyright notice, which (a) you don't have, and (b) would be ridiculous for a patch which mainly involves deleting code. It should just be given gratis.

[Socratis]

Oh, I don't mind giving it as anything you like. But it's Oracle actually that sets the rules. Either I give the "patch"<1> under the MIT license, or I have to go through a gazillion of paperwork. Otherwise it doesn't get accepted. That's what Oracle says: see the Allowing Oracle to incorporate your contributions part. That's actually how all the translations are handled, I know it first hand:

The <MyLanguage> (<my_MY>) translation attached here by <my_full_name>
is published under the terms of the [wiki:"MIT license"].

Since I got your attention, the "patch" looks correct, right? I'm not a C person, but I think I deleted the correct part...

<1>: Yeah, right, select two lines, press Delete. An hour! Pfft... I want my name in bold in the contributors list! :D

[mpack]

As a C person, but not a team developer, I wouldn't have bothered with the formal patch process. I'd just have pm'd the changes to a devteam member, and let them make the change under their own name. :)

[Frank Mehnert]

Actually I prefer the solution from r64103.

[Socratis]

I appreciate the quick fix, but that's outputing it in decimal GB, which is not consistent with what the rest of the VBox.log is reporting. Why do you insist on having my brain cells exercise? :)

IMHO, it would have been simpler and more straight forward to convert it to MB, like everywhere else.

[Frank Mehnert]

I prefer GB values for bigger RAM sizes :-)

[Socratis]

Sure. But make it consistent throughout the log. Example from ​https://forums.virtualbox.org/viewtopic.php?f=3&t=79953 which is the origin of this ticket:

Original behavior, as it stands before r64103:

00:00:02.990006 Host RAM: 5090MB total, 2592MB available
00:00:04.832083 RamSize <integer> = 0x00000000be300000 (3 190 816 768, 2 GB)

With your patch (r64103):

00:00:02.990006 Host RAM: 5090MB total, 2592MB available
00:00:04.832083 RamSize <integer> = 0x00000000be300000 (3 190 816 768, 2.97 GB)

With the originally proposed solution and patch:

00:00:02.990006 Host RAM: 5090MB total, 2592MB available
00:00:04.832083 RamSize <integer> = 0x00000000be300000 (3 190 816 768, 3043 MB)

Do you see how much easier it becomes to compare host available and guest allocated RAM with one glance?

I don't want to drag this thing on forever. You have more important things to work on. I sincerely hope that you'll change your mind on this, but that the last comment I'm going to make on this issue. I'll simply use my calculator to make my life easier spotting users' memory allocation problems.

[Frank Mehnert]

Please check r64110, r64111. Before you ask: No, I'm aware about the different spacing between host ram total/available and the other values but I don't intend to change that. But

00:00:00.731994 Host RAM: 15918MB (15.5GB) total, 13773MB (13.4GB) available

and

00:00:00.861328 RamHoleSize <integer> = 0x0000000020000000 (536 870 912, 512 MB)
00:00:00.861330 RamSize <integer> = 0x00000000a0000000 (2 684 354 560, 2 560 MB, 2.5 GB)

should satisfy you, at least I hope so :)

[Socratis]

Replying to frank:

should satisfy you, at least I hope so :)

Beyond my wildest dreams !!!

THANK YOU!!!

[Socratis]

I just tried VBox Version 5.1.7 rev111203 (Qt5.5.1) and it seems that it's (almost) fixed. The RAM is reported in MB, as originally envisioned (THANK YOU), but something is not right with the math for the GB. Somehow the decimals are missing and the integer is rounded down:

00:00:00.943054 RamSize <integer> = 0x00000001f8000000 (8 455 716 864, 8 064 MB, 7 GB)

It's closer to the intended numbers for the Host RAM, but still a little bit off; 9.2 vs 9.30 (one digit instead of two, and this is floored):

00:00:00.874776 Host RAM: 16384MB (16.0GB) total, 9521MB (9.2GB) available

BTW, I reopened it so that you can close it properly, as "Fixed" ;)

[Frank Mehnert]

Now closing as "fixed", see VBox 5.1.8 :-)