[libvirt] [PATCH 00/16] Hyper-V: Improve 2008, Introduce 2012

[Jason Miesionczek]

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 at redhat.com> 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