Plug some memory leaks, and eliminate some unnecessary
authorscottmk <scottmk@dcc99617-32d9-48b4-a31d-7c20da2025e4>
Tue, 25 May 2010 16:21:56 +0000 (16:21 +0000)
committerscottmk <scottmk@dcc99617-32d9-48b4-a31d-7c20da2025e4>
Tue, 25 May 2010 16:21:56 +0000 (16:21 +0000)
memory allocations.

In oils_utils.[ch]:

-- Create a new function, oilsFMGetStringConst(), similar to
oilsFMGetString() except that it doesn't allocate memory; it returns
a const pointer to a string internal to the specified object.

-- Add some comments, tidy up white space.

In oils_sql.c:

-- Replace oilsFMGetString() with oilsFMGetStringConst in a number
of places; partly to reduce memory churn, and partly to plug some
memory leaks where the function call was nested within a
parameter list.

-- Change org_tree_root() so as to return a const pointer to a
static buffer (which was already in use as a cache) instead of
allocating a copy of the string.  This change reduces memory
churn.  In addition the allocated string was leaking anyway, and
now that leak is plugged.

M    Open-ILS/include/openils/oils_utils.h
M    Open-ILS/src/c-apps/oils_sql.c
M    Open-ILS/src/c-apps/oils_utils.c

git-svn-id: svn://svn.open-ils.org/ILS/trunk@16494 dcc99617-32d9-48b4-a31d-7c20da2025e4

Open-ILS/include/openils/oils_utils.h
Open-ILS/src/c-apps/oils_sql.c
Open-ILS/src/c-apps/oils_utils.c

index 3c56103..fae7e4d 100644 (file)
@@ -26,26 +26,10 @@ extern "C" {
  */
 osrfHash* oilsInitIDL( const char* idl_filename );
 
-/**
-  Returns the string value for field 'field' in the given object.
-  This method calls jsonObjectToSimpleString so numbers will be
-  returned as strings.
-  @param object The object to inspect
-  @param field The field whose value is requsted
-  @return The string at the given position, if none exists, 
-  then NULL is returned.  The caller must free the returned string
-  */
-char* oilsFMGetString( const jsonObject* object, const char* field );
+const char* oilsFMGetStringConst( const jsonObject* object, const char* field );
 
+char* oilsFMGetString( const jsonObject* object, const char* field );
 
-/**
-  Returns the jsonObject found at the specified field in the
-  given object.
-  @param object The object to inspect
-  @param field The field whose value is requsted
-  @return The found object or NULL if none exists.  Do NOT free the 
-  returned object.
-  */
 const jsonObject* oilsFMGetObject( const jsonObject* object, const char* field );
 
 /**
index c23bb94..f87483f 100644 (file)
@@ -114,7 +114,7 @@ static void clear_query_stack( void );
 
 static const jsonObject* verifyUserPCRUD( osrfMethodContext* );
 static int verifyObjectPCRUD( osrfMethodContext*, const jsonObject* );
-static char* org_tree_root( osrfMethodContext* ctx );
+static const char* org_tree_root( osrfMethodContext* ctx );
 static jsonObject* single_hash( const char* key, const char* value );
 
 static int child_initialized = 0;   /* boolean */
@@ -1302,7 +1302,7 @@ static int verifyObjectPCRUD (  osrfMethodContext* ctx, const jsonObject* obj )
        if( !user )
                return 0;    // Not logged in?  No access.
 
-       int userid = atoi( oilsFMGetString( user, "id" ) );
+       int userid = atoi( oilsFMGetStringConst( user, "id" ) );
 
        // Get a list of permissions from the permacrud entry.
        osrfStringArray* permission = osrfHashGet( pcrud, "permission" );
@@ -1325,7 +1325,7 @@ static int verifyObjectPCRUD (  osrfMethodContext* ctx, const jsonObject* obj )
        osrfStringArray* context_org_array = osrfNewStringArray( 1 );
 
        int err = 0;
-       char* pkey_value = NULL;
+       const char* pkey_value = NULL;
        if( str_is_true( osrfHashGet(pcrud, "global_required") ) ) {
                // If the global_required attribute is present and true, then the only owning
                // org unit is the root org unit, i.e. the one with no parent.
@@ -1333,7 +1333,7 @@ static int verifyObjectPCRUD (  osrfMethodContext* ctx, const jsonObject* obj )
                                "global-level permissions required, fetching top of the org tree" );
 
                // check for perm at top of org tree
-               char* org_tree_root_id = org_tree_root( ctx );
+               const char* org_tree_root_id = org_tree_root( ctx );
                if( org_tree_root_id ) {
                        osrfStringArrayAdd( context_org_array, org_tree_root_id );
                        osrfLogDebug( OSRF_LOG_MARK, "top of the org tree is %s", org_tree_root_id );
@@ -1357,13 +1357,13 @@ static int verifyObjectPCRUD (  osrfMethodContext* ctx, const jsonObject* obj )
                jsonObject *param = NULL;
 
                if( obj->classname ) {
-                       pkey_value = oilsFMGetString( obj, pkey );
+                       pkey_value = oilsFMGetStringConst( obj, pkey );
                        if( !fetch )
                                param = jsonObjectClone( obj );
                        osrfLogDebug( OSRF_LOG_MARK, "Object supplied, using primary key value of %s",
                                        pkey_value );
                } else {
-                       pkey_value = jsonObjectToSimpleString( obj );
+                       pkey_value = jsonObjectGetString( obj );
                        fetch = 1;
                        osrfLogDebug( OSRF_LOG_MARK, "Object not supplied, using primary key value "
                                        "of %s and retrieving from the database", pkey_value );
@@ -1404,9 +1404,6 @@ static int verifyObjectPCRUD (  osrfMethodContext* ctx, const jsonObject* obj )
                        );
 
                        free( m );
-                       if( pkey_value )
-                               free( pkey_value );
-
                        return 0;
                }
 
@@ -1419,7 +1416,7 @@ static int verifyObjectPCRUD (  osrfMethodContext* ctx, const jsonObject* obj )
                        int i = 0;
                        const char* lcontext = NULL;
                        while ( (lcontext = osrfStringArrayGetString(local_context, i++)) ) {
-                               osrfStringArrayAdd( context_org_array, oilsFMGetString( param, lcontext ) );
+                               osrfStringArrayAdd( context_org_array, oilsFMGetStringConst( param, lcontext ));
                                osrfLogDebug(
                                        OSRF_LOG_MARK,
                                        "adding class-local field %s (value: %s) to the context org list",
@@ -1548,7 +1545,7 @@ static int verifyObjectPCRUD (  osrfMethodContext* ctx, const jsonObject* obj )
                                        osrfStringArray* ctx_array = osrfHashGet( fcontext, "context" );
                                        while ( (foreign_field = osrfStringArrayGetString( ctx_array, j++ )) ) {
                                                osrfStringArrayAdd( context_org_array,
-                                                               oilsFMGetString( _fparam, foreign_field ) );
+                                                       oilsFMGetStringConst( _fparam, foreign_field ));
                                                osrfLogDebug(
                                                        OSRF_LOG_MARK,
                                                        "adding foreign class %s field %s (value: %s) to the context org list",
@@ -1584,7 +1581,7 @@ static int verifyObjectPCRUD (  osrfMethodContext* ctx, const jsonObject* obj )
                while( (context_org = osrfStringArrayGetString( context_org_array, j++ )) ) {
                        dbi_result result;
 
-                       if(  pkey_value ) {
+                       if( pkey_value ) {
                                osrfLogDebug(
                                        OSRF_LOG_MARK,
                                        "Checking object permission [%s] for user %d "
@@ -1683,8 +1680,6 @@ static int verifyObjectPCRUD (  osrfMethodContext* ctx, const jsonObject* obj )
                        break;
        }
 
-       if( pkey_value )
-               free(pkey_value);
        osrfStringArrayFree( context_org_array );
 
        return OK;
@@ -1702,7 +1697,7 @@ static int verifyObjectPCRUD (  osrfMethodContext* ctx, const jsonObject* obj )
 
        The calling code is responsible for freeing the returned string.
 */
-static char* org_tree_root( osrfMethodContext* ctx ) {
+static const char* org_tree_root( osrfMethodContext* ctx ) {
 
        static char cached_root_id[ 32 ] = "";  // extravagantly large buffer
        static time_t last_lookup_time = 0;
@@ -1740,13 +1735,12 @@ static char* org_tree_root( osrfMethodContext* ctx ) {
                return NULL;
        }
 
-       char* root_org_unit_id = oilsFMGetString( tree_top, "id" );
+       const char* root_org_unit_id = oilsFMGetStringConst( tree_top, "id" );
        osrfLogDebug( OSRF_LOG_MARK, "Top of the org tree is %s", root_org_unit_id );
 
-       jsonObjectFree( result );
-
        strcpy( cached_root_id, root_org_unit_id );
-       return root_org_unit_id;
+       jsonObjectFree( result );
+       return cached_root_id;
 }
 
 /**
index d8b6929..3739413 100644 (file)
@@ -22,24 +22,67 @@ osrfHash* oilsInitIDL(const char* idl_filename) {
 
        if (!oilsIDLInit( filename )) {
                osrfLogError(OSRF_LOG_MARK, "Problem loading IDL file [%s]!", filename);
-               if(freeable_filename) free(freeable_filename);
+               if(freeable_filename)
+                       free(freeable_filename);
                return NULL;
        }
 
-       if(freeable_filename) free(freeable_filename);
+       if(freeable_filename)
+               free(freeable_filename);
        return oilsIDL();
 }
 
+/**
+       @brief Return a const string with the value of a specified column in a row object.
+       @param object Pointer to the row object.
+       @param field Name of the column.
+       @return Pointer to a const string representing the value of the specified column,
+               or NULL in case of error.
+
+       The row object must be a JSON_ARRAY with a classname.  The column value must be a
+       JSON_STRING or a JSON_NUMBER.  Any other object type results in a return of NULL.
+
+       The return value points into the internal contents of the row object, which
+       retains ownership.
+*/
+const char* oilsFMGetStringConst( const jsonObject* object, const char* field ) {
+       return jsonObjectGetString(oilsFMGetObject( object, field ));
+}
+
+/**
+       @brief Return a string with the value of a specified column in a row object.
+       @param object Pointer to the row object.
+       @param field Name of the column.
+       @return Pointer to a newly allocated string representing the value of the specified column,
+               or NULL in case of error.
+
+       The row object must be a JSON_ARRAY with a classname.  The column value must be a
+       JSON_STRING or a JSON_NUMBER.  Any other object type results in a return of NULL.
+
+       The calling code is responsible for freeing the returned string by calling free().
+ */
 char* oilsFMGetString( const jsonObject* object, const char* field ) {
        return jsonObjectToSimpleString(oilsFMGetObject( object, field ));
 }
 
+/**
+       @brief Return a pointer to the value of a specified column in a row object.
+       @param object Pointer to the row object.
+       @param field Name of the column.
+       @return Pointer to the jsonObject representing the value of the specified column, or NULL
+               in case of error.
 
+       The row object must be a JSON_ARRAY with a classname.
+
+       The return value may point to a JSON_NULL, JSON_STRING, JSON_NUMBER, or JSON_ARRAY.  It
+       points into the internal contents of the row object, which retains ownership.
+*/
 const jsonObject* oilsFMGetObject( const jsonObject* object, const char* field ) {
        if(!(object && field)) return NULL;
        if( object->type != JSON_ARRAY || !object->classname ) return NULL;
        int pos = fm_ntop(object->classname, field);
-       if( pos > -1 ) return jsonObjectGetIndex( object, pos );
+       if( pos > -1 )
+               return jsonObjectGetIndex( object, pos );
        return NULL;
 }
 
@@ -68,7 +111,10 @@ long oilsFMGetObjectId( const jsonObject* obj ) {
        long id = -1;
        if(!obj) return id;
        char* ids = oilsFMGetString( obj, "id" );
-       if(ids) { id = atol(ids); free(ids); }
+       if(ids) {
+               id = atol(ids);
+               free(ids);
+       }
        return id;
 }
 
@@ -78,6 +124,8 @@ oilsEvent* oilsUtilsCheckPerms( int userid, int orgid, char* permissions[], int
        int i;
        oilsEvent* evt = NULL;
 
+       // Find the root org unit, i.e. the one with no parent.
+       // Assumption: there is only one org unit with no parent.
        if (orgid == -1) {
                jsonObject* where_clause = jsonParse( "{\"parent_ou\":null}" );
                jsonObject* org = oilsUtilsQuickReq(
@@ -87,60 +135,116 @@ oilsEvent* oilsUtilsCheckPerms( int userid, int orgid, char* permissions[], int
                );
                jsonObjectFree( where_clause );
 
-        orgid = (int)jsonObjectGetNumber( oilsFMGetObject( org, "id" ) );
+               orgid = (int)jsonObjectGetNumber( oilsFMGetObject( org, "id" ) );
 
-        jsonObjectFree(org);
-    }
+               jsonObjectFree(org);
+       }
 
        for( i = 0; i < size && permissions[i]; i++ ) {
 
                char* perm = permissions[i];
                jsonObject* params = jsonParseFmt("[%d, \"%s\", %d]", userid, perm, orgid);
-               jsonObject* o = oilsUtilsQuickReq( "open-ils.storage", 
+               jsonObject* o = oilsUtilsQuickReq( "open-ils.storage",
                        "open-ils.storage.permission.user_has_perm", params );
 
                char* r = jsonObjectToSimpleString(o);
 
-               if(r && !strcmp(r, "0")) 
+               if(r && !strcmp(r, "0"))
                        evt = oilsNewEvent3( OSRF_LOG_MARK, OILS_EVENT_PERM_FAILURE, perm, orgid );
 
                jsonObjectFree(params);
                jsonObjectFree(o);
                free(r);
 
-               if(evt) break;
+               if(evt)
+                       break;
        }
 
        return evt;
 }
 
+/**
+       @brief Perform a remote procedure call.
+       @param service The name of the service to invoke.
+       @param method The name of the method to call.
+       @param params The parameters to be passed to the method, if any.
+       @return A copy of whatever the method returns as a result, or a JSON_NULL if the method
+       doesn't return anything.
+
+       If the @a params parameter points to a JSON_ARRAY, pass each element of the array
+       as a separate parameter.  If it points to any other kind of jsonObject, pass it as a
+       single parameter.  If it is NULL, pass no parameters.
+
+       The calling code is responsible for freeing the returned object by calling jsonObjectFree().
+*/
 jsonObject* oilsUtilsQuickReq( const char* service, const char* method,
                const jsonObject* params ) {
        if(!(service && method)) return NULL;
+
        osrfLogDebug(OSRF_LOG_MARK, "oilsUtilsQuickReq(): %s - %s", service, method );
-       osrfAppSession* session = osrfAppSessionClientInit( service ); 
+
+       // Open an application session with the service, and send the request
+       osrfAppSession* session = osrfAppSessionClientInit( service );
        int reqid = osrfAppSessionSendRequest( session, params, method, 1 );
-       osrfMessage* omsg = osrfAppSessionRequestRecv( session, reqid, 60 ); 
-       jsonObject* result = jsonObjectClone(osrfMessageGetResult(omsg));
+
+       // Get the response
+       osrfMessage* omsg = osrfAppSessionRequestRecv( session, reqid, 60 );
+       jsonObject* result = jsonObjectClone( osrfMessageGetResult(omsg) );
+
+       // Clean up
        osrfMessageFree(omsg);
        osrfAppSessionFree(session);
        return result;
 }
 
+/**
+       @brief Call a method of the open-ils.storage service.
+       @param method Name of the method.
+       @param params Parameters to be passed to the method, if any.
+       @return A copy of whatever the method returns as a result, or a JSON_NULL if the method
+       doesn't return anything.
+
+       If the @a params parameter points to a JSON_ARRAY, pass each element of the array
+       as a separate parameter.  If it points to any other kind of jsonObject, pass it as a
+       single parameter.  If it is NULL, pass no parameters.
+
+       The calling code is responsible for freeing the returned object by calling jsonObjectFree().
+*/
 jsonObject* oilsUtilsStorageReq( const char* method, const jsonObject* params ) {
        return oilsUtilsQuickReq( "open-ils.storage", method, params );
 }
 
+/**
+       @brief Call a method of the open-ils.cstore service.
+       @param method Name of the method.
+       @param params Parameters to be passed to the method, if any.
+       @return A copy of whatever the method returns as a result, or a JSON_NULL if the method
+       doesn't return anything.
+
+       If the @a params parameter points to a JSON_ARRAY, pass each element of the array
+       as a separate parameter.  If it points to any other kind of jsonObject, pass it as a
+       single parameter.  If it is NULL, pass no parameters.
+
+       The calling code is responsible for freeing the returned object by calling jsonObjectFree().
+*/
 jsonObject* oilsUtilsCStoreReq( const char* method, const jsonObject* params ) {
        return oilsUtilsQuickReq("open-ils.cstore", method, params);
 }
 
 
 
+/**
+       @brief Given a username, fetch the corresponding row from the actor.usr table, if any.
+       @param name The username for which to search.
+       @return A Fieldmapper object for the relevant row in the actor.usr table, if it exists;
+       or a JSON_NULL if it doesn't.
+
+       The calling code is responsible for freeing the returned object by calling jsonObjectFree().
+*/
 jsonObject* oilsUtilsFetchUserByUsername( const char* name ) {
        if(!name) return NULL;
        jsonObject* params = jsonParseFmt("{\"usrname\":\"%s\"}", name);
-       jsonObject* user = oilsUtilsQuickReq( 
+       jsonObject* user = oilsUtilsQuickReq(
                "open-ils.cstore", "open-ils.cstore.direct.actor.user.search", params );
 
        jsonObjectFree(params);
@@ -149,6 +253,17 @@ jsonObject* oilsUtilsFetchUserByUsername( const char* name ) {
        return user;
 }
 
+/**
+       @brief Given a barcode, fetch the corresponding row from the actor.usr table, if any.
+       @param name The barcode for which to search.
+       @return A Fieldmapper object for the relevant row in the actor.usr table, if it exists;
+       or a JSON_NULL if it doesn't.
+
+       Look up the barcode in actor.card.  Follow a foreign key from there to get a row in
+       actor.usr.
+
+       The calling code is responsible for freeing the returned object by calling jsonObjectFree().
+*/
 jsonObject* oilsUtilsFetchUserByBarcode(const char* barcode) {
        if(!barcode) return NULL;
 
@@ -159,14 +274,18 @@ jsonObject* oilsUtilsFetchUserByBarcode(const char* barcode) {
                "open-ils.cstore", "open-ils.cstore.direct.actor.card.search", params );
        jsonObjectFree(params);
 
-       if(!card) return NULL;
+       if(!card)
+               return NULL;   // No such card
 
+       // Get the user's id as a double
        char* usr = oilsFMGetString(card, "usr");
        jsonObjectFree(card);
-       if(!usr) return NULL;
+       if(!usr)
+               return NULL;   // No user id (shouldn't happen)
        double iusr = strtod(usr, NULL);
        free(usr);
 
+       // Look up the user in actor.usr
        params = jsonParseFmt("[%f]", iusr);
        jsonObject* user = oilsUtilsQuickReq(
                "open-ils.cstore", "open-ils.cstore.direct.actor.user.retrieve", params);
@@ -182,9 +301,9 @@ char* oilsUtilsFetchOrgSetting( int orgid, const char* setting ) {
 
        jsonObject* set = oilsUtilsQuickReq(
                "open-ils.actor",
-        "open-ils.actor.ou_setting.ancestor_default", params);
+               "open-ils.actor.ou_setting.ancestor_default", params);
 
-    char* value = jsonObjectToSimpleString(jsonObjectGetKey(set, "value"));
+       char* value = jsonObjectToSimpleString(jsonObjectGetKey(set, "value"));
        jsonObjectFree(params);
        jsonObjectFree(set);
        osrfLogDebug(OSRF_LOG_MARK, "Fetched org [%d] setting: %s => %s", orgid, setting, value);
@@ -201,7 +320,7 @@ char* oilsUtilsLogin( const char* uname, const char* passwd, const char* type, i
 
        jsonObject* params = jsonParseFmt("[\"%s\"]", uname);
 
-       jsonObject* o = oilsUtilsQuickReq( 
+       jsonObject* o = oilsUtilsQuickReq(
                "open-ils.auth", "open-ils.auth.authenticate.init", params );
 
        const char* seed = jsonObjectGetString(o);
@@ -221,7 +340,8 @@ char* oilsUtilsLogin( const char* uname, const char* passwd, const char* type, i
        if(o) {
                const char* tok = jsonObjectGetString(
                        jsonObjectGetKey(jsonObjectGetKey(o,"payload"), "authtoken"));
-               if(tok) token = strdup(tok);
+               if( tok )
+                       token = strdup( tok );
        }
 
        free(fullhash);
@@ -235,7 +355,7 @@ char* oilsUtilsLogin( const char* uname, const char* passwd, const char* type, i
 jsonObject* oilsUtilsFetchWorkstation( long id ) {
        jsonObject* p = jsonParseFmt("[%ld]", id);
        jsonObject* r = oilsUtilsQuickReq(
-               "open-ils.storage", 
+               "open-ils.storage",
                "open-ils.storage.direct.actor.workstation.retrieve", p );
        jsonObjectFree(p);
        return r;
@@ -243,11 +363,8 @@ jsonObject* oilsUtilsFetchWorkstation( long id ) {
 
 jsonObject* oilsUtilsFetchWorkstationByName( const char* name ) {
        jsonObject* p = jsonParseFmt("{\"name\":\"%s\"}", name);
-    jsonObject* r = oilsUtilsCStoreReq(
-        "open-ils.cstore.direct.actor.workstation.search", p);
+       jsonObject* r = oilsUtilsCStoreReq(
+               "open-ils.cstore.direct.actor.workstation.search", p);
        jsonObjectFree(p);
-    return r;
+       return r;
 }
-
-
-