On 10/08/2014 06:33 AM, Yves Vinter wrote:
From: yvinter <yves.vinter@bull.net>
Looks like you had identity problems throughout the series. I don't mind touching up first-time contributions, but it would be nice to get it right in the future so I don't have to spend time on it.
--- src/hyperv/hyperv_driver.c | 55 +++++++++++++++++++++++++++++++++++ src/hyperv/hyperv_wmi_generator.input | 37 +++++++++++++++++++++++ src/hyperv/hyperv_wmi_generator.py | 4 +-- 3 files changed, 94 insertions(+), 2 deletions(-)
diff --git a/src/hyperv/hyperv_driver.c b/src/hyperv/hyperv_driver.c index 372ff39..f2017c3 100644 --- a/src/hyperv/hyperv_driver.c +++ b/src/hyperv/hyperv_driver.c @@ -1232,6 +1232,7 @@ hypervDomainManagedSaveRemove(virDomainPtr domain, unsigned int flags) }
+ #define MATCH(FLAG) (flags & (FLAG))
Why the added blank line?
static int hypervConnectListAllDomains(virConnectPtr conn, @@ -1366,6 +1367,59 @@ hypervConnectListAllDomains(virConnectPtr conn,
+static int
Wow, that's a lot of blank lines already there.
+hypervConnectGetVersion(virConnectPtr conn, unsigned long *version) +{ + int result = -1; + hypervPrivate *priv = conn->privateData; + CIM_DataFile *datafile = NULL;
Why the double space?
+ virBuffer query = VIR_BUFFER_INITIALIZER; + char *p; + + virBufferAddLit(&query, " Select * from CIM_DataFile where Name='c:\\\\windows\\\\system32\\\\vmms.exe' ");
Is the leading and trailing space really necessary in the string literal?
+ if (hypervGetCIMDataFileList(priv, &query, &datafile) < 0) { + goto cleanup; + }
Libvirt style says {} are not necessary for one-line bodies (qemu style would require it, but we're a bit more lenient).
+ + /* Check the result of convertion */ + if (datafile == NULL) {
It's sufficient to write 'if (datafile)' (comparison to NULL is just extra verbosity); but as that appears to be the prevailing style already in this file, I'm not going to worry about shortening it.
+ virReportError(VIR_ERR_INTERNAL_ERROR, + _("Could not lookup %s for domain %s"), + "Msvm_VirtualSystemSettingData", + datafile->data->Version); + goto cleanup; + } + + /* Delete release number and last digit of build number 1.1.111x.xxxx */ + p = strrchr(datafile->data->Version,'.');
This failed 'make syntax-check' due to a missing space: GEN bracket-spacing-check src/hyperv/hyperv_driver.c:1381: p = strrchr(datafile->data->Version,'.'); maint.mk: incorrect whitespace, see HACKING for rules make: *** [bracket-spacing-check] Error 1
+ if (p == NULL) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("Could not parse version number from '%s'"), + datafile->data->Version); + goto cleanup; + } + p--; + *p = '\0'; + + /* Parse Version String to Long */ + if (virParseVersionString(datafile->data->Version, + version, true) < 0) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("Could not parse version number from '%s'"), + datafile->data->Version); + goto cleanup; + } + + result = 0; + + cleanup: + hypervFreeObject(priv, (hypervObject *)datafile); + virBufferFreeAndReset(&query);
This line is not necessary if you ACK my v2 of 1/21.
+ + return result; +} + +
static virDriver hypervDriver = { .no = VIR_DRV_HYPERV, @@ -1402,6 +1456,7 @@ static virDriver hypervDriver = { .domainHasManagedSaveImage = hypervDomainHasManagedSaveImage, /* 0.9.5 */ .domainManagedSaveRemove = hypervDomainManagedSaveRemove, /* 0.9.5 */ .connectIsAlive = hypervConnectIsAlive, /* 0.9.8 */ + .connectGetVersion = hypervConnectGetVersion, /* 1.2.10 */ };
I got through here in my review for now; mostly looks good other than style. When I finish the rest of the patch, I may end up just fixing it myself and pushing. -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org