On Sun, 2008-07-13 at 12:06 +1000, Brad Hards wrote: > 1. We should never return -1. That is pointless. MAPI_RETVAL_IF should return > an enum MAPISTATUS.
I completely agree with you. This should only be a minor change given
the current implementation.
> 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.
Correct. In the meantime, MAPI_RETVAL_IF was initially only intended to
be used in libmapi code (sanity checks). Laziness leaded to use it
outside the original context.
> 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.
Currently, there's only a very few call using the private data pointer.
Moreover this private data is generally allocated using the session
context. So when MAPIUninitialize is called, this data should be free'd
whatever happen.
The only - but still relevant - problem is that the object is not
released on the Exchange server. However I don't think this would
justify the mapi_object talloc.
> 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.
libmapi has never been designed to be multi-threaded. Depending on how
multisession is implemented (have some models in mind I need to test),
we may find a nice workaround.
> 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)
I'm not a big "goto" fan. I'd rather use some kind of convenient API
with a queue mechanism. This need to be discussed, so it makes code
saner without adding a layer of complexity, but I'm confident this is
highly feasible. I just try to keep in mind this model needs to fit with
the multisession mode and does not require to rewrite/rethink it from
scratch.
--
Julien Kerihuel
[EMAIL PROTECTED]
OpenChange Project Manager
GPG Fingerprint: 0B55 783D A781 6329 108A B609 7EF6 FE11 A35F 1F79
signature.asc
Description: This is a digitally signed message part
_______________________________________________ devel mailing list [email protected] http://mailman.openchange.org/listinfo/devel
