On Saturday 12 April 2008 05:39:23 pm Brad Hards wrote:
> However the issue I need help with relates to the MAPI_RETVAL_IF() macro.
> We have a lot of API documentation that says something like:
> \return MAPI_E_SUCCESS on success, otherwise -1
>
> \note Developers should call GetLastError() to retrieve the last MAPI
> error code. Possible MAPI error codes are:
> - MAPI_E_NOT_INITIALIZED: MAPI subsystem has not been initialized
> - MAPI_E_SESSION_LIMIT: No session has been opened on the provider
>
> The function signature looks like:
> _PUBLIC_ enum MAPISTATUS function()
>
> The MAPI_RETVAL_IF macro returns -1 (or doesn't return)
>
> Some of those things are wrong.
>
> Do we really want users to check GetLastError()?
>
> Should MAPI_RETVAL_IF() return a MAPISTATUS?
OK, here is the proposed way forward. Suggestions appreciated.
1. We should never return -1. That is pointless. MAPI_RETVAL_IF should return
an enum MAPISTATUS.
2. We should only use MAPI_RETVAL_IF() in early sanity checks of arguments.
The problem with using it in the middle of code is that we don't clean up
everything. MAPI_RETVAL_IF can clean up the talloc context, but it doesn't
call mapi_object_release() for example.
3. Instead, we adopt a pattern that looks like:
enum MAPISTATUS foo()
{
enum MAPISTATUS retval = MAPI_E_SUCCESS;
....
mapi_object_init(&obj_store);
retval = OpenMsgStore(&obj_store);
if (retval != MAPI_E_SUCCESS) {
goto cleanup;
}
retval = GetDefaultFolder(&obj_store, &id_folder, olFolderOutbox);
if (retval != MAPI_E_SUCCESS) {
goto cleanup;
}
mapi_object_init(&obj_folder);
retval = OpenFolder(&obj_folder, id_folder, &obj_folder);
if (retval != MAPI_E_SUCCESS) {
goto cleanup;
}
/* more functions, same pattern */
cleanup:
mapi_object_release(&obj_folder);
mapi_object_release(&obj_store);
return retval;
}
That relies on it being safe to call mapi_object_release() on something that
has not been mapi_object_init()'d. If not, we need to release everything by
hand on failure.
4. Another option might be to require all mapi_object_t objects to be talloc'd
and setting a destructor that calls mapi_object_release(). I haven't tried
this yet though.
5. I'm steering away from GetLastError(), because it doesn't appear
thread-safe to me. Global errno will always be racy, and I'm not so fond of
it that I'd like to implement a locking mechanism. Instead, we keep each
retval and use that.
6. The code could look nicer with some macros:
#define MAPI_STATUS_IS_ERR_GOTO(x, LABEL) do { \
if (MAPI_STATUS_IS_ERR(x)) {\
goto LABEL;\
}\
} while (0)
Thoughts?
Brad
_______________________________________________
devel mailing list
[email protected]
http://mailman.openchange.org/listinfo/devel
