Expand resource object query options

[dmccomas]

While integrating File Manager app 2.5.2 with cFE 6.7.1 (OSAL 5.0.1) I ran into an issue. FM has a
command that allows users to receive a telemetry packt listing all of the open files. In order to do this FM needs to be able to query OSAL's file stream resource objects. The current implementation only allows a creator to query all of the resources objects by using OS_ForEachObject(). I think having a more general query feature would be helpful. I added a new function OS_QueryObjectType() that allows anyone (not restricted to the creator) to query a resource type. The specific OSAL changes are below followed by the FM code that uses the function. These changes were made for OpenStaKit 2.1 that can be found at https://github.com/OpenSatKit/OpenSatKit.

osapi-os-core.h:

/*
** Typedef for object query OSAL callback functions. A query does not
** have to be performed by the object creator. All fields of the
** query_record are completed.
**
** This may be used by multiple APIs
*/

typedef struct
{
const char *name_entry;
uint32 creator;
uint16 refcount;
} OS_query_record_t;

typedef void (*OS_ObjQueryCallback_t)(OS_query_record_t *query_rec, void *callback_arg); //dcm - Added for OSK

/-------------------------------------------------------------------------------------/
/**

• @brief Query an object resource type maintained by the OSAL
• User supplied callback is called for all active resources of a particular type
• regardless of whether the caller created the object.

*/
uint32 OS_QueryObjectType (uint32 obj_type, OS_ObjQueryCallback_t callback_ptr, OS_query_record_t *query_rec, void *callback_arg); // dcm - Added for OSK

osapi-idmap.c:

/*----------------------------------------------------------------
*

• Function: OS_QueryObjectType
• Purpose: Implemented per public OSAL API

•      See description in API and header file for detail
  

-----------------------------------------------------------------/
uint32 OS_QueryObjectType (uint32 obj_type, OS_ObjQueryCallback_t callback_ptr, OS_query_record_t *query_rec, void *callback_arg)
{

uint32 obj_index;
uint32 obj_max;
uint32 obj_id;
uint32 active_obj_cnt = 0;
OS_common_record_t  *obj_rec;

obj_max = OS_GetMaxForObjectType(obj_type);
if (obj_max > 0)
{
    OS_Lock_Global_Impl(obj_type);
    obj_index = OS_GetBaseForObjectType(obj_type);
    while (obj_max > 0)
    {
        obj_rec = &OS_common_table[obj_index];
        obj_id = obj_rec->active_id;
        if (obj_id != 0) 
        {

            query_rec->name_entry = obj_rec->name_entry;
            query_rec->creator    = obj_rec->creator;
            query_rec->refcount   = obj_rec->refcount;
            
            /*
             * Handle the object - Note that we must UN-lock before callback.
             * The callback function might lock again in a different manner.
             */
             OS_Unlock_Global_Impl(obj_type);
             (*callback_ptr)(query_rec, callback_arg);
             OS_Lock_Global_Impl(obj_type);
             
             ++active_obj_cnt;

        }
        ++obj_index;
        --obj_max;
    }
    OS_Unlock_Global_Impl(obj_type);
}

return active_obj_cnt;

} /* End OS_QueryObjectType() */

fm_cmd_utils.c:

static uint32 open_file_cnt = 0;
static void LoadOpenFileData(OS_query_record_t *query_rec, void *callback_arg)
{

FM_OpenFilesEntry_t *OpenFilesData = (FM_OpenFilesEntry_t *)callback_arg;
CFE_ES_TaskInfo_t   TaskInfo;

if (OpenFilesData != (FM_OpenFilesEntry_t *) NULL)
{
    /* FDTableEntry.Path has logical filename saved when file was opened */
    strcpy(OpenFilesData[open_file_cnt].LogicalName, query_rec->name_entry);

    /* Get the name of the application that opened the file */
    CFE_PSP_MemSet(&TaskInfo, 0, sizeof(CFE_ES_TaskInfo_t));
    if (CFE_ES_GetTaskInfo(&TaskInfo, query_rec->creator) == CFE_SUCCESS)
    {
        strcpy(OpenFilesData[open_file_cnt].AppName, (char *) TaskInfo.AppName);
    } 
}
++open_file_cnt;

} /* End LoadOpenFileData() */

/* * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * /
/ /
/ FM utility function -- get open files data /
/ /
/ * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * */

uint32 FM_GetOpenFilesData(FM_OpenFilesEntry_t *OpenFilesData)
{

OS_query_record_t query_rec;

open_file_cnt = 0;
OS_QueryObjectType (OS_OBJECT_TYPE_OS_STREAM, LoadOpenFileData, &query_rec, (void *)OpenFilesData);
   
return open_file_cnt;

} /* End FM_GetOpenFilesData */

[skliper]

@jphickey Could you review/comment?

[jphickey]

There are a few issues with the specifics of this proposal.

FM should first use OS_ForEachObject() (or a similar routine) to enumerate the active object IDs of the open files, then call OS_FDGetInfo() on each object ID which in turn will get the name associated with the file, along with other info about it.

I'm not convinced that OSAL needs a different callback signature that directly includes the name of the object since this info can be easily obtained after the fact with a follow-on query. Furthermore, including this introduces a race condition, as a direct string pointer to the internal object should never be accessed while "unlocked".

The main thing that is missing here is just that OS_ForEachObject() iterates all objects of all types that exist in OSAL, so its rather inefficient if you want only one specific object type, such as open files. I would support adding a version of OS_ForEachObject that only iterates one specific object type (i.e. OS_ForEachObjectOfType() maybe?) but otherwise shares the same callback signature as OS_ForEachObject() uses.

Another worthwhile addition for OSAL might be a generic API (i.e. not type-specific) to obtain the name of any resource/object ID. As every OSAL resource has a name associated with it, stored in the "common" table, this is possible and simple to do.

[dmccomas]

My motivation for recommending a new function is that OS_ForEachObject() requires the caller to pass in a creator_id. FM is reporting on all of the open files regardless of who created/opened the file. Once I thought a new function would help descoping the function seemed prudent.

I agree with the use of OS_FDGetInfo(). I overlooked this function.

[jphickey]

Note that the creator ID can be passed as 0, which disables filtering by creator, allowing all items to be returned.

[dmccomas]

I've pasted a File Manager solution below that uses the existing OSAL API. The only remaining question is whether looping over all object types is acceptable. Additional documentation would also be helpful but 6.7.1 is a pre-release and I rushed over some details when I first looked into updating FM.

static uint32 open_file_cnt = 0;
static void LoadOpenFileData(uint32 ObjId, void* CallbackArg)
{

   FM_OpenFilesEntry_t *OpenFilesData = (FM_OpenFilesEntry_t *)CallbackArg;
   CFE_ES_TaskInfo_t   TaskInfo;
   OS_file_prop_t      FdProp;
   
   if (OS_IdentifyObject(ObjId) == OS_OBJECT_TYPE_OS_STREAM) {
      
      if (OpenFilesData != (FM_OpenFilesEntry_t *) NULL) {
     
         if (OS_FDGetInfo (ObjId, &FdProp) == OS_SUCCESS) {
           
            strcpy(OpenFilesData[open_file_cnt].LogicalName, FdProp.Path);

            /* Get the name of the application that opened the file */
            CFE_PSP_MemSet(&TaskInfo, 0, sizeof(CFE_ES_TaskInfo_t));
            if (CFE_ES_GetTaskInfo(&TaskInfo, FdProp.User) == CFE_SUCCESS) {
               strcpy(OpenFilesData[open_file_cnt].AppName, (char *)TaskInfo.AppName);
            } 
         }
      } /* End if load FM's data */
      
      ++open_file_cnt;
      
   } /* End if OS_OBJECT_TYPE_OS_STREAM */
   
} /* End LoadOpenFileData() */

/* FM utility function -- get open files data  */

uint32 FM_GetOpenFilesData(FM_OpenFilesEntry_t *OpenFilesData)
{

    open_file_cnt = 0;
    OS_ForEachObject (0, LoadOpenFileData, (void *)OpenFilesData);
       
    return open_file_cnt;
    
} /* End FM_GetOpenFilesData */

[jphickey]

I don't think we should close this - I do think it is worthwhile to add a new "foreach" style API that only does a single resource type, because although the proposed change to FM does work and should be ok "for now" - it is not particularly efficient. A fairly easy API addition can make it more correct.

[jphickey]

This issue (and the related FM issue) came up again in testing, so its probably worthwhile to address it now.

[dmccomas]

I agree it shouldn't be closed. don't recall intentionally closing it, but it might have been a late night.