Re: [Libvirt-cim] [PATCH] [CU][RFC](#2) Added lexer/parser support for string array properties

1 Jun 2009

Thanks Richard,

This looks really good.
...
diff -r 1cb3975921d5 -r 0b79dd93e4ed eo_util_parser.y

--- a/eo_util_parser.y	Mon Apr 27 16:11:32 2009 -0700
+++ b/eo_util_parser.y	Sat May 30 14:03:04 2009 -0300
...
@@ -20,15 +21,24 @@
     /* specify prototypes to get rid of warnings */
 int eo_parse_lex (void);
 void eo_parse_error(char *);
+inline void ins_chars_into_cmstr_arr(const CMPIBroker *broker,
+                                     CMPIArray *arr,
+                                     CMPICount index,
+                                     char *str);
#define RC_OK 0
 #define RC_EOF EOF
 #define RC_INVALID_CLASS -1000
+#define EODEBUG 1
Be sure to remove this when you resubmit.
...
/* DEFINE ANY GLOBAL VARS HERE */
 static const CMPIBroker * _BROKER;
 static CMPIInstance ** _INSTANCE;
 static const char * _NAMESPACE;
+static CMPICount stringarraysize;
+static char **stringarray;
+static char *stringarraypropname;
+

...
@@ -127,7 +138,17 @@
      	CMSetProperty(*_INSTANCE, $1, &($3), CMPI_boolean);
      	free($1);
      	}
+        |       PROPERTYNAME '=' OPENBRACKET
+                        {
+                        EOTRACE("propertyname = %s\n"
+				"\ttype = CMPI_charsA\n",
+				$1);
You assume the array will be a character array.  What about 
CMPI_uint16A, etc?

I think it's fine to not address integer types in this patch - handling 
arrays of string is complex enough.  Nut we should fix it in a follow up 
patch.
...
+                        stringarraysize = 0;
+                        stringarraypropname = $1;
+                        }
+                arrayofstrings CLOSEBRACKET ';'
+
  |	PROPERTYNAME '=' CIMNULL ';'
      	{
      	EOTRACE("propertyname = %s\n"

...
@@ -136,11 +157,86 @@
      	}
  ;
+arrayofstrings: STRING 
+                        {
+                        EOTRACE("BootDevices[%u]=%s\n",stringarraysize, $1);
Make this trace statement more generic.
...
+
+                        stringarraysize++;
+                        stringarray = (char **)realloc(stringarray, 
+                                                       sizeof(char *) * 
+                                                       stringarraysize);
+                        stringarray[stringarraysize-1] = $1;
+                        }
+                COMMA arrayofstrings
+
+              
+        |       STRING
+                        {
+                        CMPIArray *arr;
+                        CMPICount i;
+                        CMPIStatus s;
+                        
+                        EOTRACE("\tBootDevices[%u]=%s\n",stringarraysize, $1);
Same here.
...
+                        
+                        stringarraysize++;
+
+                        arr = CMNewArray(_BROKER, 
+                                         stringarraysize, 
+                                         CMPI_string, 
+                                         &s);
+                        if (s.rc != CMPI_RC_OK || CMIsNullObject(arr))
+                                EOTRACE("Error creating array\n");
I would also return from here.  Otherwise, attempting to add values into 
the array will fail later on.
...
+
+                        // Values to stringarraysize - 2 are in the
+                        // temporary array
+                        for (i = 0; i < stringarraysize - 1; i++) {
+                                ins_chars_into_cmstr_arr(_BROKER,
+                                                         arr,
+                                                         i,
+                                                         stringarray[i]);
Trap any kind of error here and return.
...
+
+                                free(stringarray[i]);
+                        }
+                        
+                        ins_chars_into_cmstr_arr(_BROKER, 
+                                                 arr,  
+                                                 stringarraysize - 1,
+                                                 $1);
Same here.
...
+
+                        free($1);
+
+                        CMSetProperty(*_INSTANCE, 
+                                      stringarraypropname,
+                                      &arr,
+                                      CMPI_stringA);
+                        
+                        free(stringarraypropname);
+                        }
+        ;
+
 /* END OF RULES SECTION */
 %%
/* USER SUBROUTINE SECTION */
+inline void ins_chars_into_cmstr_arr(const CMPIBroker *broker,
+                                     CMPIArray *arr,
+                                     CMPICount index,
+                                     char *str)
Instead of including this function in eo_util_parser.y, I would put it 
in eo_parser.c.  This is similar to set_int_prop() - it's called in 
set_int_prop, but defined in eo_parser.c.
...
+{
+        CMPIString *cm_str;
+        CMPIStatus s;
+        
+        cm_str = CMNewString(_BROKER, str, &s);
+        if (s.rc != CMPI_RC_OK || CMIsNullObject(cm_str))
+                EOTRACE("Error creating CMPIString");
I would return an error here.  Instead of EOTRACE, CU_DEBUG().
...
+
+        s = CMSetArrayElementAt(arr, index, &cm_str, CMPI_string);
+        if (s.rc != CMPI_RC_OK)
+                EOTRACE("Error setting array element %u\n"
+                        "Error code: %d\n", index, s.rc);
Same here.

-- 
Kaitlin Rupert
IBM Linux Technology Center
kaitlin@linux.vnet.ibm.com

    