2016-09-16 21:06 GMT+02:00 Matthias Bolte <matthias.bolte@xxxxxxxxxxxxxx>:
> [please don't top-post]
>
> 2016-09-16 20:11 GMT+02:00 Jason Miesionczek <jmiesionczek@xxxxxxxxx>:
>> 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@xxxxxxxxxxxxxx> wrote:
>>>
>>> 2016-09-16 18:35 GMT+02:00 Jason Miesionczek <jmiesionczek@xxxxxxxxx>:
>>>> 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
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list