Thank you very much for your review and your excellent suggestions. I will revisit the code, clean things up as best I can and submit much smaller patches that can be reviewed and merged one at a time.
Thanks again!
Best,
Jason Miesionczek
> On Sep 15, 2016, at 5:56 PM, John Ferlan <jferlan@xxxxxxxxxx> wrote:
>
>
>
> On 08/09/2016 08:39 AM, Jason Miesionczek wrote:
>> The following patches include work originally done by Yves Vinter back
>> in 2014. The last patch introduces support for Hyper-V 2012, while still
>> supporting 2008. I am not sure that the method I used to include the 2012
>> support is the best approach, mainly due to code duplication, but I am
>> open to suggestions on how to do this better.
>>
>> Jason Miesionczek (16):
>> hyperv: additional server 2008 wmi classes
>> hyperv: add cim types support to code generator
>> hyperv: add get capabilities
>> hyperv: implement connectGetVersion
>> hyperv: implement vcpu functions
>> hyperv: implement nodeGetFreeMemory
>> hyperv: implement ability to send xml soap requests
>> hyperv: introduce basic network driver
>> hyperv: add domain shutdown function
>> hyperv: add get scheduler functions
>> hyperv: add set memory functions
>> hyperv: set vpcu functions
>> hyperv: domain undefine functions
>> hyperv: domain define and associated functions
>> hyperv: network list functions
>> hyperv: introduce 2012 support
>>
>> src/Makefile.am | 2 +
>> src/hyperv/hyperv_driver.c | 1989 ++++++++++++++++++++++++++++++++-
>> src/hyperv/hyperv_driver_2012.c | 299 +++++
>> src/hyperv/hyperv_driver_2012.h | 55 +
>> src/hyperv/hyperv_network_driver.c | 280 +++++
>> src/hyperv/hyperv_network_driver.h | 30 +
>> src/hyperv/hyperv_private.h | 8 +
>> src/hyperv/hyperv_wmi.c | 709 +++++++++++-
>> src/hyperv/hyperv_wmi.h | 78 ++
>> src/hyperv/hyperv_wmi_generator.input | 518 ++++++++-
>> src/hyperv/hyperv_wmi_generator.py | 68 +-
>> src/hyperv/openwsman.h | 4 +
>> 12 files changed, 3989 insertions(+), 51 deletions(-)
>> create mode 100644 src/hyperv/hyperv_driver_2012.c
>> create mode 100644 src/hyperv/hyperv_driver_2012.h
>> create mode 100644 src/hyperv/hyperv_network_driver.c
>> create mode 100644 src/hyperv/hyperv_network_driver.h
>>
>
> That concludes my first adventure into the hyperv driver... Don't be too
> discouraged - there's a long list of changes that need to be done, it's
> not impossible.
>
> It's very important to work from top of the upstream git tree and to be
> sure to "make check syntax-check" *before* posting patches - that'll
> clear out a lot of repetitive stuff.
>
> I think though you need to consider posting in smaller chunks. It'll be
> a marathon, not a sprint. The bandwidth for reviews isn't very wide
> either.
>
> FWIW: So that you can consider this earlier...
>
> Eventually I realized the query strings that you're formulating that
> start with "associators of" - well I think you might be better of
> creating a "generic function" rather than cut-n-paste a similar sequence
> in every function.
>
> So here's my suggestion create a common function now, then modify the
> existing code to call/use that function. Then modify each of the
> subsequent patches to do the same for example:
>
> (NOTE: Rename param1-4 to something better, it's my shorthand)
>
> static Msvm_VirtualSystemSettingData *
> hypervBuildQueryBuffer(const char *param1, const char *param2,
> const char *param3, const char *param4)
> {
> virBuffer query = VIR_BUFFER_INITIALIZER;
>
> virBufferAddLit(&query, "associators of ");
> virBufferAsprintf(&query, "{%s=\"%s\"} ", param1, param2);
> virBufferAsprintf(&query, "where AssocClass = %s ", param3);
> virBufferAsprintf(&query, "ResultClass = %s", param4);
>
> if (virBufferCheckError(&query) < 0)
> return NULL;
>
> return query;
> }
>
> ...
> Then also create constant shortcuts, such as:
>
> #define WIN32_CS_NAME "Win32_ComputerSystem.Name"
> #define WIN32_CSP "Win32_ComputerSystemProcessor"
> #define WIN32_PROC "Win32_Processor"
>
> So that calling would be (for example from existing source)
>
> if (!(query = hypervBuildQueryBuffer(WIN32_CS_NAME,
> computerSystem->data->Name,
> WIN32_CSP,
> WIN32_PROC)))
> goto cleanup/error
> ...
>
> The rest I'll leave up to you, but doing some #define string
> concatenation will make things look better. The painful looking on is
> the "Msvm_ComputerSystem.CreationClassName=..."
>
> *Anywhere* that it's possible to reuse code or reduce the cut-copy-paste
> should be considered.
>
> You may even want to consider making some sort of static struct to
> "predefine" the lookup function to be called and the parameter,
> considering the following a start:
>
> typedef int (*hypervListFunc)(hypervPrivate *priv, virBufferPtr query,
> void **list);
> struct _hypervList {
> int type;
> hypervListFunc func;
> };
> static const _hypervList hypervListTable[] = {
>
> { .type = HYPERV_LIST_WIN32_PROCESSORS,
> .func = hypervGetWin32ProcessorList,
> },
> { .type = HYPERV_LIST_MSVM_VSSD,
> .func = hypervGetMsvmVirtualSystemSettingDataList,
> },
> ...
> };
>
> Then the above QueryBuffer gets "expanded" a bit to pass in a "type" and
> "void" which we can reference into the table in order to get the .func
> to call. My brain cannot handle (right now) how to have variable types
> of data to return... It has to be possible it's just a matter of
> working through it in order to find the magic incantation or someone
> else's working example.
>
> John
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list