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

[Matthias Bolte]

2016-09-16 21:06 GMT+02:00 Matthias Bolte <matthias.bolte at googlemail.com>:
> [please don't top-post]
>
> 2016-09-16 20:11 GMT+02:00 Jason Miesionczek <jmiesionczek at 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 at googlemail.com> wrote:
>>>
>>> 2016-09-16 18:35 GMT+02:00 Jason Miesionczek <jmiesionczek at 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