4:33 p.m.
On 08/09/2016 08:39 AM, Jason Miesionczek wrote:
---
src/hyperv/hyperv_driver.c | 106 +++++++++++++++++++++++++++++++++++++++++++-
src/hyperv/hyperv_private.h | 2 +
2 files changed, 107 insertions(+), 1 deletion(-)
diff --git a/src/hyperv/hyperv_driver.c b/src/hyperv/hyperv_driver.c
index b642a02..4a5e80d 100644
--- a/src/hyperv/hyperv_driver.c
+++ b/src/hyperv/hyperv_driver.c
@@ -35,6 +35,7 @@
#include "hyperv_wmi.h"
#include "openwsman.h"
#include "virstring.h"
+#include "virtypedparam.h"
#define VIR_FROM_THIS VIR_FROM_HYPERV
@@ -51,11 +52,15 @@ hypervFreePrivate(hypervPrivate **priv)
wsmc_release((*priv)->client);
}
+ if ((*priv)->caps != NULL)
+ virObjectUnref((*priv)->caps);
+
hypervFreeParsedUri(&(*priv)->parsedUri);
VIR_FREE(*priv);
}
-
+/* Forward declaration of hypervCapsInit */
+static virCapsPtr hypervCapsInit(hypervPrivate *priv);
Ewww.... Why not define the function right here then?
static virDrvOpenStatus
hypervConnectOpen(virConnectPtr conn, virConnectAuthPtr auth,
@@ -183,6 +188,12 @@ hypervConnectOpen(virConnectPtr conn, virConnectAuthPtr auth,
goto cleanup;
}
+ /* Setup capabilities */
+ priv->caps = hypervCapsInit(priv);
+ if (priv->caps == NULL) {
+ goto cleanup;
+ }
+
If there's only 1 line, then no need for { }
This occurs multiple times in this file.
You need to run "make check syntax-check"
I would change to :
if (!(priv->caps = hypervCapsInit(priv)))
goto cleanup;
conn->privateData = priv;
priv = NULL;
result = VIR_DRV_OPEN_SUCCESS;
@@ -1324,7 +1335,19 @@ hypervConnectListAllDomains(virConnectPtr conn,
}
#undef MATCH
Try to go with 2 blank lines between functions - it's the new normal for
our reviews...
+static char*
static char *
+hypervConnectGetCapabilities(virConnectPtr conn)
+{
+ hypervPrivate *priv = conn->privateData;
+ char *xml = virCapabilitiesFormatXML(priv->caps);
+ if (xml == NULL) {
+ virReportOOMError();
This should be unnecessary. If there was a failure to memory allocation
it would already be messaged. If there was some other failure, then
you're overwriting that error.
+ return NULL;
+ }
So this simply becomes:
return virCapabilitiesFormatXML(priv->caps);
+
+ return xml;
+}
static virHypervisorDriver hypervHypervisorDriver = {
@@ -1361,8 +1384,89 @@ static virHypervisorDriver hypervHypervisorDriver = {
.domainHasManagedSaveImage = hypervDomainHasManagedSaveImage, /* 0.9.5 */
.domainManagedSaveRemove = hypervDomainManagedSaveRemove, /* 0.9.5 */
.connectIsAlive = hypervConnectIsAlive, /* 0.9.8 */
+ .connectGetCapabilities = hypervConnectGetCapabilities, /* 1.2.10 */
s/1.2.10/2.3.0/
(at the very earliest)
};
blank line
+/* 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;
+ }
Unnecessary {}
+
+ if (computerSystem == NULL) {
+ virReportError(VIR_ERR_NO_DOMAIN,
You need to a "%s, " e.g.:
virReportError(VIR_ERR_NO_DOMAIN, "%s",
(something syntax-check would complain about)
+ _("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);
+
+ return result;
+}
+
+
Only 2 blank lines necessary
+
+static virCapsPtr hypervCapsInit(hypervPrivate *priv)
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;
+ }
Unnecessary OOM
if (!(caps = virCapabilitiesNew(VIR_ARCH_X86_64, 1, 1)))
return NULL;
+
+ /* virCapabilitiesSetMacPrefix(caps, (unsigned char[]){ 0x00, 0x0c, 0x29 }); */
+
+ if (hypervLookupHostSystemBiosUuid(priv,caps->host.host_uuid) < 0) {
s/priv,caps/priv, caps/
Need a space between args - syntax-check complaint.
+ goto failure;
+ }
Unnecessary {}
s/failure/error/
> +
> + /* i686 */
> + guest = virCapabilitiesAddGuest(caps, VIR_DOMAIN_OSTYPE_HVM, VIR_ARCH_I686,
NULL, NULL, 0, NULL);
> + if (guest == NULL) {
+ goto failure;
+ }
> + if (virCapabilitiesAddGuestDomain(guest, VIR_DOMAIN_VIRT_HYPERV,
NULL, NULL, 0, NULL) == NULL) {
+ goto failure;
+ }
> +
> + /* x86_64 */
> + guest = virCapabilitiesAddGuest(caps, VIR_DOMAIN_OSTYPE_HVM, VIR_ARCH_X86_64,
NULL, NULL, 0, NULL);
> + if (guest == NULL) {
+ goto failure;
+ }
> + if (virCapabilitiesAddGuestDomain(guest, VIR_DOMAIN_VIRT_HYPERV,
NULL, NULL, 0, NULL) == NULL) {
+ goto failure;
+ }
These are all really long lines... They all fail syntax-check due to {}
issues... Try to have each line be 80 columns, consider
if (!(guest = virCapabilitiesAddGuest(...)))
goto error;
if (!
+
+ return caps;
+
+ failure:
We use error for error labels not failure
John
> + virObjectUnref(caps);
> + return NULL;
> +}
>
>
> static void
> diff --git a/src/hyperv/hyperv_private.h b/src/hyperv/hyperv_private.h
> index 574bb5f..d9aa0bd 100644
> --- a/src/hyperv/hyperv_private.h
> +++ b/src/hyperv/hyperv_private.h
> @@ -26,6 +26,7 @@
> # include "internal.h"
> # include "virerror.h"
> # include "hyperv_util.h"
> +# include "capabilities.h"
> # include "openwsman.h"
>
> typedef struct _hypervPrivate hypervPrivate;
> @@ -33,6 +34,7 @@ typedef struct _hypervPrivate hypervPrivate;
> struct _hypervPrivate {
> hypervParsedUri *parsedUri;
> WsManClient *client;
> + virCapsPtr caps;
};
> #endif /* __HYPERV_PRIVATE_H__ */
>