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__ */