8:19 a.m.
(1) Long subject line. I'd suggest
hyperv: add implementation for virConnectGetCapabilities
Done by adding the Win32_ComputerSystemProduct class.
"Done" is not the appropriate word;
adding Win32_ComputerSystemProduct class was just a requirement to make the
implementation.
(2) Dead 'if'; virObjectUnref(NULL) is safe
Stands also for patch #20 with xmlopt.
(3) Why do you need a forward declaration of a static function?
Just because the new code was "grouped" near the end of the file.
It's not a problem to move the block of functions hypervLookupHostSystemBiosUuid &
hypervCapsInit just before function hypervConnectOpen and remove this forward
declaration.
(4) Is VIR_ERR_NO_DOMAIN really the right error to be using here?
No; it's a mistake; VIR_ERR_INTERNAL_ERROR error should be used instead.
(5) Libvirt style is two blank lines between functions, and function name starting in
column 1 of a line separate from the return type:
In the current version of the hyperv libvirt driver, functions are actually separated by
three blank lines.
(6) Might be nice to explain why this is commented out.
/* virCapabilitiesSetMacPrefix(caps, (unsigned char[]){ 0x00, 0x0c, 0x29 }); */
Has been commented because virCapabilitiesSetMacPrefix does not exist.
Is that something that a generic function that has been removed from the libvirt?
BTY. the Mac Prefix for hyperv is 00-15-0D
(7) Only need two blank lines
In the current version of the hyperv libvirt driver, functions are actually separated by
three blank lines.
All the other functions must be reviewed then...
-----Message d'origine-----
De : Eric Blake [mailto:eblake@redhat.com]
Envoyé : jeudi 9 octobre 2014 00:34
À : Yves Vinter; libvir-list(a)redhat.com
Objet : Re: [libvirt] [PATCH 03/21] Added basic implementation for
virConnectGetCapabilities (required Win32_ComputerSystemProduct class)
On 10/08/2014 06:33 AM, Yves Vinter wrote:
From: yvinter <yves.vinter(a)bull.net>
Long subject line. I'd suggest:
hyperv: add implementation for virConnectGetCapabilities
Done by adding the Win32_ComputerSystemProduct class.
---
src/hyperv/hyperv_driver.c | 112 ++++++++++++++++++++++++++++++++++
src/hyperv/hyperv_private.h | 2 +
src/hyperv/hyperv_wmi_generator.input | 12 ++++
3 files changed, 126 insertions(+)
diff --git a/src/hyperv/hyperv_driver.c b/src/hyperv/hyperv_driver.c
index f2017c3..dd56fb0 100644
--- a/src/hyperv/hyperv_driver.c
+++ b/src/hyperv/hyperv_driver.c
@@ -58,12 +58,19 @@ hypervFreePrivate(hypervPrivate **priv)
wsmc_release((*priv)->client);
}
+ if ((*priv)->caps != NULL)
+ virObjectUnref((*priv)->caps);
Dead 'if'; virObjectUnref(NULL) is safe.
+/* Forward declaration of hypervCapsInit */ static virCapsPtr
+hypervCapsInit(hypervPrivate *priv);
Why do you need a forward declaration of a static function? If it's not recursive,
just put the whole function body here (that may mean moving more than one function, to
keep things topologically sorted; if you need to do code motion of existing functions, do
it in a separate patch from where you add new code, to ease review).
+/* Retrieves host system UUID */
+static int
+hypervLookupHostSystemBiosUuid(hypervPrivate *priv, unsigned char
+*uuid) {
+ Win32_ComputerSystemProduct *computerSystem = NULL;
+ virBuffer query = VIR_BUFFER_INITIALIZER;
+ int result = -1;
+
+ virBufferAddLit(&query, WIN32_COMPUTERSYSTEMPRODUCT_WQL_SELECT);
+
+ if (hypervGetWin32ComputerSystemProductList(priv, &query, &computerSystem)
< 0) {
+ goto cleanup;
+ }
+
+ if (computerSystem == NULL) {
+ virReportError(VIR_ERR_NO_DOMAIN,
Is VIR_ERR_NO_DOMAIN really the right error to be using here?
+ _("Unable to get
Win32_ComputerSystemProduct"));
+ goto cleanup;
+ }
+
+ if (virUUIDParse(computerSystem->data->UUID, uuid) < 0) {
+ virReportError(VIR_ERR_INTERNAL_ERROR,
+ _("Could not parse UUID from string '%s'"),
+ computerSystem->data->UUID);
+ goto cleanup;
+ }
+
+ result = 0;
+
+ cleanup:
+ hypervFreeObject(priv, (hypervObject *)computerSystem);
+ virBufferFreeAndReset(&query);
This virBufferFreeAndReset is not needed if you take my alternate patch for 1/21.
+
+ return result;
+}
+
+
+
+static virCapsPtr hypervCapsInit(hypervPrivate *priv)
Libvirt style is two blank lines between functions, and function name starting in column 1
of a line separate from the return type:
static virCapsPtr
hypervCapsInit(hypervPrivate *priv)
+{
+ virCapsPtr caps = NULL;
+ virCapsGuestPtr guest = NULL;
+
+ caps = virCapabilitiesNew(VIR_ARCH_X86_64, 1, 1);
+
+ if (caps == NULL) {
+ virReportOOMError();
+ return NULL;
+ }
+
+ /* virCapabilitiesSetMacPrefix(caps, (unsigned char[]){ 0x00,
+ 0x0c, 0x29 }); */
Might be nice to explain why this is commented out.
+}
+
+
+
+static char*
Only need two blank lines.
+hypervConnectGetCapabilities(virConnectPtr conn) {
+ hypervPrivate *priv = conn->privateData;
+ char *xml = virCapabilitiesFormatXML(priv->caps);
virCapabilitiesFormatXML can only return NULL on error, and it already outputs a decent
error message; also, error is not always due to OOM...
+
+ if (xml == NULL) {
+ virReportOOMError();
...so this virReportOOMError() is bogus, because it overwrites the decent message.
+ return NULL;
+ }
Once you realize that, you can simplify this function to:
static char*
hypervConnectGetCapabilities(virConnectPtr conn) {
hypervPrivate *priv = conn->privateData;
return virCapabilitiesFormatXML(priv->caps);
}
--
Eric Blake eblake redhat com +1-919-301-3266
Libvirt virtualization library http://libvirt.org