[PATCH 0/3] free-page-reporting cleanups
In the end, I decided to not change the attribute name to avoid confusion (QEMU uses the name for the feature), but if you want me to we still can do it. Michal Prívozník (3): tests: Turn virtio-options-memballoon-freepage-reporting.xml into a symlink lib: s/free-page-reporting/free_page_reporting/ docs: Clarify free_page_reporting attribute docs/formatdomain.rst | 8 ++++--- docs/schemas/domaincommon.rng | 2 +- src/conf/domain_conf.c | 8 +++---- src/qemu/qemu_validate.c | 2 +- ...-options-memballoon-freepage-reporting.err | 2 +- ...-options-memballoon-freepage-reporting.xml | 2 +- ...-options-memballoon-freepage-reporting.xml | 23 +------------------ 7 files changed, 14 insertions(+), 33 deletions(-) mode change 100644 => 120000 tests/qemuxml2xmloutdata/virtio-options-memballoon-freepage-reporting.xml -- 2.26.2
The output virtio-options-memballoon-freepage-reporting.xml of xml2xmlout is the same as the input. Make it as symlink to save space. Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- ...-options-memballoon-freepage-reporting.xml | 23 +------------------ 1 file changed, 1 insertion(+), 22 deletions(-) mode change 100644 => 120000 tests/qemuxml2xmloutdata/virtio-options-memballoon-freepage-reporting.xml diff --git a/tests/qemuxml2xmloutdata/virtio-options-memballoon-freepage-reporting.xml b/tests/qemuxml2xmloutdata/virtio-options-memballoon-freepage-reporting.xml deleted file mode 100644 index ff7549a789..0000000000 --- a/tests/qemuxml2xmloutdata/virtio-options-memballoon-freepage-reporting.xml +++ /dev/null @@ -1,22 +0,0 @@ -<domain type='qemu'> - <name>QEMUGuest1</name> - <uuid>c7a5fdbd-edaf-9455-926a-d65c16db1809</uuid> - <memory unit='KiB'>219136</memory> - <currentMemory unit='KiB'>219136</currentMemory> - <vcpu placement='static'>1</vcpu> - <os> - <type arch='x86_64' machine='pc'>hvm</type> - <boot dev='hd'/> - </os> - <clock offset='utc'/> - <on_poweroff>destroy</on_poweroff> - <on_reboot>restart</on_reboot> - <on_crash>destroy</on_crash> - <devices> - <emulator>/usr/bin/qemu-system-x86_64</emulator> - <controller type='pci' index='0' model='pci-root'/> - <memballoon model='virtio' free-page-reporting='on'> - <address type='pci' domain='0x0000' bus='0x00' slot='0x02' function='0x0'/> - </memballoon> - </devices> -</domain> diff --git a/tests/qemuxml2xmloutdata/virtio-options-memballoon-freepage-reporting.xml b/tests/qemuxml2xmloutdata/virtio-options-memballoon-freepage-reporting.xml new file mode 120000 index 0000000000..e09d9d7283 --- /dev/null +++ b/tests/qemuxml2xmloutdata/virtio-options-memballoon-freepage-reporting.xml @@ -0,0 +1 @@ +../qemuxml2argvdata/virtio-options-memballoon-freepage-reporting.xml \ No newline at end of file -- 2.26.2
In fee8a61d29 a new attribute to <memballoon/> was introduced: free-page-reporting. We don't really like hyphens in attribute names. Use underscores instead. Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- docs/formatdomain.rst | 4 ++-- docs/schemas/domaincommon.rng | 2 +- src/conf/domain_conf.c | 8 ++++---- src/qemu/qemu_validate.c | 2 +- .../virtio-options-memballoon-freepage-reporting.err | 2 +- .../virtio-options-memballoon-freepage-reporting.xml | 2 +- 6 files changed, 10 insertions(+), 10 deletions(-) diff --git a/docs/formatdomain.rst b/docs/formatdomain.rst index 83dec62f30..5ec765ad3d 100644 --- a/docs/formatdomain.rst +++ b/docs/formatdomain.rst @@ -6755,8 +6755,8 @@ Example: manually added device with static PCI slot 2 requested release some memory at the last moment before a guest's process get killed by Out of Memory killer. :since:`Since 1.3.1, QEMU and KVM only` -``free-page-reporting`` - The optional ``free-page-reporting`` attribute allows to enable/disable +``free_page_reporting`` + The optional ``free_page_reporting`` attribute allows to enable/disable ("on"/"off", respectively) the ability of the QEMU virtio memory balloon to return unused pages back to the hypervisor to be used by other guests or processes. :since:`Since 6.9.0, QEMU and KVM only` diff --git a/docs/schemas/domaincommon.rng b/docs/schemas/domaincommon.rng index 0a0f0ed8a8..1f37e0744d 100644 --- a/docs/schemas/domaincommon.rng +++ b/docs/schemas/domaincommon.rng @@ -4540,7 +4540,7 @@ </attribute> </optional> <optional> - <attribute name="free-page-reporting"> + <attribute name="free_page_reporting"> <ref name="virOnOff"/> </attribute> </optional> diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 1d3661c21f..006e5a170b 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -15361,10 +15361,10 @@ virDomainMemballoonDefParseXML(virDomainXMLOptionPtr xmlopt, goto error; } - if ((freepage_reporting = virXMLPropString(node, "free-page-reporting")) && + if ((freepage_reporting = virXMLPropString(node, "free_page_reporting")) && (def->free_page_reporting = virTristateSwitchTypeFromString(freepage_reporting)) <= 0) { virReportError(VIR_ERR_CONFIG_UNSUPPORTED, - _("invalid free-page-reporting attribute value '%s'"), freepage_reporting); + _("invalid free_page_reporting attribute value '%s'"), freepage_reporting); goto error; } @@ -23548,7 +23548,7 @@ virDomainMemballoonDefCheckABIStability(virDomainMemballoonDefPtr src, if (src->free_page_reporting != dst->free_page_reporting) { virReportError(VIR_ERR_CONFIG_UNSUPPORTED, - _("Target balloon free-page-reporting attribute value " + _("Target balloon free_page_reporting attribute value " "'%s' does not match source '%s'"), virTristateSwitchTypeToString(dst->free_page_reporting), virTristateSwitchTypeToString(src->free_page_reporting)); @@ -27662,7 +27662,7 @@ virDomainMemballoonDefFormat(virBufferPtr buf, virTristateSwitchTypeToString(def->autodeflate)); if (def->free_page_reporting != VIR_TRISTATE_SWITCH_ABSENT) - virBufferAsprintf(&attrBuf, " free-page-reporting='%s'", + virBufferAsprintf(&attrBuf, " free_page_reporting='%s'", virTristateSwitchTypeToString(def->free_page_reporting)); if (def->period) diff --git a/src/qemu/qemu_validate.c b/src/qemu/qemu_validate.c index bc3043bb3f..3ea8249977 100644 --- a/src/qemu/qemu_validate.c +++ b/src/qemu/qemu_validate.c @@ -3934,7 +3934,7 @@ qemuValidateDomainDeviceDefMemballoon(const virDomainMemballoonDef *memballoon, if (memballoon->free_page_reporting != VIR_TRISTATE_SWITCH_ABSENT && !virQEMUCapsGet(qemuCaps, QEMU_CAPS_VIRTIO_BALLOON_FREE_PAGE_REPORTING)) { virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", - _("free-page-reporting is not supported by this QEMU binary")); + _("free_page_reporting is not supported by this QEMU binary")); return -1; } diff --git a/tests/qemuxml2argvdata/virtio-options-memballoon-freepage-reporting.err b/tests/qemuxml2argvdata/virtio-options-memballoon-freepage-reporting.err index de06de0a85..8cf7e0bf4a 100644 --- a/tests/qemuxml2argvdata/virtio-options-memballoon-freepage-reporting.err +++ b/tests/qemuxml2argvdata/virtio-options-memballoon-freepage-reporting.err @@ -1 +1 @@ -unsupported configuration: free-page-reporting is not supported by this QEMU binary +unsupported configuration: free_page_reporting is not supported by this QEMU binary diff --git a/tests/qemuxml2argvdata/virtio-options-memballoon-freepage-reporting.xml b/tests/qemuxml2argvdata/virtio-options-memballoon-freepage-reporting.xml index ff7549a789..04f4ce7b75 100644 --- a/tests/qemuxml2argvdata/virtio-options-memballoon-freepage-reporting.xml +++ b/tests/qemuxml2argvdata/virtio-options-memballoon-freepage-reporting.xml @@ -15,7 +15,7 @@ <devices> <emulator>/usr/bin/qemu-system-x86_64</emulator> <controller type='pci' index='0' model='pci-root'/> - <memballoon model='virtio' free-page-reporting='on'> + <memballoon model='virtio' free_page_reporting='on'> <address type='pci' domain='0x0000' bus='0x00' slot='0x02' function='0x0'/> </memballoon> </devices> -- 2.26.2
On Wed, Oct 14, 2020 at 05:29:33PM +0200, Michal Privoznik wrote:
In fee8a61d29 a new attribute to <memballoon/> was introduced: free-page-reporting. We don't really like hyphens in attribute names. Use underscores instead.
Disclaimer: I just noticed this by accident, I'm sure I've missed several other cases in the past half a year at least, nevertheless, I'll comment here. Back when we added support for SEV I remember being asked to rename the underscored name to a camel case one as apparently at that time we were trying to standardize on it and so I did change it. Back to the present, we're still not consistent with this, so I'd like to propose we either let this in or we switch to camelcase and update the code-style page with a new paragraph under the naming convention dealing with XML attributes regardless. Regards, Erik
On 10/15/20 8:03 AM, Erik Skultety wrote:
On Wed, Oct 14, 2020 at 05:29:33PM +0200, Michal Privoznik wrote:
In fee8a61d29 a new attribute to <memballoon/> was introduced: free-page-reporting. We don't really like hyphens in attribute names. Use underscores instead.
Disclaimer: I just noticed this by accident, I'm sure I've missed several other cases in the past half a year at least, nevertheless, I'll comment here.
Back when we added support for SEV I remember being asked to rename the underscored name to a camel case one as apparently at that time we were trying to standardize on it and so I did change it. Back to the present, we're still not consistent with this, so I'd like to propose we either let this in or we switch to camelcase and update the code-style page with a new paragraph under the naming convention dealing with XML attributes regardless.
Fair enough. Dan, I hope your Reviewed-by stands if I do the change to camelCase locally without sending v2? I'll post the code-style change as a separate patch. Michal
On Thu, Oct 15, 2020 at 10:36:48AM +0200, Michal Privoznik wrote:
On 10/15/20 8:03 AM, Erik Skultety wrote:
On Wed, Oct 14, 2020 at 05:29:33PM +0200, Michal Privoznik wrote:
In fee8a61d29 a new attribute to <memballoon/> was introduced: free-page-reporting. We don't really like hyphens in attribute names. Use underscores instead.
Disclaimer: I just noticed this by accident, I'm sure I've missed several other cases in the past half a year at least, nevertheless, I'll comment here.
Back when we added support for SEV I remember being asked to rename the underscored name to a camel case one as apparently at that time we were trying to standardize on it and so I did change it. Back to the present, we're still not consistent with this, so I'd like to propose we either let this in or we switch to camelcase and update the code-style page with a new paragraph under the naming convention dealing with XML attributes regardless.
Fair enough. Dan, I hope your Reviewed-by stands if I do the change to camelCase locally without sending v2?
Anything except hyphens :-) Regards, Daniel -- |: https://berrange.com -o- https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o- https://fstop138.berrange.com :| |: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|
The 'reporting' suffix of the attribute makes it sound like we could be reporting something to user. While in fact, this is purely virtio membaloon <-> QEMU business. Clarify the docs to make it clear. Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- docs/formatdomain.rst | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/docs/formatdomain.rst b/docs/formatdomain.rst index 5ec765ad3d..7abcb9d773 100644 --- a/docs/formatdomain.rst +++ b/docs/formatdomain.rst @@ -6759,7 +6759,9 @@ Example: manually added device with static PCI slot 2 requested The optional ``free_page_reporting`` attribute allows to enable/disable ("on"/"off", respectively) the ability of the QEMU virtio memory balloon to return unused pages back to the hypervisor to be used by other guests or - processes. :since:`Since 6.9.0, QEMU and KVM only` + processes. Please note that despite its name it has no effect on free memory + as reported by ``virDomainMemoryStats()`` and/or ``virsh dommemstat``. + :since:`Since 6.9.0, QEMU and KVM only` ``period`` The optional ``period`` allows the QEMU virtio memory balloon driver to -- 2.26.2
On Wed, Oct 14, 2020 at 05:29:31PM +0200, Michal Privoznik wrote:
In the end, I decided to not change the attribute name to avoid confusion (QEMU uses the name for the feature), but if you want me to we still can do it.
Michal Prívozník (3): tests: Turn virtio-options-memballoon-freepage-reporting.xml into a symlink lib: s/free-page-reporting/free_page_reporting/ docs: Clarify free_page_reporting attribute
Reviewed-by: Daniel P. Berrangé <berrange@redhat.com> Regards, Daniel -- |: https://berrange.com -o- https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o- https://fstop138.berrange.com :| |: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|
participants (3)
-
Daniel P. Berrangé -
Erik Skultety -
Michal Privoznik