[PATCH 3/8] hyperv: break out common lookups into separate functions

Pino Toscano ptoscano at redhat.com
Fri Oct 2 06:25:30 UTC 2020


On Thursday, 1 October 2020 23:47:12 CEST mcoleman at datto.com wrote:
> +static int
> +hypervGetProcessorsByName(hypervPrivate *priv, const char *name,
> +                          Win32_Processor **processorList)
> +{
> +    g_auto(virBuffer) query = VIR_BUFFER_INITIALIZER;
> +    virBufferEscapeSQL(&query,
> +                       "ASSOCIATORS OF {Win32_ComputerSystem.Name=\"%s\"} "
> +                       "WHERE AssocClass = Win32_ComputerSystemProcessor "
> +                       "ResultClass = Win32_Processor",
> +                       name);
> +
> +    if (hypervGetWin32ProcessorList(priv, &query, processorList) < 0 || !processorList) {

This line seems too long, and it is easy to not notice the second check
when not using work wrapping. I suggest to split it in two lines, i.e.:

   if (hypervGetWin32ProcessorList(priv, &query, processorList) < 0 ||
       !processorList) {

The same applies for all the other checks in the new functions of this
patch.

> +static int
> +hypervGetActiveVirtualSystemList(hypervPrivate *priv,
> +                                 Msvm_ComputerSystem **computerSystemList)
> +{
> +    g_auto(virBuffer) query = { g_string_new(MSVM_COMPUTERSYSTEM_WQL_SELECT
> +                                             "WHERE " MSVM_COMPUTERSYSTEM_WQL_VIRTUAL
> +                                             "AND " MSVM_COMPUTERSYSTEM_WQL_ACTIVE), 0 };
> +
> +    if (hypervGetMsvmComputerSystemList(priv, &query, computerSystemList) < 0 || !*computerSystemList) {
> +        virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("Could not look up active virtual machines"));

This virReportError() needs to be split in multiple lines; the same
applies to the other virReportError()s.

> @@ -410,18 +559,9 @@ hypervDomainLookupByID(virConnectPtr conn, int id)
>  {
>      virDomainPtr domain = NULL;
>      hypervPrivate *priv = conn->privateData;
> -    g_auto(virBuffer) query = VIR_BUFFER_INITIALIZER;
>      Msvm_ComputerSystem *computerSystem = NULL;
>  
> -    virBufferAddLit(&query, MSVM_COMPUTERSYSTEM_WQL_SELECT);
> -    virBufferAddLit(&query, "where ");
> -    virBufferAddLit(&query, MSVM_COMPUTERSYSTEM_WQL_VIRTUAL);
> -    virBufferAsprintf(&query, "and ProcessID = %d", id);
> -
> -    if (hypervGetMsvmComputerSystemList(priv, &query, &computerSystem) < 0)
> -        goto cleanup;
> -
> -    if (computerSystem == NULL) {
> +    if (hypervGetVirtualSystemByID(priv, id, &computerSystem) < 0) {
>          virReportError(VIR_ERR_NO_DOMAIN, _("No domain with ID %d"), id);
>          goto cleanup;

Note that now hypervGetVirtualSystemByID() issues VIR_ERR_INTERNAL_ERROR
in case &computerSystem / *computerSystemList is null, instead of
VIR_ERR_NO_DOMAIN. Shouldn't this still be able to explicitly report
when the requested domain does not exist?

> @@ -442,20 +582,11 @@ hypervDomainLookupByUUID(virConnectPtr conn, const unsigned char *uuid)
>      virDomainPtr domain = NULL;
>      hypervPrivate *priv = conn->privateData;
>      char uuid_string[VIR_UUID_STRING_BUFLEN];
> -    g_auto(virBuffer) query = VIR_BUFFER_INITIALIZER;
>      Msvm_ComputerSystem *computerSystem = NULL;
>  
>      virUUIDFormat(uuid, uuid_string);
>  
> -    virBufferAddLit(&query, MSVM_COMPUTERSYSTEM_WQL_SELECT);
> -    virBufferAddLit(&query, "where ");
> -    virBufferAddLit(&query, MSVM_COMPUTERSYSTEM_WQL_VIRTUAL);
> -    virBufferEscapeSQL(&query, "and Name = \"%s\"", uuid_string);
> -
> -    if (hypervGetMsvmComputerSystemList(priv, &query, &computerSystem) < 0)
> -        goto cleanup;
> -
> -    if (computerSystem == NULL) {
> +    if (hypervGetVirtualSystemByUUID(priv, uuid_string, &computerSystem) < 0) {
>          virReportError(VIR_ERR_NO_DOMAIN,
>                         _("No domain with UUID %s"), uuid_string);

Ditto.

> @@ -476,18 +607,9 @@ hypervDomainLookupByName(virConnectPtr conn, const char *name)
>  {
>      virDomainPtr domain = NULL;
>      hypervPrivate *priv = conn->privateData;
> -    g_auto(virBuffer) query = VIR_BUFFER_INITIALIZER;
>      Msvm_ComputerSystem *computerSystem = NULL;
>  
> -    virBufferAddLit(&query, MSVM_COMPUTERSYSTEM_WQL_SELECT);
> -    virBufferAddLit(&query, "where ");
> -    virBufferAddLit(&query, MSVM_COMPUTERSYSTEM_WQL_VIRTUAL);
> -    virBufferEscapeSQL(&query, "and ElementName = \"%s\"", name);
> -
> -    if (hypervGetMsvmComputerSystemList(priv, &query, &computerSystem) < 0)
> -        goto cleanup;
> -
> -    if (computerSystem == NULL) {
> +    if (hypervGetVirtualSystemByName(priv, name, &computerSystem) < 0) {
>          virReportError(VIR_ERR_NO_DOMAIN,
>                         _("No domain with name %s"), name);

Ditto.

> @@ -615,7 +737,6 @@ hypervDomainGetInfo(virDomainPtr domain, virDomainInfoPtr info)
>      int result = -1;
>      hypervPrivate *priv = domain->conn->privateData;
>      char uuid_string[VIR_UUID_STRING_BUFLEN];
> -    g_auto(virBuffer) query = VIR_BUFFER_INITIALIZER;
>      Msvm_ComputerSystem *computerSystem = NULL;
>      Msvm_VirtualSystemSettingData *virtualSystemSettingData = NULL;
>      Msvm_ProcessorSettingData *processorSettingData = NULL;
> @@ -629,21 +750,8 @@ hypervDomainGetInfo(virDomainPtr domain, virDomainInfoPtr info)
>      if (hypervMsvmComputerSystemFromDomain(domain, &computerSystem) < 0)
>          goto cleanup;
>  
> -    /* Get Msvm_VirtualSystemSettingData */
> -    virBufferEscapeSQL(&query,
> -                       "associators of "
> -                       "{Msvm_ComputerSystem.CreationClassName=\"Msvm_ComputerSystem\","
> -                       "Name=\"%s\"} "
> -                       "where AssocClass = Msvm_SettingsDefineState "
> -                       "ResultClass = Msvm_VirtualSystemSettingData",
> -                       uuid_string);
> -
> -    if (hypervGetMsvmVirtualSystemSettingDataList(priv, &query,
> -                                                  &virtualSystemSettingData) < 0) {
> -        goto cleanup;
> -    }
> -
> -    if (virtualSystemSettingData == NULL) {
> +    if (hypervGetVSSDFromUUID(priv, uuid_string,
> +                              &virtualSystemSettingData) < 0) {
>          virReportError(VIR_ERR_INTERNAL_ERROR,
>                         _("Could not lookup %s for domain %s"),
>                         "Msvm_VirtualSystemSettingData",

Considering the new hypervGetVSSDFromUUID() already uses
virReportError() on failure, this virReportError() above seems not
needed anymore.

> @@ -651,20 +759,9 @@ hypervDomainGetInfo(virDomainPtr domain, virDomainInfoPtr info)
>          goto cleanup;
>      }
>  
> -    /* Get Msvm_ProcessorSettingData */
> -    virBufferEscapeSQL(&query,
> -                       "associators of "
> -                       "{Msvm_VirtualSystemSettingData.InstanceID=\"%s\"} "
> -                       "where AssocClass = Msvm_VirtualSystemSettingDataComponent "
> -                       "ResultClass = Msvm_ProcessorSettingData",
> -                       virtualSystemSettingData->data.common->InstanceID);
> -
> -    if (hypervGetMsvmProcessorSettingDataList(priv, &query,
> -                                              &processorSettingData) < 0) {
> -        goto cleanup;
> -    }
> -
> -    if (processorSettingData == NULL) {
> +    if (hypervGetProcSDByVSSDInstanceId(priv,
> +                            virtualSystemSettingData->data.common->InstanceID,
> +                            &processorSettingData) < 0) {
>          virReportError(VIR_ERR_INTERNAL_ERROR,
>                         _("Could not lookup %s for domain %s"),
>                         "Msvm_ProcessorSettingData",

Ditto.

> @@ -672,21 +769,9 @@ hypervDomainGetInfo(virDomainPtr domain, virDomainInfoPtr info)
>          goto cleanup;
>      }
>  
> -    /* Get Msvm_MemorySettingData */
> -    virBufferEscapeSQL(&query,
> -                       "associators of "
> -                       "{Msvm_VirtualSystemSettingData.InstanceID=\"%s\"} "
> -                       "where AssocClass = Msvm_VirtualSystemSettingDataComponent "
> -                       "ResultClass = Msvm_MemorySettingData",
> -                       virtualSystemSettingData->data.common->InstanceID);
> -
> -    if (hypervGetMsvmMemorySettingDataList(priv, &query,
> -                                           &memorySettingData) < 0) {
> -        goto cleanup;
> -    }
> -
> -
> -    if (memorySettingData == NULL) {
> +    if (hypervGetMemSDByVSSDInstanceId(priv,
> +                           virtualSystemSettingData->data.common->InstanceID,
> +                           &memorySettingData) < 0) {
>          virReportError(VIR_ERR_INTERNAL_ERROR,
>                         _("Could not lookup %s for domain %s"),
>                         "Msvm_MemorySettingData",

Ditto.

> @@ -749,7 +834,6 @@ hypervDomainGetXMLDesc(virDomainPtr domain, unsigned int flags)
>      hypervPrivate *priv = domain->conn->privateData;
>      virDomainDefPtr def = NULL;
>      char uuid_string[VIR_UUID_STRING_BUFLEN];
> -    g_auto(virBuffer) query = VIR_BUFFER_INITIALIZER;
>      Msvm_ComputerSystem *computerSystem = NULL;
>      Msvm_VirtualSystemSettingData *virtualSystemSettingData = NULL;
>      Msvm_ProcessorSettingData *processorSettingData = NULL;
> @@ -766,21 +850,8 @@ hypervDomainGetXMLDesc(virDomainPtr domain, unsigned int flags)
>      if (hypervMsvmComputerSystemFromDomain(domain, &computerSystem) < 0)
>          goto cleanup;
>  
> -    /* Get Msvm_VirtualSystemSettingData */
> -    virBufferEscapeSQL(&query,
> -                       "associators of "
> -                       "{Msvm_ComputerSystem.CreationClassName=\"Msvm_ComputerSystem\","
> -                       "Name=\"%s\"} "
> -                       "where AssocClass = Msvm_SettingsDefineState "
> -                       "ResultClass = Msvm_VirtualSystemSettingData",
> -                       uuid_string);
> -
> -    if (hypervGetMsvmVirtualSystemSettingDataList(priv, &query,
> -                                                  &virtualSystemSettingData) < 0) {
> -        goto cleanup;
> -    }
> -
> -    if (virtualSystemSettingData == NULL) {
> +    if (hypervGetVSSDFromUUID(priv, uuid_string,
> +                              &virtualSystemSettingData) < 0) {
>          virReportError(VIR_ERR_INTERNAL_ERROR,
>                         _("Could not lookup %s for domain %s"),
>                         "Msvm_VirtualSystemSettingData",

Ditto.

> @@ -788,20 +859,9 @@ hypervDomainGetXMLDesc(virDomainPtr domain, unsigned int flags)
>          goto cleanup;
>      }
>  
> -    /* Get Msvm_ProcessorSettingData */
> -    virBufferEscapeSQL(&query,
> -                       "associators of "
> -                       "{Msvm_VirtualSystemSettingData.InstanceID=\"%s\"} "
> -                       "where AssocClass = Msvm_VirtualSystemSettingDataComponent "
> -                       "ResultClass = Msvm_ProcessorSettingData",
> -                       virtualSystemSettingData->data.common->InstanceID);
> -
> -    if (hypervGetMsvmProcessorSettingDataList(priv, &query,
> -                                              &processorSettingData) < 0) {
> -        goto cleanup;
> -    }
> -
> -    if (processorSettingData == NULL) {
> +    if (hypervGetProcSDByVSSDInstanceId(priv,
> +                           virtualSystemSettingData->data.common->InstanceID,
> +                           &processorSettingData) < 0) {
>          virReportError(VIR_ERR_INTERNAL_ERROR,
>                         _("Could not lookup %s for domain %s"),
>                         "Msvm_ProcessorSettingData",

Ditto.

> @@ -809,21 +869,9 @@ hypervDomainGetXMLDesc(virDomainPtr domain, unsigned int flags)
>          goto cleanup;
>      }
>  
> -    /* Get Msvm_MemorySettingData */
> -    virBufferEscapeSQL(&query,
> -                       "associators of "
> -                       "{Msvm_VirtualSystemSettingData.InstanceID=\"%s\"} "
> -                       "where AssocClass = Msvm_VirtualSystemSettingDataComponent "
> -                       "ResultClass = Msvm_MemorySettingData",
> -                       virtualSystemSettingData->data.common->InstanceID);
> -
> -    if (hypervGetMsvmMemorySettingDataList(priv, &query,
> -                                           &memorySettingData) < 0) {
> -        goto cleanup;
> -    }
> -
> -
> -    if (memorySettingData == NULL) {
> +    if (hypervGetMemSDByVSSDInstanceId(priv,
> +                           virtualSystemSettingData->data.common->InstanceID,
> +                           &memorySettingData) < 0) {
>          virReportError(VIR_ERR_INTERNAL_ERROR,
>                         _("Could not lookup %s for domain %s"),
>                         "Msvm_MemorySettingData",

Ditto.

-- 
Pino Toscano
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 833 bytes
Desc: This is a digitally signed message part.
URL: <http://listman.redhat.com/archives/libvir-list/attachments/20201002/f966aff5/attachment-0001.sig>


More information about the libvir-list mailing list