Re: [libvirt] [PATCH 2/3] esx: Add esxVI_checkArgList macro

[Matthias Bolte]

2018-07-03 4:20 GMT+02:00 Marcos Paulo de Souza :
> This macro avoids code duplication when checking for arrays of objects.
>
> Signed-off-by: Marcos Paulo de Souza 
> ---
>  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

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH 2/3] esx: Add esxVI_checkArgList macro

[Marcos Paulo de Souza]

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 
> > ---
> >  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

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH 2/3] esx: Add esxVI_checkArgList macro

[Michal Prívozník]

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 
> ---
>  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

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

[libvirt] [PATCH 2/3] esx: Add esxVI_checkArgList macro

[Marcos Paulo de Souza]

This macro avoids code duplication when checking for arrays of objects.

Signed-off-by: Marcos Paulo de Souza 
---
 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(, 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() < 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)