(1) syntax-check:
I run make syntax-check and found some other "white space" or "TAB" issues (coming with subsequent patches)
(2) Is this error message correct? That is, is datafile->data->Version the name of the domain you are referring to in the message?
More than that: this part of code is completely wrong!
datafile->data->Version shouldn't be evaluated if datafile == NULL!!!
> + if (datafile == NULL) {
> + virReportError(VIR_ERR_INTERNAL_ERROR,
> + _("Could not lookup %s for domain %s"),
> + "Msvm_VirtualSystemSettingData",
> + datafile->data->Version
Right code is:
If (datafile == NULL) {
virReportError(VIR_ERR_INTERNAL_ERROR,
_("Unable to get the hypervisor version"));
}
[ Removed the comment: /* Check the result of conversion */]
(3) 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).
It is safe; datafile is a temporary object that is released at the end of hypervConnectGetVersion call.
(4) The comment is a bit redundant with the name of the function you are calling. I'd just omit the comment.
No problem...
-----Message d'origine-----
De : Eric Blake [mailto:eblake@xxxxxxxxxx]
Envoyé : jeudi 9 octobre 2014 00:15
À : Yves Vinter; libvir-list@xxxxxxxxxx
Objet : Re: [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@xxxxxxxx>
>
> ---
> 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
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list