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

On 08/09/2016 08:39 AM, Jason Miesionczek wrote: 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