10:30 a.m.
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(a)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
>