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 <jfer...@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 -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list