9:07 a.m.
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(a)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