[libvirt] [PATCH 1/3] esx_vi.c: Simplify error handling in MachineByName
The same pattern is used in lots of other places. Signed-off-by: Marcos Paulo de Souza <marcos.souza.org@gmail.com> --- src/esx/esx_vi.c | 14 ++++---------- 1 file changed, 4 insertions(+), 10 deletions(-) diff --git a/src/esx/esx_vi.c b/src/esx/esx_vi.c index 43ff7ea048..25fbdc7e44 100644 --- a/src/esx/esx_vi.c +++ b/src/esx/esx_vi.c @@ -3014,16 +3014,10 @@ esxVI_LookupVirtualMachineByName(esxVI_Context *ctx, const char *name, break; } - if (!(*virtualMachine)) { - if (occurrence == esxVI_Occurrence_OptionalItem) { - result = 0; - - goto cleanup; - } else { - virReportError(VIR_ERR_NO_DOMAIN, - _("Could not find domain with name '%s'"), name); - goto cleanup; - } + if (!(*virtualMachine) && occurrence != esxVI_Occurrence_OptionalItem) { + virReportError(VIR_ERR_NO_DOMAIN, + _("Could not find domain with name '%s'"), name); + goto cleanup; } result = 0; -- 2.17.1
This macro avoids code duplication when checking for arrays of objects. Signed-off-by: Marcos Paulo de Souza <marcos.souza.org@gmail.com> --- src/esx/esx_vi.c | 189 ++++++++++++----------------------------------- 1 file changed, 46 insertions(+), 143 deletions(-) diff --git a/src/esx/esx_vi.c b/src/esx/esx_vi.c index 25fbdc7e44..212300dbff 100644 --- a/src/esx/esx_vi.c +++ b/src/esx/esx_vi.c @@ -41,6 +41,16 @@ VIR_LOG_INIT("esx.esx_vi"); +#define esxVI_checkArgList(val) \ + do { \ + if (!val || *val) { \ + virReportError(VIR_ERR_INVALID_ARG, "%s", _("Invalid argument")); \ + return -1; \ + } \ + } while (0) + + + #define ESX_VI__SOAP__RESPONSE_XPATH(_type) \ ((char *)"/soapenv:Envelope/soapenv:Body/" \ "vim:"_type"Response/vim:returnval") @@ -51,11 +61,7 @@ VIR_LOG_INIT("esx.esx_vi"); int \ esxVI_##_type##_Alloc(esxVI_##_type **ptrptr) \ { \ - if (!ptrptr || *ptrptr) { \ - virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("Invalid argument")); \ - return -1; \ - } \ - \ + esxVI_checkArgList(ptrptr); \ if (VIR_ALLOC(*ptrptr) < 0) \ return -1; \ return 0; \ @@ -372,10 +378,7 @@ esxVI_CURL_Download(esxVI_CURL *curl, const char *url, char **content, virBuffer buffer = VIR_BUFFER_INITIALIZER; int responseCode = 0; - if (!content || *content) { - virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("Invalid argument")); - return -1; - } + esxVI_checkArgList(content); if (length && *length > 0) { /* @@ -1762,10 +1765,7 @@ esxVI_List_DeepCopy(esxVI_List **destList, esxVI_List *srcList, esxVI_List *dest = NULL; esxVI_List *src = NULL; - if (!destList || *destList) { - virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("Invalid argument")); - return -1; - } + esxVI_checkArgList(destList); for (src = srcList; src; src = src->_next) { if (deepCopyFunc(&dest, src) < 0 || @@ -2170,10 +2170,7 @@ esxVI_LookupObjectContentByType(esxVI_Context *ctx, bool propertySpec_isAppended = false; esxVI_PropertyFilterSpec *propertyFilterSpec = NULL; - if (!objectContentList || *objectContentList) { - virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("Invalid argument")); - return -1; - } + esxVI_checkArgList(objectContentList); if (esxVI_ObjectSpec_Alloc(&objectSpec) < 0) return -1; @@ -2372,10 +2369,7 @@ esxVI_GetVirtualMachineQuestionInfo { esxVI_DynamicProperty *dynamicProperty; - if (!questionInfo || *questionInfo) { - virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("Invalid argument")); - return -1; - } + esxVI_checkArgList(questionInfo); for (dynamicProperty = virtualMachine->propSet; dynamicProperty; dynamicProperty = dynamicProperty->_next) { @@ -2447,10 +2441,7 @@ esxVI_GetInt(esxVI_ObjectContent *objectContent, const char *propertyName, { esxVI_DynamicProperty *dynamicProperty; - if (!value || *value) { - virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("Invalid argument")); - return -1; - } + esxVI_checkArgList(value); for (dynamicProperty = objectContent->propSet; dynamicProperty; dynamicProperty = dynamicProperty->_next) { @@ -2479,10 +2470,7 @@ esxVI_GetLong(esxVI_ObjectContent *objectContent, const char *propertyName, { esxVI_DynamicProperty *dynamicProperty; - if (!value || *value) { - virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("Invalid argument")); - return -1; - } + esxVI_checkArgList(value); for (dynamicProperty = objectContent->propSet; dynamicProperty; dynamicProperty = dynamicProperty->_next) { @@ -2512,10 +2500,7 @@ esxVI_GetStringValue(esxVI_ObjectContent *objectContent, { esxVI_DynamicProperty *dynamicProperty; - if (!value || *value) { - virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("Invalid argument")); - return -1; - } + esxVI_checkArgList(value); for (dynamicProperty = objectContent->propSet; dynamicProperty; dynamicProperty = dynamicProperty->_next) { @@ -2549,10 +2534,7 @@ esxVI_GetManagedObjectReference(esxVI_ObjectContent *objectContent, { esxVI_DynamicProperty *dynamicProperty; - if (!value || *value) { - virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("Invalid argument")); - return -1; - } + esxVI_checkArgList(value); for (dynamicProperty = objectContent->propSet; dynamicProperty; dynamicProperty = dynamicProperty->_next) { @@ -2863,10 +2845,7 @@ esxVI_GetSnapshotTreeBySnapshot { esxVI_VirtualMachineSnapshotTree *candidate; - if (!snapshotTree || *snapshotTree) { - virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("Invalid argument")); - return -1; - } + esxVI_checkArgList(snapshotTree); for (candidate = snapshotTreeList; candidate; candidate = candidate->_next) { @@ -2928,10 +2907,7 @@ esxVI_LookupVirtualMachineByUuid(esxVI_Context *ctx, const unsigned char *uuid, esxVI_ManagedObjectReference *managedObjectReference = NULL; char uuid_string[VIR_UUID_STRING_BUFLEN] = ""; - if (!virtualMachine || *virtualMachine) { - virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("Invalid argument")); - return -1; - } + esxVI_checkArgList(virtualMachine); virUUIDFormat(uuid, uuid_string); @@ -2983,10 +2959,7 @@ esxVI_LookupVirtualMachineByName(esxVI_Context *ctx, const char *name, esxVI_ObjectContent *candidate = NULL; char *name_candidate = NULL; - if (!virtualMachine || *virtualMachine) { - virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("Invalid argument")); - return -1; - } + esxVI_checkArgList(virtualMachine); if (esxVI_String_DeepCopyList(&completePropertyNameList, propertyNameList) < 0 || @@ -3110,10 +3083,7 @@ esxVI_LookupDatastoreByName(esxVI_Context *ctx, const char *name, esxVI_ObjectContent *candidate = NULL; char *name_candidate; - if (!datastore || *datastore) { - virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("Invalid argument")); - return -1; - } + esxVI_checkArgList(datastore); /* Get all datastores */ if (esxVI_String_DeepCopyList(&completePropertyNameList, @@ -3177,10 +3147,7 @@ esxVI_LookupDatastoreByAbsolutePath(esxVI_Context *ctx, esxVI_DatastoreHostMount *datastoreHostMountList = NULL; esxVI_DatastoreHostMount *datastoreHostMount = NULL; - if (!datastore || *datastore) { - virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("Invalid argument")); - return -1; - } + esxVI_checkArgList(datastore); /* Get all datastores */ if (esxVI_String_DeepCopyList(&completePropertyNameList, @@ -3263,10 +3230,7 @@ esxVI_LookupDatastoreHostMount(esxVI_Context *ctx, esxVI_DatastoreHostMount *hostMountList = NULL; esxVI_DatastoreHostMount *candidate = NULL; - if (!hostMount || *hostMount) { - virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("Invalid argument")); - return -1; - } + esxVI_checkArgList(hostMount); if (esxVI_String_AppendValueToList(&propertyNameList, "host") < 0 || esxVI_LookupObjectContentByType(ctx, datastore, "Datastore", @@ -3327,10 +3291,7 @@ esxVI_LookupTaskInfoByTask(esxVI_Context *ctx, esxVI_ObjectContent *objectContent = NULL; esxVI_DynamicProperty *dynamicProperty = NULL; - if (!taskInfo || *taskInfo) { - virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("Invalid argument")); - return -1; - } + esxVI_checkArgList(taskInfo); if (esxVI_String_AppendValueToList(&propertyNameList, "info") < 0 || esxVI_LookupObjectContentByType(ctx, task, "Task", propertyNameList, @@ -3375,10 +3336,7 @@ esxVI_LookupPendingTaskInfoListByVirtualMachine esxVI_DynamicProperty *dynamicProperty = NULL; esxVI_TaskInfo *taskInfo = NULL; - if (!pendingTaskInfoList || *pendingTaskInfoList) { - virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("Invalid argument")); - return -1; - } + esxVI_checkArgList(pendingTaskInfoList); /* Get list of recent tasks */ for (dynamicProperty = virtualMachine->propSet; dynamicProperty; @@ -3481,10 +3439,7 @@ esxVI_LookupRootSnapshotTreeList esxVI_ObjectContent *virtualMachine = NULL; esxVI_DynamicProperty *dynamicProperty = NULL; - if (!rootSnapshotTreeList || *rootSnapshotTreeList) { - virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("Invalid argument")); - return -1; - } + esxVI_checkArgList(rootSnapshotTreeList); if (esxVI_String_AppendValueToList(&propertyNameList, "snapshot.rootSnapshotList") < 0 || @@ -3536,10 +3491,7 @@ esxVI_LookupCurrentSnapshotTree esxVI_VirtualMachineSnapshotTree *rootSnapshotTreeList = NULL; esxVI_VirtualMachineSnapshotTree *snapshotTree = NULL; - if (!currentSnapshotTree || *currentSnapshotTree) { - virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("Invalid argument")); - return -1; - } + esxVI_checkArgList(currentSnapshotTree); if (esxVI_String_AppendValueListToList(&propertyNameList, "snapshot.currentSnapshot\0" @@ -3633,10 +3585,7 @@ esxVI_LookupFileInfoByDatastorePath(esxVI_Context *ctx, esxVI_TaskInfo *taskInfo = NULL; esxVI_HostDatastoreBrowserSearchResults *searchResults = NULL; - if (!fileInfo || *fileInfo) { - virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("Invalid argument")); - return -1; - } + esxVI_checkArgList(fileInfo); if (esxUtil_ParseDatastorePath(datastorePath, &datastoreName, &directoryName, &directoryAndFileName) < 0) { @@ -3829,10 +3778,7 @@ esxVI_LookupDatastoreContentByDatastoreName char *taskInfoErrorMessage = NULL; esxVI_TaskInfo *taskInfo = NULL; - if (!searchResultsList || *searchResultsList) { - virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("Invalid argument")); - return -1; - } + esxVI_checkArgList(searchResultsList); /* Lookup Datastore and HostDatastoreBrowser */ if (esxVI_String_AppendValueToList(&propertyNameList, "browser") < 0 || @@ -3941,10 +3887,7 @@ esxVI_LookupStorageVolumeKeyByDatastorePath(esxVI_Context *ctx, esxVI_FileInfo *fileInfo = NULL; char *uuid_string = NULL; - if (!key || *key) { - virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("Invalid argument")); - return -1; - } + esxVI_checkArgList(key); if (ctx->hasQueryVirtualDiskUuid) { if (esxVI_LookupFileInfoByDatastorePath @@ -3995,10 +3938,7 @@ esxVI_LookupAutoStartDefaults(esxVI_Context *ctx, esxVI_ObjectContent *hostAutoStartManager = NULL; esxVI_DynamicProperty *dynamicProperty = NULL; - if (!defaults || *defaults) { - virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("Invalid argument")); - return -1; - } + esxVI_checkArgList(defaults); /* * Lookup HostAutoStartManagerConfig from the HostAutoStartManager because @@ -4052,10 +3992,7 @@ esxVI_LookupAutoStartPowerInfoList(esxVI_Context *ctx, esxVI_ObjectContent *hostAutoStartManager = NULL; esxVI_DynamicProperty *dynamicProperty = NULL; - if (!powerInfoList || *powerInfoList) { - virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("Invalid argument")); - return -1; - } + esxVI_checkArgList(powerInfoList); /* * Lookup HostAutoStartManagerConfig from the HostAutoStartManager because @@ -4103,10 +4040,7 @@ esxVI_LookupPhysicalNicList(esxVI_Context *ctx, esxVI_ObjectContent *hostSystem = NULL; esxVI_DynamicProperty *dynamicProperty = NULL; - if (!physicalNicList || *physicalNicList) { - virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("Invalid argument")); - return -1; - } + esxVI_checkArgList(physicalNicList); if (esxVI_String_AppendValueToList(&propertyNameList, "config.network.pnic") < 0 || @@ -4147,10 +4081,7 @@ esxVI_LookupPhysicalNicByName(esxVI_Context *ctx, const char *name, esxVI_PhysicalNic *physicalNicList = NULL; esxVI_PhysicalNic *candidate = NULL; - if (!physicalNic || *physicalNic) { - virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("Invalid argument")); - return -1; - } + esxVI_checkArgList(physicalNic); if (esxVI_LookupPhysicalNicList(ctx, &physicalNicList) < 0) goto cleanup; @@ -4194,10 +4125,7 @@ esxVI_LookupPhysicalNicByMACAddress(esxVI_Context *ctx, const char *mac, esxVI_PhysicalNic *physicalNicList = NULL; esxVI_PhysicalNic *candidate = NULL; - if (!physicalNic || *physicalNic) { - virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("Invalid argument")); - return -1; - } + esxVI_checkArgList(physicalNic); if (esxVI_LookupPhysicalNicList(ctx, &physicalNicList) < 0) goto cleanup; @@ -4241,10 +4169,7 @@ esxVI_LookupHostVirtualSwitchList(esxVI_Context *ctx, esxVI_ObjectContent *hostSystem = NULL; esxVI_DynamicProperty *dynamicProperty = NULL; - if (!hostVirtualSwitchList || *hostVirtualSwitchList) { - virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("Invalid argument")); - return -1; - } + esxVI_checkArgList(hostVirtualSwitchList); if (esxVI_String_AppendValueToList(&propertyNameList, "config.network.vswitch") < 0 || @@ -4285,10 +4210,7 @@ esxVI_LookupHostVirtualSwitchByName(esxVI_Context *ctx, const char *name, esxVI_HostVirtualSwitch *hostVirtualSwitchList = NULL; esxVI_HostVirtualSwitch *candidate = NULL; - if (!hostVirtualSwitch || *hostVirtualSwitch) { - virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("Invalid argument")); - return -1; - } + esxVI_checkArgList(hostVirtualSwitch); if (esxVI_LookupHostVirtualSwitchList(ctx, &hostVirtualSwitchList) < 0) goto cleanup; @@ -4336,10 +4258,7 @@ esxVI_LookupHostPortGroupList(esxVI_Context *ctx, esxVI_ObjectContent *hostSystem = NULL; esxVI_DynamicProperty *dynamicProperty = NULL; - if (!hostPortGroupList || *hostPortGroupList) { - virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("Invalid argument")); - return -1; - } + esxVI_checkArgList(hostPortGroupList); if (esxVI_String_AppendValueToList(&propertyNameList, "config.network.portgroup") < 0 || @@ -4513,10 +4432,7 @@ esxVI_WaitForTaskCompletion(esxVI_Context *ctx, bool blocked; esxVI_TaskInfo *taskInfo = NULL; - if (!errorMessage || *errorMessage) { - virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("Invalid argument")); - return -1; - } + esxVI_checkArgList(errorMessage); if (VIR_STRDUP(version, "") < 0) return -1; @@ -4982,10 +4898,7 @@ esxVI_LookupHostScsiTopologyLunListByTargetName bool found = false; esxVI_HostInternetScsiTargetTransport *candidate = NULL; - if (!hostScsiTopologyLunList || *hostScsiTopologyLunList) { - virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("Invalid argument")); - return -1; - } + esxVI_checkArgList(hostScsiTopologyLunList); if (esxVI_String_AppendValueToList (&propertyNameList, @@ -5076,10 +4989,7 @@ esxVI_LookupStoragePoolNameByScsiLunKey(esxVI_Context *ctx, esxVI_HostScsiTopologyLun *hostScsiTopologyLun; bool found = false; - if (!poolName || *poolName) { - virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("Invalid argument")); - return -1; - } + esxVI_checkArgList(poolName); if (esxVI_String_AppendValueToList (&propertyNameList, @@ -5207,11 +5117,7 @@ esxVI_LookupStoragePoolNameByScsiLunKey(esxVI_Context *ctx, esxVI_ObjectContent *objectContentList = NULL; \ esxVI_DynamicProperty *dynamicProperty = NULL; \ \ - if (!ptrptr || *ptrptr) { \ - virReportError(VIR_ERR_INTERNAL_ERROR, "%s", \ - _("Invalid argument")); \ - return -1; \ - } \ + esxVI_checkArgList(ptrptr); \ \ propertyNameList = selectedPropertyNameList; \ \ @@ -5287,11 +5193,8 @@ esxVI_LookupManagedObjectHelper(esxVI_Context *ctx, esxVI_ObjectContent *candidate = NULL; char *name_candidate; - if (!objectContent || *objectContent || - !objectContentList || *objectContentList) { - virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("Invalid argument")); - return -1; - } + esxVI_checkArgList(objectContent); + esxVI_checkArgList(objectContentList); if (!esxVI_String_ListContainsValue(propertyNameList, "name")) { virReportError(VIR_ERR_INTERNAL_ERROR, -- 2.17.1
On 07/03/2018 04:20 AM, Marcos Paulo de Souza wrote:
This macro avoids code duplication when checking for arrays of objects.
Signed-off-by: Marcos Paulo de Souza <marcos.souza.org@gmail.com> --- src/esx/esx_vi.c | 189 ++++++++++++----------------------------------- 1 file changed, 46 insertions(+), 143 deletions(-)
diff --git a/src/esx/esx_vi.c b/src/esx/esx_vi.c index 25fbdc7e44..212300dbff 100644 --- a/src/esx/esx_vi.c +++ b/src/esx/esx_vi.c @@ -41,6 +41,16 @@
VIR_LOG_INIT("esx.esx_vi");
+#define esxVI_checkArgList(val) \ + do { \ + if (!val || *val) { \ + virReportError(VIR_ERR_INVALID_ARG, "%s", _("Invalid argument")); \
Actually, this is one of the few places where VIR_ERR_INTERNAL_ERROR is not misused and makes sense. The only way how this error can be reported is if there's a bug in our code, not because of some input user made. There are also other occurrences of this pattern. coccinelle helped to find other. I used the following spatch: @@ identifier ptr; @@ - (!ptr || *ptr) + (esxVI_checkArgList(ptr)) Mind putting them here too? Otherwise the patch looks good. Michal
On Tue, Jul 03, 2018 at 11:31:36AM +0200, Michal Prívozník wrote:
On 07/03/2018 04:20 AM, Marcos Paulo de Souza wrote:
This macro avoids code duplication when checking for arrays of objects.
Signed-off-by: Marcos Paulo de Souza <marcos.souza.org@gmail.com> --- src/esx/esx_vi.c | 189 ++++++++++++----------------------------------- 1 file changed, 46 insertions(+), 143 deletions(-)
diff --git a/src/esx/esx_vi.c b/src/esx/esx_vi.c index 25fbdc7e44..212300dbff 100644 --- a/src/esx/esx_vi.c +++ b/src/esx/esx_vi.c @@ -41,6 +41,16 @@
VIR_LOG_INIT("esx.esx_vi");
+#define esxVI_checkArgList(val) \ + do { \ + if (!val || *val) { \ + virReportError(VIR_ERR_INVALID_ARG, "%s", _("Invalid argument")); \
Actually, this is one of the few places where VIR_ERR_INTERNAL_ERROR is not misused and makes sense. The only way how this error can be reported is if there's a bug in our code, not because of some input user made.
There are also other occurrences of this pattern. coccinelle helped to find other. I used the following spatch:
@@ identifier ptr; @@
- (!ptr || *ptr) + (esxVI_checkArgList(ptr))
Mind putting them here too? Otherwise the patch looks good.
No problem. I will take a look into coccinele and resend the patch once all other places are covered. Thanks for pushing the other patches and fixing the code style.
Michal
-- Thanks, Marcos
2018-07-03 4:20 GMT+02:00 Marcos Paulo de Souza <marcos.souza.org@gmail.com>:
This macro avoids code duplication when checking for arrays of objects.
Signed-off-by: Marcos Paulo de Souza <marcos.souza.org@gmail.com> --- src/esx/esx_vi.c | 189 ++++++++++++----------------------------------- 1 file changed, 46 insertions(+), 143 deletions(-)
diff --git a/src/esx/esx_vi.c b/src/esx/esx_vi.c index 25fbdc7e44..212300dbff 100644 --- a/src/esx/esx_vi.c +++ b/src/esx/esx_vi.c @@ -41,6 +41,16 @@
VIR_LOG_INIT("esx.esx_vi");
+#define esxVI_checkArgList(val) \ + do { \ + if (!val || *val) { \ + virReportError(VIR_ERR_INVALID_ARG, "%s", _("Invalid argument")); \ + return -1; \ + } \ + } while (0)
As this is a macro I suggest naming it ESX_VI_CHECK_ARG_LIST instead of esxVI_checkArgList. Because at the moment the ESX code makes a clear distinction between macros and functions in their spelling. I'd like to keep it that way, even if the rest of the code base is no consistent about this. -- Matthias Bolte http://photron.blogspot.com
Instead of duplicating code to do the same checking. Now all functions of virHypervisorDriver from esx driver are using this macro. Signed-off-by: Marcos Paulo de Souza <marcos.souza.org@gmail.com> --- src/esx/esx_driver.c | 10 ++-------- 1 file changed, 2 insertions(+), 8 deletions(-) diff --git a/src/esx/esx_driver.c b/src/esx/esx_driver.c index 60aa5fc252..705b0d1a41 100644 --- a/src/esx/esx_driver.c +++ b/src/esx/esx_driver.c @@ -2488,10 +2488,7 @@ esxDomainSetVcpusFlags(virDomainPtr domain, unsigned int nvcpus, esxVI_TaskInfoState taskInfoState; char *taskInfoErrorMessage = NULL; - if (flags != VIR_DOMAIN_AFFECT_LIVE) { - virReportError(VIR_ERR_INVALID_ARG, _("unsupported flags: (0x%x)"), flags); - return -1; - } + virCheckFlags(VIR_DOMAIN_AFFECT_LIVE, -1); if (nvcpus < 1) { virReportError(VIR_ERR_INVALID_ARG, "%s", @@ -2570,10 +2567,7 @@ esxDomainGetVcpusFlags(virDomainPtr domain, unsigned int flags) esxVI_ObjectContent *hostSystem = NULL; esxVI_DynamicProperty *dynamicProperty = NULL; - if (flags != (VIR_DOMAIN_AFFECT_LIVE | VIR_DOMAIN_VCPU_MAXIMUM)) { - virReportError(VIR_ERR_INVALID_ARG, _("unsupported flags: (0x%x)"), flags); - return -1; - } + virCheckFlags(VIR_DOMAIN_AFFECT_LIVE | VIR_DOMAIN_VCPU_MAXIMUM, -1); if (priv->maxVcpus > 0) return priv->maxVcpus; -- 2.17.1
On 07/03/2018 04:21 AM, Marcos Paulo de Souza wrote:
Instead of duplicating code to do the same checking. Now all functions of virHypervisorDriver from esx driver are using this macro.
Signed-off-by: Marcos Paulo de Souza <marcos.souza.org@gmail.com> --- src/esx/esx_driver.c | 10 ++-------- 1 file changed, 2 insertions(+), 8 deletions(-)
diff --git a/src/esx/esx_driver.c b/src/esx/esx_driver.c index 60aa5fc252..705b0d1a41 100644 --- a/src/esx/esx_driver.c +++ b/src/esx/esx_driver.c @@ -2488,10 +2488,7 @@ esxDomainSetVcpusFlags(virDomainPtr domain, unsigned int nvcpus, esxVI_TaskInfoState taskInfoState; char *taskInfoErrorMessage = NULL;
- if (flags != VIR_DOMAIN_AFFECT_LIVE) { - virReportError(VIR_ERR_INVALID_ARG, _("unsupported flags: (0x%x)"), flags); - return -1; - } + virCheckFlags(VIR_DOMAIN_AFFECT_LIVE, -1);
if (nvcpus < 1) { virReportError(VIR_ERR_INVALID_ARG, "%s", @@ -2570,10 +2567,7 @@ esxDomainGetVcpusFlags(virDomainPtr domain, unsigned int flags) esxVI_ObjectContent *hostSystem = NULL; esxVI_DynamicProperty *dynamicProperty = NULL;
- if (flags != (VIR_DOMAIN_AFFECT_LIVE | VIR_DOMAIN_VCPU_MAXIMUM)) { - virReportError(VIR_ERR_INVALID_ARG, _("unsupported flags: (0x%x)"), flags); - return -1; - } + virCheckFlags(VIR_DOMAIN_AFFECT_LIVE | VIR_DOMAIN_VCPU_MAXIMUM, -1);
We tend to put one flag per line as it helps us size down changes needed when adding new flag (= patches are smaller). Fixed, ACKed and pushed. Michal
On 07/03/2018 04:20 AM, Marcos Paulo de Souza wrote:
The same pattern is used in lots of other places.
Signed-off-by: Marcos Paulo de Souza <marcos.souza.org@gmail.com> --- src/esx/esx_vi.c | 14 ++++---------- 1 file changed, 4 insertions(+), 10 deletions(-)
diff --git a/src/esx/esx_vi.c b/src/esx/esx_vi.c index 43ff7ea048..25fbdc7e44 100644 --- a/src/esx/esx_vi.c +++ b/src/esx/esx_vi.c @@ -3014,16 +3014,10 @@ esxVI_LookupVirtualMachineByName(esxVI_Context *ctx, const char *name, break; }
- if (!(*virtualMachine)) { - if (occurrence == esxVI_Occurrence_OptionalItem) { - result = 0; - - goto cleanup; - } else { - virReportError(VIR_ERR_NO_DOMAIN, - _("Could not find domain with name '%s'"), name); - goto cleanup; - } + if (!(*virtualMachine) && occurrence != esxVI_Occurrence_OptionalItem) { + virReportError(VIR_ERR_NO_DOMAIN, + _("Could not find domain with name '%s'"), name); + goto cleanup; }
I think this error message should be dropped. Firstly, this function is called from esxDomainDefineXMLFlags() where it is used to check if a domain with the same name as in provided XML does not exist. If it doesn't this function reports an error which is undesired. Secondly, every caller checks for virtualMachine == NULL and reports their own error effectively overwriting this one. Thirdly, this function is supposed to act like virDomainObjListFindByName() which doesn't report error either. ACK with this squashed in: diff --git i/src/esx/esx_vi.c w/src/esx/esx_vi.c index 25fbdc7e44..0bdfc5a8be 100644 --- i/src/esx/esx_vi.c +++ w/src/esx/esx_vi.c @@ -3014,11 +3014,8 @@ esxVI_LookupVirtualMachineByName(esxVI_Context *ctx, const char *name, break; } - if (!(*virtualMachine) && occurrence != esxVI_Occurrence_OptionalItem) { - virReportError(VIR_ERR_NO_DOMAIN, - _("Could not find domain with name '%s'"), name); + if (!(*virtualMachine) && occurrence != esxVI_Occurrence_OptionalItem) goto cleanup; - } result = 0; And pushed. Michal
2018-07-03 11:31 GMT+02:00 Michal Prívozník <mprivozn@redhat.com>:
On 07/03/2018 04:20 AM, Marcos Paulo de Souza wrote:
The same pattern is used in lots of other places.
Signed-off-by: Marcos Paulo de Souza <marcos.souza.org@gmail.com> --- src/esx/esx_vi.c | 14 ++++---------- 1 file changed, 4 insertions(+), 10 deletions(-)
diff --git a/src/esx/esx_vi.c b/src/esx/esx_vi.c index 43ff7ea048..25fbdc7e44 100644 --- a/src/esx/esx_vi.c +++ b/src/esx/esx_vi.c @@ -3014,16 +3014,10 @@ esxVI_LookupVirtualMachineByName(esxVI_Context *ctx, const char *name, break; }
- if (!(*virtualMachine)) { - if (occurrence == esxVI_Occurrence_OptionalItem) { - result = 0; - - goto cleanup; - } else { - virReportError(VIR_ERR_NO_DOMAIN, - _("Could not find domain with name '%s'"), name); - goto cleanup; - } + if (!(*virtualMachine) && occurrence != esxVI_Occurrence_OptionalItem) { + virReportError(VIR_ERR_NO_DOMAIN, + _("Could not find domain with name '%s'"), name); + goto cleanup; }
I think this error message should be dropped. Firstly, this function is called from esxDomainDefineXMLFlags() where it is used to check if a domain with the same name as in provided XML does not exist. If it doesn't this function reports an error which is undesired.
Secondly, every caller checks for virtualMachine == NULL and reports their own error effectively overwriting this one.
Thirdly, this function is supposed to act like virDomainObjListFindByName() which doesn't report error either.
ACK with this squashed in:
diff --git i/src/esx/esx_vi.c w/src/esx/esx_vi.c index 25fbdc7e44..0bdfc5a8be 100644 --- i/src/esx/esx_vi.c +++ w/src/esx/esx_vi.c @@ -3014,11 +3014,8 @@ esxVI_LookupVirtualMachineByName(esxVI_Context *ctx, const char *name, break; }
- if (!(*virtualMachine) && occurrence != esxVI_Occurrence_OptionalItem) { - virReportError(VIR_ERR_NO_DOMAIN, - _("Could not find domain with name '%s'"), name); + if (!(*virtualMachine) && occurrence != esxVI_Occurrence_OptionalItem) goto cleanup; - }
result = 0;
And pushed.
The original patch was fine, your modification is wrong. The error message needs to stay. You didn't follow the logic closely enough. The caller of esxVI_LookupVirtualMachineByName specified the expected occurrence. esxDomainDefineXMLFlags uses this to test if the domain exists and calls esxVI_LookupVirtualMachineByName with occurrence OptionalItem. If the domain is missing no error is reported by esxVI_LookupVirtualMachineByName, because of occurrence OptionalItem. esxDomainLookupByName uses this to find a domain with the given name. It also uses occurrence OptionalItem. Here you argue that the caller checks for virtualMachine == NULL anyway. But no error is overwritten here because esxVI_LookupVirtualMachineByName doesn't report an error with occurrence OptionalItem. The actual point that could be made here is that esxDomainLookupByName should actually use occurrence RequiredItem instead of doing the virtualMachine == NULL check itself. Also esxVI_LookupVirtualMachineByUuid has the same logic that got simplified in the original patch, but again, the error reporting there is correct as well and needs to stay. -- Matthias Bolte http://photron.blogspot.com
On 07/03/2018 03:43 PM, Matthias Bolte wrote:
2018-07-03 11:31 GMT+02:00 Michal Prívozník <mprivozn@redhat.com>:
On 07/03/2018 04:20 AM, Marcos Paulo de Souza wrote:
The same pattern is used in lots of other places.
Signed-off-by: Marcos Paulo de Souza <marcos.souza.org@gmail.com> --- src/esx/esx_vi.c | 14 ++++---------- 1 file changed, 4 insertions(+), 10 deletions(-)
diff --git a/src/esx/esx_vi.c b/src/esx/esx_vi.c index 43ff7ea048..25fbdc7e44 100644 --- a/src/esx/esx_vi.c +++ b/src/esx/esx_vi.c @@ -3014,16 +3014,10 @@ esxVI_LookupVirtualMachineByName(esxVI_Context *ctx, const char *name, break; }
- if (!(*virtualMachine)) { - if (occurrence == esxVI_Occurrence_OptionalItem) { - result = 0; - - goto cleanup; - } else { - virReportError(VIR_ERR_NO_DOMAIN, - _("Could not find domain with name '%s'"), name); - goto cleanup; - } + if (!(*virtualMachine) && occurrence != esxVI_Occurrence_OptionalItem) { + virReportError(VIR_ERR_NO_DOMAIN, + _("Could not find domain with name '%s'"), name); + goto cleanup; }
I think this error message should be dropped. Firstly, this function is called from esxDomainDefineXMLFlags() where it is used to check if a domain with the same name as in provided XML does not exist. If it doesn't this function reports an error which is undesired.
Secondly, every caller checks for virtualMachine == NULL and reports their own error effectively overwriting this one.
Thirdly, this function is supposed to act like virDomainObjListFindByName() which doesn't report error either.
ACK with this squashed in:
diff --git i/src/esx/esx_vi.c w/src/esx/esx_vi.c index 25fbdc7e44..0bdfc5a8be 100644 --- i/src/esx/esx_vi.c +++ w/src/esx/esx_vi.c @@ -3014,11 +3014,8 @@ esxVI_LookupVirtualMachineByName(esxVI_Context *ctx, const char *name, break; }
- if (!(*virtualMachine) && occurrence != esxVI_Occurrence_OptionalItem) { - virReportError(VIR_ERR_NO_DOMAIN, - _("Could not find domain with name '%s'"), name); + if (!(*virtualMachine) && occurrence != esxVI_Occurrence_OptionalItem) goto cleanup; - }
result = 0;
And pushed.
The original patch was fine, your modification is wrong. The error message needs to stay. You didn't follow the logic closely enough. The caller of esxVI_LookupVirtualMachineByName specified the expected occurrence.
esxDomainDefineXMLFlags uses this to test if the domain exists and calls esxVI_LookupVirtualMachineByName with occurrence OptionalItem. If the domain is missing no error is reported by esxVI_LookupVirtualMachineByName, because of occurrence OptionalItem.
esxDomainLookupByName uses this to find a domain with the given name. It also uses occurrence OptionalItem. Here you argue that the caller checks for virtualMachine == NULL anyway. But no error is overwritten here because esxVI_LookupVirtualMachineByName doesn't report an error with occurrence OptionalItem. The actual point that could be made here is that esxDomainLookupByName should actually use occurrence RequiredItem instead of doing the virtualMachine == NULL check itself.
Also esxVI_LookupVirtualMachineByUuid has the same logic that got simplified in the original patch, but again, the error reporting there is correct as well and needs to stay.
D'oh. Well, so far all (as in both) callers pass OptionalItem so no real harm done. But sure, I'll post a patch to fix my mess. Michal
participants (3)
-
Marcos Paulo de Souza -
Matthias Bolte -
Michal Prívozník