GNOME Bugzilla – Bug 694430
Fix KiB/B confusion in box properties
Last modified: 2016-03-31 13:22:07 UTC
When going to a box properties, the amount of RAM in the VM is currently wrong by a factor of 1024 as it's in KiB and not bytes.
Created attachment 237163 [details] [review] Add memory units to diagnostics log The troubleshooting log that can be obtained through a box properties shows the current/max memory for the box. As this amount is in KiB, this commit adds a unit to this log to make this explicit.
Created attachment 237164 [details] [review] Use NodeInfo::memory as max memory in diagnostics log This is what add_ram_property() uses as the maximum for the memory slider, so it's better to log that value rather than the unused info.maxMem
Created attachment 237165 [details] [review] libvirt: Fix current memory size display in properties The memory size returned by LibvirtGConfig is in KiB.
Review of attachment 237165 [details] [review]: Good catch, ACK
Review of attachment 237163 [details] [review]: ::: src/libvirt-machine-properties.vala @@ +65,3 @@ var info = machine.domain.get_info (); builder.append_printf ("Cpu time: %"+uint64.FORMAT_MODIFIER+"d\n", info.cpuTime); + builder.append_printf ("Memory: %"+uint64.FORMAT_MODIFIER+"d KiB\n", info.memory); I think we should use the GLib.format_size in here too.
Review of attachment 237164 [details] [review]: ACK
Review of attachment 237163 [details] [review]: ::: src/libvirt-machine-properties.vala @@ +65,3 @@ var info = machine.domain.get_info (); builder.append_printf ("Cpu time: %"+uint64.FORMAT_MODIFIER+"d\n", info.cpuTime); + builder.append_printf ("Memory: %"+uint64.FORMAT_MODIFIER+"d KiB\n", info.memory); As this is debugging/troubleshooting output, displaying the raw value rather than rounding it is better imo.
Review of attachment 237163 [details] [review]: ack then ::: src/libvirt-machine-properties.vala @@ +65,3 @@ var info = machine.domain.get_info (); builder.append_printf ("Cpu time: %"+uint64.FORMAT_MODIFIER+"d\n", info.cpuTime); + builder.append_printf ("Memory: %"+uint64.FORMAT_MODIFIER+"d KiB\n", info.memory); good point.
Attachment 237163 [details] pushed as 35cf6c0 - Add memory units to diagnostics log Attachment 237164 [details] pushed as 26ce4c2 - Use NodeInfo::memory as max memory in diagnostics log Attachment 237165 [details] pushed as c7e63f3 - libvirt: Fix current memory size display in properties
Created attachment 238990 [details] [review] libvirt-machine-props: Set RAM size in correct units The RAM size we get from property widget is in bytes where as default units used by libvirt config are in KiB. So we must convert before setting.
Still some confusion left. :) Check attached patch.
*** Bug 695986 has been marked as a duplicate of this bug. ***
Review of attachment 238990 [details] [review]: ::: src/libvirt-machine-properties.vala @@ +431,3 @@ // Ensure that we don't end-up changing RAM like a 1000 times a second while user moves the slider.. property.deferred_change = () => { + var ram = value / Osinfo.KIBIBYTES; Do we want to make sure this is rounded up if 'value' is not a multiple of 1024? This would make it var ram = (value + Osinfo.KIBIBYTES - 1) / Osinfo.KIBIBYTES; @@ +439,2 @@ machine.domain.set_config (config); + debug ("RAM changed to %llu", ram); Please add a unit here... @@ +447,1 @@ error.message); ... and to this message as well
Review of attachment 238990 [details] [review]: ::: src/libvirt-machine-properties.vala @@ +431,3 @@ // Ensure that we don't end-up changing RAM like a 1000 times a second while user moves the slider.. property.deferred_change = () => { + var ram = value / Osinfo.KIBIBYTES; Probably a good idea but I'd put that in a separate patch.
Review of attachment 238990 [details] [review]: ::: src/libvirt-machine-properties.vala @@ +431,3 @@ // Ensure that we don't end-up changing RAM like a 1000 times a second while user moves the slider.. property.deferred_change = () => { + var ram = value / Osinfo.KIBIBYTES; Why? This patch is about converting bytes to kib, why do the conversion one way, and then have an additional patch to change how we do the conversion?
Created attachment 239130 [details] [review] libvirt-machine-props: Set RAM size in correct units The RAM size we get from property widget is in bytes where as default units used by libvirt config are in KiB. So we must convert before setting.
Review of attachment 239130 [details] [review]: Failed to publish review: Undefined subroutine &extensions::splinter::lib::WSSplinter::ThrowCodeError called at /var/www/bugzilla.gnome.org/extensions/splinter/lib/WSSplinter.pm line 88.
Attachment 239130 [details] pushed as d30f242 - libvirt-machine-props: Set RAM size in correct units