API's Null pointer check in void methods

[zanzaben]

Is your feature request related to a problem? Please describe.
Some pointers are used in methods that return void so the OS_CHECK_POINTER function doesn't work in those methods since it returns an error code. There needs to be a way to check that pointers are valid inside void methods.

Describe the solution you'd like
Need to discuss the best solution

Additional context
related to #742, That branch has this comment marking everywhere it comes up.
/* TODO: void pointer, #765 */

Requester Info
Alex Campbell GSFC

[jphickey]

You should be able to use BUGCHECK() macro directly but leave the second argument blank, i.e.:

BUGCHECK(ptrarg != NULL,)

Note the comma because its a two-arg macro, but you don't actually have to use the second arg, but it has to be there syntactically.

However in general void functions are less common - and it calls for the question:

1. should it be considered an error in the first place? (maybe it should just accept all inputs)
2. otherwise, then should it really be returning an int32 so it can communicate to the caller that the function didn't do its job?

By design if a function returns void it is basically saying there is no way this function can fail. It's good and simple and I like these type of functions - but if the implementation actually can fail but cannot communicate that failure - then that seems more like an API design flaw. A void function should always accept its inputs and do its performed task (item 1 above) no matter what. If it doesn't meet that model then it should probably be returning an error code (item 2 above).

[skliper]

Interesting topic... let's go with the BUGCHECK for now. We can trade changing function returns for APIs that can only fail when a bad pointer is passed in at a later date.

[jphickey]

FWIW I think most (if not all?) of the functions that fall into this category are internal/helper functions and therefore should just do what they are told (case 1). If it were a public API then it should probably return something (case 2).

At this point its not clear to me if there are any public APIs that are affected - so there may not be any real issue here...

[zanzaben]

After going through and removing the checks from internal methods there are still 2 cases.

osal/src/os/shared/src/osapi-idmap.c

Lines 1428 to 1454 in bfca5b2

	/*-----------------------------------------------------------------
	*
	* Function: OS_ForEachObjectOfType
	*
	* Purpose: Implemented per public OSAL API
	* See description in API and header file for detail
	*
	*-----------------------------------------------------------------*/
	void OS_ForEachObjectOfType(osal_objtype_t idtype, osal_id_t creator_id, OS_ArgCallback_t callback_ptr, void *callback_arg)
	{
	OS_object_iter_t iter;
	OS_creator_filter_t filter;
	filter.creator_id = creator_id;
	filter.user_callback = callback_ptr;
	filter.user_arg = callback_arg;
	if (OS_ObjectIdIteratorInit(OS_ForEachFilterCreator, &filter, idtype, &iter) == OS_SUCCESS)
	{
	while (OS_ObjectIdIteratorGetNext(&iter))
	{
	OS_ObjectIdIteratorProcessEntry(&iter, OS_ForEachDoCallback);
	}
	OS_ObjectIdIteratorDestroy(&iter);
	}
	} /* end OS_ForEachObjectOfType */

osal/src/os/shared/src/osapi-printf.c

Lines 245 to 306 in bfca5b2

	/*----------------------------------------------------------------
	*
	* Function: OS_printf
	*
	* Purpose: Implemented per public OSAL API
	* See description in API and header file for detail
	*
	*-----------------------------------------------------------------*/
	void OS_printf(const char *String, ...)
	{
	va_list va;
	char msg_buffer[OS_BUFFER_SIZE];
	int actualsz;
	if (!OS_SharedGlobalVars.Initialized)
	{
	/*
	* Catch some historical mis-use of the OS_printf() call.
	*
	* Typically OS_printf() should NOT be called before OS_API_Init().
	*
	* This was never guaranteed to work, particularly on a VxWorks
	* deployment where the utility task was enabled.
	*
	* However, some PSPs do this, particularly those that used POSIX
	* where it happened to work (because OS_printf just called printf).
	*
	* As a workaround, use the OS_DEBUG facility to dump the message,
	* along with a clue that this API is being used inappropriately.
	*
	* If debugging is not enabled, then this message will be silently
	* discarded.
	*/
	OS_DEBUG("BUG: OS_printf() called before init: %s", String);
	}
	else if (OS_SharedGlobalVars.PrintfEnabled)
	{
	/*
	* Call vsnprintf() to determine the actual size of the
	* string we are going to write to the buffer after formatting.
	*/
	va_start(va, String);
	actualsz = vsnprintf(msg_buffer, sizeof(msg_buffer), String, va);
	va_end(va);
	if (actualsz < 0)
	{
	/* unlikely: vsnprintf failed */
	actualsz = 0;
	}
	else if (actualsz >= OS_BUFFER_SIZE)
	{
	/* truncate */
	actualsz = OS_BUFFER_SIZE - 1;
	}
	msg_buffer[actualsz] = 0;
	OS_ConsoleWrite(OS_SharedGlobalVars.PrintfConsoleId, msg_buffer);
	}
	} /* end OS_printf */

[jphickey]

Yeah for those two it should be possible to use the BUGCHECK macro but leave the second argument blank. That way it basically turns into a simple assert (depending on whether strict checking is enabled in the config).