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__ */
>
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list