On 10/08/2014 06:33 AM, Yves Vinter wrote:
> From: yvinter <yves.vinter@xxxxxxxx>
>
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
Attachment:
signature.asc
Description: OpenPGP digital signature
-- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list