Re: [libvirt] [PATCH v2a] HyperV: Improve 2008, introduce 2012

[please don't top-post] 2016-09-16 20:11 GMT+02:00 Jason Miesionczek <jmiesionczek(a)datto.com&gt;: > 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(a)googlemail.com&gt; wrote: >> >> 2016-09-16 18:35 GMT+02:00 Jason Miesionczek <jmiesionczek(a)datto.com&gt;: >>> 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.