9:20 p.m.
The same pattern is used in lots of other places.
Signed-off-by: Marcos Paulo de Souza <marcos.souza.org(a)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
9:20 p.m.
New subject: [libvirt] [PATCH 2/3] esx: Add esxVI_checkArgList macro
This macro avoids code duplication when checking for arrays of objects.
Signed-off-by: Marcos Paulo de Souza <marcos.souza.org(a)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
4:31 a.m.
New subject: [libvirt] [PATCH 2/3] esx: Add esxVI_checkArgList macro
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(a)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
5:30 a.m.
New subject: [libvirt] [PATCH 2/3] esx: Add esxVI_checkArgList macro
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(a)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
8:48 a.m.
New subject: [libvirt] [PATCH 2/3] esx: Add esxVI_checkArgList macro
2018-07-03 4:20 GMT+02:00 Marcos Paulo de Souza <marcos.souza.org(a)gmail.com>:
This macro avoids code duplication when checking for arrays of
objects.
Signed-off-by: Marcos Paulo de Souza <marcos.souza.org(a)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
9:21 p.m.
New subject: [libvirt] [PATCH 3/3] esx_driver: Use virCheckFlag macro
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(a)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
4:31 a.m.
New subject: [libvirt] [PATCH 3/3] esx_driver: Use virCheckFlag macro
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(a)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
4:31 a.m.
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(a)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
8:43 a.m.
2018-07-03 11:31 GMT+02:00 Michal Prívozník <mprivozn(a)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(a)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
9:46 a.m.
On 07/03/2018 03:43 PM, Matthias Bolte wrote:
2018-07-03 11:31 GMT+02:00 Michal Prívozník
<mprivozn(a)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(a)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
2334
days inactive
2334
days old
9 comments
3 participants
participants (3)
-
Marcos Paulo de Souza -
Matthias Bolte -
Michal Prívozník