[PATCH 00/55] Hyper-V: code cleanup & prep for future changes

[Laine Stump]

(Thought I sent this 7 hours ago before I went to sleep, but when I sat 
down this morning I saw it was still sitting there as a draft :-/)

On 1/21/21 1:50 PM, Matt Coleman wrote:
> This series of patches simplifies the code in several ways and makes a
> few changes required by the next round of patches that I'll submit.
>
> Simplifications:
>
> * add a macro to cut down on repetitive SettingData code
> * enable GLib auto-cleanup for hypervObject and several OpenWSMAN types
>
> Changes:
>
> * store the version in hypervPrivate, which will be used to handle
>    breaking changes in the Hyper-V API: despite 2012R2 and 2016+ all
>    using Hyper-V's "V2" API, backwards-incompatible changes were made in
>    2016
> * add inheritance to the WMI generator to simplify handling of the
>    backwards-incompatible changes introduced in Hyper-V 2016


I've gone through all of these, and just have two questions that affect 
multiple patches each (I've replied to the associated patches):


1) There are several cleanup functions in external libraries that in the 
past were only called after checking that the pointer was != NULL. 
g_autoptr cleanups need to handle being called with NULL as a NOP, and 
I'm concerned that these functions may not behave properly in that case. 
Can you either verify that it's safe to call them with NULL, or provide 
a wrapper function that checks for NULL and use that as the cleanup?


2) There are several places where you're returning a value that is 
retrieved from an object that is being auto-freed, and I don't know 
enough about the details of the teardown code that's generated by the 
compiler to be certain that the return value would be retrieved from the 
object *before* it's freed rather than *after*. If someone knows the 
answer to this, then that's great, otherwise someone should compile a 
test program and list out the assembly code using gdb to see what the 
order is.


Once those two questions are resolved (possibly requiring no change to 
your patches), then


  Reviewed-by: Laine Stump <laine at redhat.com>


>
> Matt Coleman (55):
>    hyperv: add a macro for retrieving setting data
>    hyperv: store the Hyper-V version when connecting
>    hyperv: add inheritance to the WMI generator
>    hyperv: store hypervPrivate in hypervObject
>    hyperv: enable use of g_autoptr for hypervObject
>    hyperv: enable use of g_autoptr for the rest of the CIM/WMI classes
>    hyperv: enable automatic cleanup for OpenWSMAN types
>    hyperv: use g_autoptr for Win32_OperatingSystem in hypervConnectOpen
>    hyperv: use g_autoptr for Win32_ComputerSystem in
>      hypervConnectGetHostname
>    hyperv: use g_autoptr for Msvm_ProcessorSettingData in
>      hypervConnectGetMaxVcpus
>    hyperv: use g_autoptr for WMI classes in hypervNodeGetInfo
>    hyperv: use g_autoptr for Msvm_ComputerSystem in
>      hypervConnectNumOfDomains
>    hyperv: use g_autoptr for Msvm_ComputerSystem in
>      hypervConnectListDomains
>    hyperv: use g_autoptr for Msvm_ComputerSystem in
>      hypervDomainLookupByID
>    hyperv: use g_autoptr for Msvm_ComputerSystem in
>      hypervDomainLookupByUUID
>    hyperv: use g_autoptr for Msvm_ComputerSystem in
>      hypervDomainLookupByName
>    hyperv: use g_autoptr for Msvm_ComputerSystem in hypervDomainResume
>    hyperv: use g_autoptr for WMI classes in hypervDomainShutdownFlags
>    hyperv: use g_autoptr for Msvm_ComputerSystem in
>      hypervDomainDestroyFlags
>    hyperv: use g_autoptr for WMI classes in hypervDomainGetMaxMemory
>    hyperv: use g_autoptr for WMI classes in hypervDomainSetMemoryProperty
>    hyperv: use g_autoptr for Msvm_ComputerSystem in
>      hypervRequestStateChange
>    hyperv: use g_autoptr for Win32_ComputerSystemProduct in
>      hypervLookupHostSystemBiosUuid
>    hyperv: use g_autoptr for Msvm_ResourceAllocationSettingData in
>      hypervDomainAttachPhysicalDisk
>    hyperv: use g_autoptr for WMI classes in hypervDomainAttachStorage
>    hyperv: use g_autoptr for Msvm_DiskDrive in
>      hypervDomainDefParsePhysicalDisk
>    hyperv: use g_autoptr for WMI classes in hypervDomainGetInfo
>    hyperv: use g_autoptr for Msvm_ComputerSystem in hypervDomainGetState
>    hyperv: use g_autoptr for WMI classes in hypervDomainSetVcpusFlags
>    hyperv: use g_autoptr for WMI classes in hypervDomainGetVcpusFlags
>    hyperv: use g_autoptr for Msvm_ComputerSystem in
>      hypervConnectListDefinedDomains
>    hyperv: use g_autoptr for Msvm_ComputerSystem in
>      hypervConnectNumOfDefinedDomains
>    hyperv: use g_autoptr for Msvm_ComputerSystem in
>      hypervDomainCreateWithFlags
>    hyperv: use g_autoptr for Msvm_VirtualSystemSettingData in
>      hypervDomainGetAutostart
>    hyperv: use g_autoptr for Msvm_VirtualSystemSettingData in
>      hypervDomainSetAutostart
>    hyperv: use g_autoptr for WMI classes in
>      hypervDomainGetSchedulerParametersFlags
>    hyperv: use g_autoptr for Msvm_ComputerSystem in hypervDomainIsActive
>    hyperv: use g_autoptr for Msvm_ComputerSystem in
>      hypervDomainManagedSave
>    hyperv: use g_autoptr for Msvm_ComputerSystem in
>      hypervDomainHasManagedSaveImage
>    hyperv: use g_autoptr for Msvm_ComputerSystem in
>      hypervDomainManagedSaveRemove
>    hyperv: use g_autoptr for Msvm_ComputerSystem in
>      hypervConnectListAllDomains
>    hyperv: use GLib auto-cleanup in hypervDomainSendKey
>    hyperv: use GLib auto-cleanup in hypervInvokeMethod
>    hyperv: use GLib auto-cleanup in
>      hypervInvokeMsvmComputerSystemRequestStateChange
>    hyperv: use GLib auto-cleanup in hypervMsvmVSMSAddResourceSettings and
>      hypervMsvmVSMSModifyResourceSettings
>    hyperv: use g_autoptr for
>      Win32_PerfRawData_HvStats_HyperVHypervisorVirtualProcessor in
>      hypervDomainGetVcpus
>    hyperv: use g_autoptr for Win32_OperatingSystem in
>      hypervNodeGetFreeMemory
>    hyperv: use GLib auto-cleanup in hypervDomainGetXMLDesc
>    hyperv: use g_autoptr for WMI classes in hypervDomainAttachDeviceFlags
>    hyperv: use GLib auto-cleanup in hypervSerializeEprParam
>    hyperv: use GLib auto-cleanup in hypervEnumAndPull
>    hyperv: use GLib auto-cleanup in hypervSerializeEmbeddedParam
>    hyperv: use GLib auto-cleanup in hypervCreateInvokeXmlDoc
>    hyperv: use g_auto for WsXmlDocH in hypervDomainAttachVirtualDisk
>    hyperv: use g_auto for WsXmlDocH in hypervDomainAttachCDROM
>
>   scripts/hyperv_wmi_generator.py |  16 +-
>   src/hyperv/hyperv_driver.c      | 755 +++++++++++---------------------
>   src/hyperv/hyperv_private.h     |   4 +-
>   src/hyperv/hyperv_wmi.c         | 408 +++++++----------
>   src/hyperv/hyperv_wmi.h         |   4 +-
>   src/hyperv/hyperv_wsman.h       |  28 ++
>   6 files changed, 457 insertions(+), 758 deletions(-)
>   create mode 100644 src/hyperv/hyperv_wsman.h
>