Re: [libvirt] [PATCH 02/21] Added implementation for virConnectGetVersion (required CIM_DataFile class)
On 10/08/2014 06:33 AM, Yves Vinter wrote:
From: yvinter <yves.vinter(a)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
Attachments:
- signature.asc (application/pgp-signature — 539 bytes)