[libvirt] [PATCH v2a] HyperV: Improve 2008, introduce 2012
Second round of patches based on recently complete code review. Going to submit patches in much smaller chunks, starting with this one. Future patches will be submitted as each previous patch is reviewed and merged. Jason Miesionczek (1): hyperv: add new WMI classes and improve generator src/hyperv/hyperv_wmi_generator.input | 485 ++++++++++++++++++++++++++++++++++ src/hyperv/hyperv_wmi_generator.py | 57 +++- 2 files changed, 539 insertions(+), 3 deletions(-) -- 2.7.4
--- src/hyperv/hyperv_wmi_generator.input | 485 ++++++++++++++++++++++++++++++++++ src/hyperv/hyperv_wmi_generator.py | 57 +++- 2 files changed, 539 insertions(+), 3 deletions(-) diff --git a/src/hyperv/hyperv_wmi_generator.input b/src/hyperv/hyperv_wmi_generator.input index 97f9dff..abd72e8 100644 --- a/src/hyperv/hyperv_wmi_generator.input +++ b/src/hyperv/hyperv_wmi_generator.input @@ -296,3 +296,488 @@ class Win32_Processor string Version uint32 VoltageCaps end + +class CIM_DataFile + uint32 AccessMask + boolean Archive + string Caption + boolean Compressed + string CompressionMethod + string CreationClassName + datetime CreationDate + string CSCreationClassName + string CSName + string Description + string Drive + string EightDotThreeFileName + boolean Encrypted + string EncryptionMethod + string Extension + string FileName + uint64 FileSize + string FileType + string FSCreationClassName + string FSName + boolean Hidden + datetime InstallDate + uint64 InUseCount + datetime LastAccessed + datetime LastModified + string Manufacturer + string Name + string Path + boolean Readable + string Status + boolean System + string Version + boolean Writeable +end + + +class Win32_ComputerSystemProduct + string Caption + string Description + string IdentifyingNumber + string Name + string SKUNumber + string UUID + string Vendor + string Version +end + + +class Win32_PerfRawData_HvStats_HyperVHypervisorVirtualProcessor + uint64 AddressDomainFlushesPersec + uint64 AddressSpaceEvictionsPersec + uint64 AddressSpaceFlushesPersec + uint64 AddressSpaceSwitchesPersec + uint64 APICEOIAccessesPersec + uint64 APICIPIsSentPersec + uint64 APICMMIOAccessesPersec + uint64 APICSelfIPIsSentPersec + uint64 APICTPRAccessesPersec + string Caption + uint64 ControlRegisterAccessesCost + uint64 ControlRegisterAccessesCost_Base + uint64 ControlRegisterAccessesPersec + uint64 CPUIDInstructionsCost + uint64 CPUIDInstructionsCost_Base + uint64 CPUIDInstructionsPersec + uint64 CPUWaitTimePerDispatch + uint64 CPUWaitTimePerDispatch_Base + uint64 DebugRegisterAccessesCost + uint64 DebugRegisterAccessesCost_Base + uint64 DebugRegisterAccessesPersec + string Description + uint64 EmulatedInstructionsCost + uint64 EmulatedInstructionsCost_Base + uint64 EmulatedInstructionsPersec + uint64 ExternalInterruptsCost + uint64 ExternalInterruptsCost_Base + uint64 ExternalInterruptsPersec + uint64 Frequency_Object + uint64 Frequency_PerfTime + uint64 Frequency_Sys100NS + uint64 GlobalGVARangeFlushesPersec + uint64 GPASpaceHypercallsPersec + uint64 GuestPageTableMapsPersec + uint64 HardwareInterruptsPersec + uint64 HLTInstructionsCost + uint64 HLTInstructionsCost_Base + uint64 HLTInstructionsPersec + uint64 HypercallsCost + uint64 HypercallsCost_Base + uint64 HypercallsPersec + uint64 IOInstructionsCost + uint64 IOInstructionsCost_Base + uint64 IOInstructionsPersec + uint64 IOInterceptMessagesPersec + uint64 LargePageTLBFillsPersec + uint64 LocalFlushedGVARangesPersec + uint64 LogicalProcessorDispatchesPersec + uint64 LogicalProcessorHypercallsPersec + uint64 LogicalProcessorMigrationsPersec + uint64 LongSpinWaitHypercallsPersec + uint64 MemoryInterceptMessagesPersec + uint64 MSRAccessesCost + uint64 MSRAccessesCost_Base + uint64 MSRAccessesPersec + uint64 MWAITInstructionsCost + uint64 MWAITInstructionsCost_Base + uint64 MWAITInstructionsPersec + string Name + uint64 NestedPageFaultInterceptsCost + uint64 NestedPageFaultInterceptsCost_Base + uint64 NestedPageFaultInterceptsPersec + uint64 OtherHypercallsPersec + uint64 OtherInterceptsCost + uint64 OtherInterceptsCost_Base + uint64 OtherInterceptsPersec + uint64 OtherMessagesPersec + uint64 PageFaultInterceptsCost + uint64 PageFaultInterceptsCost_Base + uint64 PageFaultInterceptsPersec + uint64 PageInvalidationsCost + uint64 PageInvalidationsCost_Base + uint64 PageInvalidationsPersec + uint64 PageTableAllocationsPersec + uint64 PageTableEvictionsPersec + uint64 PageTableReclamationsPersec + uint64 PageTableResetsPersec + uint64 PageTableValidationsPersec + uint64 PageTableWriteInterceptsPersec + uint64 PendingInterruptsCost + uint64 PendingInterruptsCost_Base + uint64 PendingInterruptsPersec + uint64 PercentGuestRunTime + uint64 PercentGuestRunTime_Base + uint64 PercentHypervisorRunTime + uint64 PercentHypervisorRunTime_Base + uint64 PercentRemoteRunTime + uint64 PercentRemoteRunTime_Base + uint64 PercentTotalRunTime + uint64 PercentTotalRunTime_Base + uint64 ReflectedGuestPageFaultsPersec + uint64 SmallPageTLBFillsPersec + uint64 SyntheticInterruptHypercallsPersec + uint64 SyntheticInterruptsPersec + uint64 Timestamp_Object + uint64 Timestamp_PerfTime + uint64 Timestamp_Sys100NS + uint64 TotalInterceptsCost + uint64 TotalInterceptsCost_Base + uint64 TotalInterceptsPersec + uint64 TotalMessagesPersec + uint64 VirtualInterruptHypercallsPersec + uint64 VirtualInterruptsPersec + uint64 VirtualMMUHypercallsPersec + uint64 VirtualProcessorHypercallsPersec +end + + +class Win32_OperatingSystem + string BootDevice + string BuildNumber + string BuildType + string Caption + string CodeSet + string CountryCode + string CreationClassName + string CSCreationClassName + string CSDVersion + string CSName + uint16 CurrentTimeZone + boolean DataExecutionPrevention_Available + boolean DataExecutionPrevention_32BitApplications + boolean DataExecutionPrevention_Drivers + uint8 DataExecutionPrevention_SupportPolicy + boolean Debug + string Description + boolean Distributed + uint32 EncryptionLevel + uint8 ForegroundApplicationBoost + uint64 FreePhysicalMemory + uint64 FreeSpaceInPagingFiles + uint64 FreeVirtualMemory + datetime InstallDate + uint32 LargeSystemCache + datetime LastBootUpTime + datetime LocalDateTime + string Locale + string Manufacturer + uint32 MaxNumberOfProcesses + uint64 MaxProcessMemorySize + string MUILanguages[] + string Name + uint32 NumberOfLicensedUsers + uint32 NumberOfProcesses + uint32 NumberOfUsers + uint32 OperatingSystemSKU + string Organization + string OSArchitecture + uint32 OSLanguage + uint32 OSProductSuite + uint16 OSType + string OtherTypeDescription + boolean PAEEnabled + string PlusProductID + string PlusVersionNumber +# boolean PortableOperatingSystem # documented, but not found + boolean Primary + uint32 ProductType + string RegisteredUser + string SerialNumber + uint16 ServicePackMajorVersion + uint16 ServicePackMinorVersion + uint64 SizeStoredInPagingFiles + string Status + uint32 SuiteMask + string SystemDevice + string SystemDirectory + string SystemDrive + uint64 TotalSwapSpaceSize + uint64 TotalVirtualMemorySize + uint64 TotalVisibleMemorySize + string Version + string WindowsDirectory +end + + +class Msvm_VirtualSwitch + string Caption + string Description + string ElementName + datetime InstallDate + uint16 OperationalStatus[] + string StatusDescriptions[] + string Status + uint16 HealthState + uint16 EnabledState + string OtherEnabledState + uint16 RequestedState + uint16 EnabledDefault + datetime TimeOfLastStateChange + string CreationClassName + string Name + string PrimaryOwnerContact + string PrimaryOwnerName + string Roles[] + string NameFormat + string OtherIdentifyingInfo[] + string IdentifyingDescriptions[] + uint16 Dedicated[] + string OtherDedicatedDescriptions[] + uint16 ResetCapability + uint16 PowerManagementCapabilities[] + string ScopeOfResidence + uint32 NumLearnableAddresses + uint32 MaxVMQOffloads + uint32 MaxChimneyOffloads +end + + +class Msvm_VirtualSystemManagementService + string Caption + string Description + string ElementName + datetime InstallDate + uint16 OperationalStatus + string StatusDescriptions + string Status + uint16 HealthState + uint16 EnabledState + string OtherEnabledState + uint16 RequestedState + uint16 EnabledDefault + datetime TimeOfLastStateChange + string SystemCreationClassName + string SystemName + string CreationClassName + string Name + string PrimaryOwnerName + string PrimaryOwnerContact + string StartMode + boolean Started +end + + +class Msvm_VirtualSystemGlobalSettingData + string Caption + string Description + string ElementName + string InstanceID + string SystemName + uint16 SettingType + uint16 VirtualSystemType + string OtherVirtualSystemType + boolean AutoActivate + datetime CreationTime + string ExternalDataRoot + string SnapshotDataRoot + uint16 AutomaticStartupAction + datetime AutomaticStartupActionDelay + uint16 AutomaticShutdownAction + uint16 AutomaticRecoveryAction + string AdditionalRecoveryInformation + string ScopeOfResidence + uint32 DebugChannelId + boolean AllowFullSCSICommandSet + string Version +end + + +class Msvm_VirtualSwitch + string Caption + string Description + string ElementName + datetime InstallDate + uint16 OperationalStatus[] + string StatusDescriptions[] + string Status + uint16 HealthState + uint16 EnabledState + string OtherEnabledState + uint16 RequestedState + uint16 EnabledDefault + datetime TimeOfLastStateChange + string CreationClassName + string Name + string PrimaryOwnerContact + string PrimaryOwnerName + string Roles[] + string NameFormat + string OtherIdentifyingInfo[] + string IdentifyingDescriptions[] + uint16 Dedicated[] + string OtherDedicatedDescriptions[] + uint16 ResetCapability + uint16 PowerManagementCapabilities[] + string ScopeOfResidence + uint32 NumLearnableAddresses + uint32 MaxVMQOffloads + uint32 MaxChimneyOffloads +end + + +class Msvm_ResourceAllocationSettingData + string Caption + string Description + string InstanceID + string ElementName + uint16 ResourceType + string OtherResourceType + string ResourceSubType + string PoolID + uint16 ConsumerVisibility + string HostResource[] + string AllocationUnits + uint64 VirtualQuantity + uint64 Reservation + uint64 Limit + uint32 Weight + boolean AutomaticAllocation + boolean AutomaticDeallocation + string Parent + string Connection[] + string Address + uint16 MappingBehavior + string VirtualSystemIdentifiers[] +end + + +class Msvm_AllocationCapabilities + string Caption + string Description + string ElementName + string InstanceID + string OtherResourceType + uint16 RequestTypesSupported + string ResourceSubType + uint16 ResourceType + uint16 SharingMode + uint16 SupportedAddStates[] + uint16 SupportedRemoveStates[] +end + + +class Msvm_SwitchPort + string Caption + string ElementName + datetime InstallDate + string StatusDescriptions[] + string Status + uint16 HealthState + string OtherEnabledState + uint16 RequestedState + uint16 EnabledDefault + string SystemCreationClassName + string SystemName + string CreationClassName + string Description + uint16 OperationalStatus[] + uint16 EnabledState + datetime TimeOfLastStateChange + string Name + string NameFormat + uint16 ProtocolType + uint16 ProtocolIFType + string OtherTypeDescription + boolean BroadcastResetSupported + uint16 PortNumber + string ScopeOfResidence + uint32 VMQOffloadWeight + uint32 ChimneyOffloadWeight + uint32 VMQOffloadUsage + uint32 ChimneyOffloadUsage + uint32 VMQOffloadLimit + uint32 ChimneyOffloadLimit + boolean AllowMacSpoofing +end + + +class Msvm_SyntheticEthernetPortSettingData + string Caption + string Description + string InstanceID + string ElementName + uint16 ResourceType + string OtherResourceType + string ResourceSubType + string PoolID + uint16 ConsumerVisibility + string HostResource[] + string AllocationUnits + uint64 VirtualQuantity + uint64 Reservation + uint64 Limit + uint32 Weight + boolean AutomaticAllocation + boolean AutomaticDeallocation + string Parent + string Connection[] + string Address + uint16 MappingBehavior + string VirtualSystemIdentifiers[] + boolean StaticMacAddress +end + + +class Msvm_VirtualSwitchManagementService + string Caption + string Description + string ElementName + datetime InstallDate + uint16 OperationalStatus[] + string StatusDescriptions[] + string Status + uint16 HealthState + uint16 EnabledState + string OtherEnabledState + uint16 RequestedState + uint16 EnabledDefault + datetime TimeOfLastStateChange + string SystemCreationClassName + string SystemName + string CreationClassName + string Name + string PrimaryOwnerName + string PrimaryOwnerContact + string StartMode + boolean Started +end + +class Msvm_VirtualHardDiskSettingData + string InstanceID + string Caption + string Description + string ElementName + uint16 Type + uint16 Format + string Path + string ParentPath + uint64 MaxInternalSize + uint32 BlockSize + uint32 LogicalSectorSize + uint32 PhysicalSectorSize + string VirtualDiskId +end diff --git a/src/hyperv/hyperv_wmi_generator.py b/src/hyperv/hyperv_wmi_generator.py index f767d54..8384634 100755 --- a/src/hyperv/hyperv_wmi_generator.py +++ b/src/hyperv/hyperv_wmi_generator.py @@ -68,7 +68,7 @@ class Class: header += "\n" header += "#define %s_RESOURCE_URI \\\n" % name_upper - if self.name.startswith("Win32_"): + if self.name.startswith("Win32_") or self.name.startswith("CIM_DataFile"): header += " \"http://schemas.microsoft.com/wbem/wsman/1/wmi/root/cimv2/%s\"\n" % self.name else: header += " \"http://schemas.microsoft.com/wbem/wsman/1/wmi/root/virtualization/%s\"\n" % self.name @@ -100,6 +100,25 @@ class Class: return header + def generate_cimtypes_header2(self): + header = separator + header += " * %s\n" % self.name + header += " */\n" + header += "\n" + header += "CimTypes cimTypes_%s[] = {\n" % self.name + for property in self.properties: + header += property.generate_cimtypes_header2() + header += ",\n" + header += "\t{ \"\", \"\", 0 },\n};\n\n" + + return header + + def generate_cimtypes_header3(self): + header = " { \"%s" % self.name + header += "\", cimTypes_%s" % self.name + header += " },\n" + + return header def generate_source(self): name_upper = self.name.upper() @@ -113,7 +132,7 @@ class Class: % (self.name.replace("_", ""), self.name) source += "{\n" - if self.name.startswith("Win32_"): + if self.name.startswith("Win32_") or self.name.startswith("CIM_DataFile"): source += " return hypervEnumAndPull(priv, query, ROOT_CIMV2,\n" else: source += " return hypervEnumAndPull(priv, query, ROOT_VIRTUALIZATION,\n" @@ -189,6 +208,16 @@ class Property: return " SER_NS_%s(%s_RESOURCE_URI, \"%s\", 1),\n" \ % (Property.typemap[self.type], class_name.upper(), self.name) + def generate_cimtypes_header2(self): + header = " { \"%s" % self.name + header += "\", \"%s\", " % self.type + if self.is_array: + header += "true" + else: + header += "false" + header += " }" + return header + def open_and_print(filename): @@ -238,7 +267,20 @@ def parse_class(block): return Class(name=name, properties=properties) - +def generate_cimtypes_header1(): + header = "struct cimTypes{\n" + header += " const char *name;\n" + header += " const char *type;\n" + header += " bool isArray;\n" + header += "};\n" + header += "typedef struct cimTypes CimTypes;\n\n" + header += "struct cimClasses{\n" + header += " const char *name;\n" + header += " CimTypes *cimTypesPtr;\n" + header += "};\n" + header += "typedef struct cimClasses CimClasses;\n\n" + + return header def main(): if "srcdir" in os.environ: @@ -253,6 +295,7 @@ def main(): classes_typedef = open_and_print(os.path.join(output_dirname, "hyperv_wmi_classes.generated.typedef")) classes_header = open_and_print(os.path.join(output_dirname, "hyperv_wmi_classes.generated.h")) classes_source = open_and_print(os.path.join(output_dirname, "hyperv_wmi_classes.generated.c")) + cimtypes_header = open_and_print(os.path.join(output_dirname, "hyperv_wmi_cimtypes.generated.h")) # parse input file number = 0 @@ -294,6 +337,8 @@ def main(): classes_typedef.write(notice) classes_header.write(notice) classes_source.write(notice) + cimtypes_header.write(notice) + cimtypes_header.write(generate_cimtypes_header1()) names = classes_by_name.keys() names.sort() @@ -304,6 +349,12 @@ def main(): classes_typedef.write(classes_by_name[name].generate_classes_typedef()) classes_header.write(classes_by_name[name].generate_classes_header()) classes_source.write(classes_by_name[name].generate_classes_source()) + cimtypes_header.write(classes_by_name[name].generate_cimtypes_header2()) + + cimtypes_header.write("CimClasses cimClasses[] = {\n") + for name in names: + cimtypes_header.write(classes_by_name[name].generate_cimtypes_header3()) + cimtypes_header.write("\t{ \"\", NULL },\n};\n") -- 2.7.4
2016-09-16 18:35 GMT+02:00 Jason Miesionczek <jmiesionczek@datto.com>:
--- src/hyperv/hyperv_wmi_generator.input | 485 ++++++++++++++++++++++++++++++++++ src/hyperv/hyperv_wmi_generator.py | 57 +++- 2 files changed, 539 insertions(+), 3 deletions(-)
I've already pushed a cleanup version of your first patch from the previous series yesterday. Along with some parts from the second patch to make it self-contained. Also you still have the code in here to generate the 2-level string lookup table for the parameter types. As I explained in my response to patch 7, I think the string based lookup inefficient and unnecessary. The generator has all the information to make this a 2-level index based lookup. Therefore, NACK. -- Matthias Bolte http://photron.blogspot.com
2016-09-16 18:35 GMT+02:00 Jason Miesionczek <jmiesionczek@datto.com>:
Second round of patches based on recently complete code review. Going to submit patches in much smaller chunks, starting with this one. Future patches will be submitted as each previous patch is reviewed and merged.
1-commit flow isn't ideal either. If I could just look at commit 1 alone then I would not have understood how you're going to use this new 2-level string lookup table. As John suggested you should try to work in smaller chunks, like 4-6 commits at a time. And also try to keep them grouped by topic. You should avoid having multiple independent things in one patch, like the new types and the table generation or the autostart functions and the general invoke functions. Do one thing per commit and ensure that each commit can stand alone, e.g. the code compiles and works cleanly after each commit. For example the first commit adds some new types. The second commit uses these new types. But those two commits would not add the string table. This would either be a separate commit or be part of the commit add the gen general code the uses the string table. -- Matthias Bolte http://photron.blogspot.com
Not sure I understand what you mean by ‘not adding the string table’. Could you please elaborate? I understand what you are saying about splitting things up into more logical chunks, by topic, so i will do that for the patches going forward. Thanks for your feedback, i’m still pretty new to C (not having touched it in about 15 years), so I appreciate the help. Also, could you elaborate on the 2-level string data vs. single level enum/index stuff? I’m not sure I follow what you are suggesting. Thanks!
On Sep 16, 2016, at 2:02 PM, Matthias Bolte <matthias.bolte@googlemail.com> wrote:
2016-09-16 18:35 GMT+02:00 Jason Miesionczek <jmiesionczek@datto.com>:
Second round of patches based on recently complete code review. Going to submit patches in much smaller chunks, starting with this one. Future patches will be submitted as each previous patch is reviewed and merged.
1-commit flow isn't ideal either.
If I could just look at commit 1 alone then I would not have understood how you're going to use this new 2-level string lookup table.
As John suggested you should try to work in smaller chunks, like 4-6 commits at a time. And also try to keep them grouped by topic.
You should avoid having multiple independent things in one patch, like the new types and the table generation or the autostart functions and the general invoke functions. Do one thing per commit and ensure that each commit can stand alone, e.g. the code compiles and works cleanly after each commit.
For example the first commit adds some new types. The second commit uses these new types.
But those two commits would not add the string table. This would either be a separate commit or be part of the commit add the gen general code the uses the string table.
-- Matthias Bolte http://photron.blogspot.com
[please don't top-post] 2016-09-16 20:11 GMT+02:00 Jason Miesionczek <jmiesionczek@datto.com>:
Not sure I understand what you mean by ‘not adding the string table’. Could you please elaborate?
I understand what you are saying about splitting things up into more logical chunks, by topic, so i will do that for the patches going forward.
Thanks for your feedback, i’m still pretty new to C (not having touched it in about 15 years), so I appreciate the help.
Also, could you elaborate on the 2-level string data vs. single level enum/index stuff? I’m not sure I follow what you are suggesting.
Thanks!
On Sep 16, 2016, at 2:02 PM, Matthias Bolte <matthias.bolte@googlemail.com> wrote:
2016-09-16 18:35 GMT+02:00 Jason Miesionczek <jmiesionczek@datto.com>:
Second round of patches based on recently complete code review. Going to submit patches in much smaller chunks, starting with this one. Future patches will be submitted as each previous patch is reviewed and merged.
1-commit flow isn't ideal either.
If I could just look at commit 1 alone then I would not have understood how you're going to use this new 2-level string lookup table.
As John suggested you should try to work in smaller chunks, like 4-6 commits at a time. And also try to keep them grouped by topic.
You should avoid having multiple independent things in one patch, like the new types and the table generation or the autostart functions and the general invoke functions. Do one thing per commit and ensure that each commit can stand alone, e.g. the code compiles and works cleanly after each commit.
For example the first commit adds some new types. The second commit uses these new types.
But those two commits would not add the string table. This would either be a separate commit or be part of the commit add the gen general code the uses the string table.
You generate a 2-level lookup table like this: CimTypes cimTypes_CIM_DataFile[] = { { "AccessMask", "uint32", false }, { "Archive", "boolean", false }, { "Caption", "string", false }, ... { "", "", 0 }, }; CimClasses cimClasses[] = { { "CIM_DataFile", cimTypes_CIM_DataFile }, ... { "", NULL }, }; As a side note, this also lacks hyperv prefixes for the types and global variables. This lookup table is then used in the invoke logic to figure out how to add embedded parameters to the WSMAN XML. To find something in the lookup table you use two nested for loops with string compares. This is inefficient. I can see no good reason that would force you to do this with strings. I suggest doing this with an index/enum based approach: typedef enum { hypervProperty_CIM_DataFile_AccessMask = 0, hypervProperty_CIM_DataFile_Archive, hypervProperty_CIM_DataFile_Caption, ... } hypervProperty; typedef struct { const char *name; const char *type; bool isArray; } hypervPropertyInfo; hypervPropertyInfo hypervPropertyInfos[] = { { "AccessMask", "uint32", false }, { "Archive", "boolean", false }, { "Caption", "string", false }, ... }; Now getting the type of a property works like this: const char *type = hypervPropertyInfos[hypervProperty_CIM_DataFile_Archive].type; This has O(1) access instead of O(n) and the compiler will tell you if you're trying to use a CIM type that the generator doesn't know. With the string based approach this would be a runtime error. Or another approach could be having a giant struct: typedef struct { hypervPropertyInfo CIM_DataFile_AccessMask; hypervPropertyInfo CIM_DataFile_Archive; hypervPropertyInfo CIM_DataFile_Caption; ... } hypervPropertyInfoCollection; static const hypervPropertyInfoCollection hypervPropertyInfos = { { "AccessMask", "uint32", false }, { "Archive", "boolean", false }, { "Caption", "string", false }, ... }; Now getting the type of a property works like this: const char *type = hypervPropertyInfos.CIM_DataFile_Archive.type; I think I'd actually prefer this way, because it results in shorter lookup expression. -- Matthias Bolte http://photron.blogspot.com
2016-09-16 21:06 GMT+02:00 Matthias Bolte <matthias.bolte@googlemail.com>:
[please don't top-post]
2016-09-16 20:11 GMT+02:00 Jason Miesionczek <jmiesionczek@datto.com>:
Not sure I understand what you mean by ‘not adding the string table’. Could you please elaborate?
I understand what you are saying about splitting things up into more logical chunks, by topic, so i will do that for the patches going forward.
Thanks for your feedback, i’m still pretty new to C (not having touched it in about 15 years), so I appreciate the help.
Also, could you elaborate on the 2-level string data vs. single level enum/index stuff? I’m not sure I follow what you are suggesting.
Thanks!
On Sep 16, 2016, at 2:02 PM, Matthias Bolte <matthias.bolte@googlemail.com> wrote:
2016-09-16 18:35 GMT+02:00 Jason Miesionczek <jmiesionczek@datto.com>:
Second round of patches based on recently complete code review. Going to submit patches in much smaller chunks, starting with this one. Future patches will be submitted as each previous patch is reviewed and merged.
1-commit flow isn't ideal either.
If I could just look at commit 1 alone then I would not have understood how you're going to use this new 2-level string lookup table.
As John suggested you should try to work in smaller chunks, like 4-6 commits at a time. And also try to keep them grouped by topic.
You should avoid having multiple independent things in one patch, like the new types and the table generation or the autostart functions and the general invoke functions. Do one thing per commit and ensure that each commit can stand alone, e.g. the code compiles and works cleanly after each commit.
For example the first commit adds some new types. The second commit uses these new types.
But those two commits would not add the string table. This would either be a separate commit or be part of the commit add the gen general code the uses the string table.
You generate a 2-level lookup table like this:
CimTypes cimTypes_CIM_DataFile[] = { { "AccessMask", "uint32", false }, { "Archive", "boolean", false }, { "Caption", "string", false }, ... { "", "", 0 }, };
CimClasses cimClasses[] = { { "CIM_DataFile", cimTypes_CIM_DataFile }, ... { "", NULL }, };
As a side note, this also lacks hyperv prefixes for the types and global variables.
This lookup table is then used in the invoke logic to figure out how to add embedded parameters to the WSMAN XML.
To find something in the lookup table you use two nested for loops with string compares. This is inefficient. I can see no good reason that would force you to do this with strings.
I suggest doing this with an index/enum based approach:
typedef enum { hypervProperty_CIM_DataFile_AccessMask = 0, hypervProperty_CIM_DataFile_Archive, hypervProperty_CIM_DataFile_Caption, ... } hypervProperty;
typedef struct { const char *name; const char *type; bool isArray; } hypervPropertyInfo;
hypervPropertyInfo hypervPropertyInfos[] = { { "AccessMask", "uint32", false }, { "Archive", "boolean", false }, { "Caption", "string", false }, ... };
Now getting the type of a property works like this:
const char *type = hypervPropertyInfos[hypervProperty_CIM_DataFile_Archive].type;
This has O(1) access instead of O(n) and the compiler will tell you if you're trying to use a CIM type that the generator doesn't know. With the string based approach this would be a runtime error.
Or another approach could be having a giant struct:
typedef struct { hypervPropertyInfo CIM_DataFile_AccessMask; hypervPropertyInfo CIM_DataFile_Archive; hypervPropertyInfo CIM_DataFile_Caption; ... } hypervPropertyInfoCollection;
static const hypervPropertyInfoCollection hypervPropertyInfos = { { "AccessMask", "uint32", false }, { "Archive", "boolean", false }, { "Caption", "string", false }, ... };
Now getting the type of a property works like this:
const char *type = hypervPropertyInfos.CIM_DataFile_Archive.type;
I think I'd actually prefer this way, because it results in shorter lookup expression.
Also, you'd change struct _properties_t{ const char *name; const char *val; }; to typedef struct { hypervProperty index; const char *val; } hypervPropertyValue; or typedef struct { hypervPropertyInfo *info; const char *value; } hypervPropertyValue; To pass the property info by index or by pointer to the invoke function. -- Matthias Bolte http://photron.blogspot.com
participants (2)
-
Jason Miesionczek -
Matthias Bolte