On 08/09/2016 08:39 AM, Jason Miesionczek wrote:
> ---
> src/Makefile.am | 1 +
> src/hyperv/hyperv_driver.c | 2 +
> src/hyperv/hyperv_network_driver.c | 124 +++++++++++++++++++++++++++++++++++++
> src/hyperv/hyperv_network_driver.h | 30 +++++++++
> src/hyperv/hyperv_wmi.c | 39 ++++++++++++
> src/hyperv/hyperv_wmi.h | 17 +++++
> 6 files changed, 213 insertions(+)
> create mode 100644 src/hyperv/hyperv_network_driver.c
> create mode 100644 src/hyperv/hyperv_network_driver.h
>
Hopefully by this point in the review I've shown the "need" to run
"syntax-check" - I'm not going to point out those things for the rest of
the review as my time to spend on this is getting shorter.
The "consistency" in formatting makes it easier to review/read libvirt
code regardless of the subsystem. Many people use 80 column displays, so
keeping your lines within that limit is beneficial.
> diff --git a/src/Makefile.am b/src/Makefile.am
> index 922b9b8..d03e6b0 100644
> --- a/src/Makefile.am
> +++ b/src/Makefile.am
> @@ -888,6 +888,7 @@ ESX_DRIVER_EXTRA_DIST = \
> HYPERV_DRIVER_SOURCES = \
> hyperv/hyperv_private.h \
> hyperv/hyperv_driver.c hyperv/hyperv_driver.h \
> + hyperv/hyperv_network_driver.c hyperv/hyperv_network_driver.h \
> hyperv/hyperv_util.c hyperv/hyperv_util.h \
> hyperv/hyperv_wmi.c hyperv/hyperv_wmi.h \
> hyperv/hyperv_wmi_classes.c hyperv/hyperv_wmi_classes.h \
> diff --git a/src/hyperv/hyperv_driver.c b/src/hyperv/hyperv_driver.c
> index aea7837..4c094e7 100644
> --- a/src/hyperv/hyperv_driver.c
> +++ b/src/hyperv/hyperv_driver.c
> @@ -30,6 +30,7 @@
> #include "virlog.h"
> #include "viruuid.h"
> #include "hyperv_driver.h"
> +#include "hyperv_network_driver.h"
> #include "hyperv_private.h"
> #include "hyperv_util.h"
> #include "hyperv_wmi.h"
> @@ -1853,6 +1854,7 @@ hypervDebugHandler(const char *message, debug_level_e level,
>
> static virConnectDriver hypervConnectDriver = {
> .hypervisorDriver = &hypervHypervisorDriver,
> + .networkDriver = &hypervNetworkDriver,
> };
>
> int
> diff --git a/src/hyperv/hyperv_network_driver.c b/src/hyperv/hyperv_network_driver.c
> new file mode 100644
> index 0000000..00037ae
> --- /dev/null
> +++ b/src/hyperv/hyperv_network_driver.c
> @@ -0,0 +1,124 @@
> +/*
> + * hyperv_network_driver.c: network driver functions for managing
> + * Microsoft Hyper-V host networks
> + *
> + * Copyright (C) 2011 Matthias Bolte <matthias.bolte@xxxxxxxxxxxxxx>
> + *
> + * This library is free software; you can redistribute it and/or
> + * modify it under the terms of the GNU Lesser General Public
> + * License as published by the Free Software Foundation; either
> + * version 2.1 of the License, or (at your option) any later version.
> + *
> + * This library is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU
> + * Lesser General Public License for more details.
> + *
> + * You should have received a copy of the GNU Lesser General Public
> + * License along with this library. If not, see
> + * <http://www.gnu.org/licenses/>.
> + *
> + */
> +
> +#include <config.h>
> +
> +#include "internal.h"
> +#include "virerror.h"
> +#include "datatypes.h"
> +#include "viralloc.h"
> +#include "viruuid.h"
> +#include "virstring.h"
> +#include "hyperv_network_driver.h"
> +#include "hyperv_wmi.h"
> +#include "network_conf.h"
> +
> +#define VIR_FROM_THIS VIR_FROM_HYPERV
> +
> +static virNetworkPtr
> +hypervNetworkLookupByName(virConnectPtr conn, const char *name)
> +{
> + virNetworkPtr network = NULL;
> + hypervPrivate *priv = conn->privateData;
> + virBuffer query = VIR_BUFFER_INITIALIZER;
> + Msvm_VirtualSwitch *virtualSwitch = NULL;
> +
> + virBufferAddLit(&query, MSVM_VIRTUALSWITCH_WQL_SELECT);
> + virBufferAsprintf(&query, "where Description = \"%s\" and ElementName = \"%s\"",
> + "Microsoft Virtual Switch", name);
> + if (hypervGetMsvmVirtualSwitchList(priv, &query, &virtualSwitch) < 0) {
> + goto cleanup;
> + }
> + if (virtualSwitch == NULL) {
> + virReportError(VIR_ERR_NO_NETWORK,
> + _("No network found with name %s"), name);
> + goto cleanup;
> + }
> +
> + hypervMsvmVirtualSwitchToNetwork(conn, virtualSwitch, &network);
> +
> + cleanup:
> + hypervFreeObject(priv, (hypervObject *) virtualSwitch);
> + virBufferFreeAndReset(&query);
> +
> + return network;
> +}
> +
> +static char *
> +hypervNetworkGetXMLDesc(virNetworkPtr network, unsigned int flags)
> +{
> + char *xml = NULL;
> + char uuid_string[VIR_UUID_STRING_BUFLEN];
> + hypervPrivate *priv = network->conn->privateData;
> + virNetworkDefPtr def = NULL;
> + virBuffer query = VIR_BUFFER_INITIALIZER;
> + Msvm_VirtualSwitch *virtualSwitch = NULL;
> +
> + /* Flags checked by virNetworkDefFormat */
> +
> + if (VIR_ALLOC(def) < 0) {
> + virReportOOMError();
unnecessary OOM - that's done for you as part of VIR_ALLOC failure
> + goto cleanup;
> + }
> +
> + virUUIDFormat(network->uuid, uuid_string);
> +
> + /* Get Msvm_VirtualSwitch */
> + virBufferAddLit(&query, MSVM_VIRTUALSWITCH_WQL_SELECT);
> + virBufferAsprintf(&query, "where Name = \"%s\"", uuid_string);
> + if (hypervGetMsvmVirtualSwitchList(priv, &query, &virtualSwitch) < 0) {
> + goto cleanup;
> + }
> + if (virtualSwitch == NULL) {
> + virReportError(VIR_ERR_NO_NETWORK,
> + _("No network found with UUID %s"), uuid_string);
> + goto cleanup;
> + }
> +
> + /* Fill struct */
> + if (virUUIDParse(virtualSwitch->data->Name, def->uuid) < 0) {
> + virReportError(VIR_ERR_INTERNAL_ERROR,
> + _("Could not parse UUID from string '%s'"),
> + virtualSwitch->data->Name);
> + return NULL;
> + }
> +
> + if (VIR_STRDUP(def->name, virtualSwitch->data->ElementName) < 0)
> + goto cleanup;
> +
> + xml = virNetworkDefFormat(def, flags);
> +
> + cleanup:
> + virNetworkDefFree(def);
> + hypervFreeObject(priv, (hypervObject *)virtualSwitch);
> + virBufferFreeAndReset(&query);
> +
> + return xml;
> +}
> +
> +
> +
> +virNetworkDriver hypervNetworkDriver = {
> + .name = "Hyper-V",
> + .networkLookupByName = hypervNetworkLookupByName, /* 1.2.10 */
> + .networkGetXMLDesc = hypervNetworkGetXMLDesc, /* 1.2.10 */
> +};
At least 2.3.0
> diff --git a/src/hyperv/hyperv_network_driver.h b/src/hyperv/hyperv_network_driver.h
> new file mode 100644
> index 0000000..54a4792
> --- /dev/null
> +++ b/src/hyperv/hyperv_network_driver.h
> @@ -0,0 +1,30 @@
> +/*
> + * hyperv_network_driver.h: network driver functions for managing
> + * Microsoft Hyper-V host networks
> + *
> + * Copyright (C) 2011 Matthias Bolte <matthias.bolte@xxxxxxxxxxxxxx>
> + *
> + * This library is free software; you can redistribute it and/or
> + * modify it under the terms of the GNU Lesser General Public
> + * License as published by the Free Software Foundation; either
> + * version 2.1 of the License, or (at your option) any later version.
> + *
> + * This library is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU
> + * Lesser General Public License for more details.
> + *
> + * You should have received a copy of the GNU Lesser General Public
> + * License along with this library. If not, see
> + * <http://www.gnu.org/licenses/>.
> + *
> + */
> +
> +#ifndef __HYPERV_NETWORK_DRIVER_H__
> +# define __HYPERV_NETWORK_DRIVER_H__
> +
> +int hypervNetworkRegister(void);
> +
> +extern virNetworkDriver hypervNetworkDriver;
> +
> +#endif /* __HYPERV_NETWORK_DRIVER_H__ */
> diff --git a/src/hyperv/hyperv_wmi.c b/src/hyperv/hyperv_wmi.c
> index 51d9b43..2334c49 100644
> --- a/src/hyperv/hyperv_wmi.c
> +++ b/src/hyperv/hyperv_wmi.c
> @@ -1326,5 +1326,44 @@ hypervInvokeMethod(hypervPrivate *priv, invokeXmlParam *param_t, int nbParameter
> }
>
>
> +int
> +hypervMsvmVirtualSwitchToNetwork(virConnectPtr conn,
> + Msvm_VirtualSwitch *virtualSwitch, virNetworkPtr *network)
> +{
> + unsigned char uuid[VIR_UUID_BUFLEN];
> + char *rawUuid = NULL;
> + char *uuidBuf;
> +
> + if (network == NULL || *network != NULL) {
> + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("Invalid argument"));
> + return -1;
> + }
> +
> + // Switch-SM-91b305f3-9a9e-408b-8b04-0ecff33aaba8-0
> + // need to parse out the 'Switch-SM-' and '-0' so that its a proper UUID
> + rawUuid = virtualSwitch->data->Name;
> + uuidBuf = (char *)calloc(37, sizeof(char));
> + strncpy(uuidBuf, rawUuid+10, 36);
Why not
if (VIR_STRNDUP(uuidBuf, rawUuid+10, 36) < 0)
What if virtualSwitch->data->Name isn't prefixed with "Switch-SM-"?
What if it's not postfixed with "-0"
Are these safe assumptions?
BTW: There's a STRPREFIX that could probably help you out here.
> +
> + if (virUUIDParse(uuidBuf, uuid) < 0) {
> + virReportError(VIR_ERR_INTERNAL_ERROR,
> + _("Could not parse UUID from string '%s'"),
> + virtualSwitch->data->Name);
> + free(uuidBuf);
VIR_FREE
> + return -1;
> + }
> +
> + *network = virGetNetwork(conn, virtualSwitch->data->ElementName, uuid);
> +
> + if (*network == NULL) {
> + free(uuidBuf);
VIR_FREE
> + return -1;
> + }
> +
> + free(uuidBuf);
VIR_FREE
> + return 0;
This is a function that should:
1. Initialize "int ret = -1;"
2. Utilize goto cleanup
3. Set ret = 0 just before the cleanup label
cleanup:
VIR_FREE(uuidBuf)
return ret;
After reading this and thinking about it, this function should be:
virNetworkPtr *
hypervMsvmVirtualSwitchToNetwork(virConnectPtr conn,
Msvm_VirtualSwitch *virtualSwitch)
{
char *rawUuid = virtualSwitch->data->Name;
char *uuidBuf = NULL;
unsigned char uuid[VIR_UUID_BUFLEN];
virNetworkPtr network = NULL;
if (strlen(rawUuid) != 40) {
virReportError(..., "unexpected length")
return NULL;
}
if (!STRPREFIX(rawUuid, "Switch-SM-")) {
virReportError(..., "unexpected prefix")
return NULL;
}
if (STRNEQ(rawUuid+38, "-0")) {
virReportError(..., "unexpected postfix")
return NULL
}
if (VIR_STRNDUP(uuidBuf, rawUuid + 10, 36) < 0)
return NULL;
if (virUUIDParse(uuidBuf, uuid) < 0) {
virReportError(VIR_ERR_INTERNAL_ERROR,
_("Could not parse UUID from string '%s'"),
rawUuid);
goto cleanup;
}
network = virGetNetwork(conn, virtualSwitch->data->ElementName,
uuid);
cleanup:
VIR_FREE(uuidBuf);
return network;
}
John
> +}
> +
> +
>
> #include "hyperv_wmi.generated.c"
> diff --git a/src/hyperv/hyperv_wmi.h b/src/hyperv/hyperv_wmi.h
> index c14d229..67f45fb 100644
> --- a/src/hyperv/hyperv_wmi.h
> +++ b/src/hyperv/hyperv_wmi.h
> @@ -172,6 +172,23 @@ int hypervMsvmComputerSystemFromDomain(virDomainPtr domain,
> Msvm_ComputerSystem **computerSystem);
>
>
> +/* * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * *
> + * Msvm_VirtualSwitch
> + */
> +
> +int hypervMsvmVirtualSwitchToNetwork(virConnectPtr conn,
> + Msvm_VirtualSwitch *virtualSwitch, virNetworkPtr *network);
> +
> +
> +
> +/* * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * *
> + * Msvm_VirtualSwitch
> + */
> +
> +int hypervMsvmVirtualSwitchToNetwork(virConnectPtr conn,
> + Msvm_VirtualSwitch *virtualSwitch, virNetworkPtr *network);
> +
> +
>
> # include "hyperv_wmi.generated.h"
>
>
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list