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

[Laine Stump]

On 1/22/21 11:05 AM, Laine Stump wrote:
> (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.


I asked about item (2) on IRC just now, and danpb produced a short 
example program that proves it is okay to use values from auto-freed 
objects as the return value of a function. So there is only question (1) 
left. Let me know and I'll either push or wait for modified patches 
accordingly.


>
>
> 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
>>
>