On 10/08/2014 06:33 AM, Yves Vinter wrote:
From: yvinter <yves.vinter@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