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>
---
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(-)
Another round of reviews; the .input and .py file look okay to me, but I
spotted more issues in the .c that I didn't point out the first time around.
+static int
+hypervConnectGetVersion(virConnectPtr conn, unsigned long *version)
+{
+ int result = -1;
+ hypervPrivate *priv = conn->privateData;
+ CIM_DataFile *datafile = NULL;
+ virBuffer query = VIR_BUFFER_INITIALIZER;
+ char *p;
+
Trailing whitespace. Another thing 'make syntax-check' will flag.
+ virBufferAddLit(&query, " Select * from CIM_DataFile
where Name='c:\\\\windows\\\\system32\\\\vmms.exe' ");
+ if (hypervGetCIMDataFileList(priv, &query, &datafile) < 0) {
+ goto cleanup;
+ }
+
+ /* Check the result of convertion */
s/convertion/conversion/
+ if (datafile == NULL) {
+ virReportError(VIR_ERR_INTERNAL_ERROR,
+ _("Could not lookup %s for domain %s"),
+ "Msvm_VirtualSystemSettingData",
+ datafile->data->Version);
Is this error message correct? That is, is datafile->data->Version the
name of the domain you are referring to in the message?
+ goto cleanup;
+ }
+
+ /* Delete release number and last digit of build number 1.1.111x.xxxx */
+ p = strrchr(datafile->data->Version,'.');
+ if (p == NULL) {
+ virReportError(VIR_ERR_INTERNAL_ERROR,
+ _("Could not parse version number from '%s'"),
+ datafile->data->Version);
+ goto cleanup;
+ }
+ p--;
+ *p = '\0';
This is modifying datafile->data in-place. I hope that's safe (if not,
you'd have to strdup a copy that you can safely manipulate locally).
+
+ /* Parse Version String to Long */
+ if (virParseVersionString(datafile->data->Version,
The comment is a bit redundant with the name of the function you are
calling. I'd just omit the comment.
--
Eric Blake eblake redhat com +1-919-301-3266
Libvirt virtualization library http://libvirt.org
Attachments:
- signature.asc (application/pgp-signature — 539 bytes)