Inconsistencies/bugs with OpenMama


Sridharan, Venkatesan <Venkatesan.Sridharan@...>
 

Hi Folks,

This is my first post here. First off, thanks for your efforts on OpenMama, the code is clean, well commented and the documentation is great too!

We've been using OpenMama for a few months now and have written bridges for TibRv and Qpid (using qpid c++ API). I am talking to my company about opensourcing them. I came across a number of seeming errors/inconsistencies and wanted to get the community's take. I can provide a patch to fix the issues if we agree that they indeed are bugs and that they do need to be fixed.

* xxxxBridge_getDefaultPayloadId (char ***name, char **id)
The function retrieves the payload id which are const and hence params must be const qualified -> (const char ***name, const char **id).

* xxxPayload_getMsg(const msgPayload msg, const char* name, mama_fid_t fid, msgPayload* result)
We are passing in a const msg and extracting its field. so, last arg should be const msgPayload*

* loadbridge uses property file before open()
During creation of the default queue, properties subsystem is accessed and this always prints an error/warning if we use a custom properties file.

* bridgeMamaPublisherSendReplyToInbox => request is void* instead of mamaMsg
typedef mama_status
(*bridgeMamaPublisher_sendReplyToInbox)(publisherBridge publisher,
=>void * request, <=
mamaMsg reply);
This is needless as the request is a mamaMsg


* throw in dtor!
MamaTransport::~MamaTransport (void)
{
if (mPimpl)
{
if ( mamaInternal_getCatchCallbackExceptions() && mPimpl->mCallback)
{
delete mPimpl->mCallback;
}
if (mPimpl->mDeleteCTransport)
{
=>mamaTry (mamaTransport_destroy (mTransport)); <=
}
delete mPimpl;
mPimpl = NULL;
}
}

* Bridges are expected to allocate memory for some functions (toString, getReplyHandle etc) but there is no hook to free the memory. Since each bridge is allowed to use custom memory allocation, there should be a hook from mamaMsg_freeString back to the bridge. Also, currently mamaMsg_freeString is a no-op

* mamaMsg_copy does not check for self copy

* Some fields are not initialized in INITIALIZE_BRIDGE () macro. Eg, mamaBridgeImpl::mInternalDispatcher is not set to NULL on bridge creation. Got a crash on this one.

* As per comments and documentation, mamaQueue_timedDispatch is supposed to keep dispatching until the given timeout has elapsed. But, the current bridge implementations use
wombatQueue_timedDispatch which dispatches only one message. So, EITHER wombatQueue_timedDispatch should be changed to keep dispatching till timeout OR another helper function
should be provided to do this (as this functionality is used across bridges)

* msgPayload_unSerialize prototype incorrect (takes in void** instead of void*)
typedef mama_status
(*msgPayload_unSerialize) (const msgPayload msg,
=>const void** buffer, <=
mama_size_t bufferLength);
This is an input buffer and the only function using this is mamaMsg_setNewBuffer which calls the above function and passes a void* buffer as a void**

* Incorrect test in MamaMsg::setNewBuffer ():
bool MamaMsg::setNewBuffer (
void* buffer,
mama_size_t size)
{
int result = 0;
// should be MAMA_STATUS_OK == ...
=> if (MAMA_STATUS_OK != (mamaTry (mamaMsg_setNewBuffer ( <=
mMsg, buffer, size))))
{
return result;
}
return result != 0;
}

* These functions are not defined, I encountered this when trying to build python, perl interfaces with swig. A default implementation returning MAMA_STATUS_NOT_IMPLEMENTED or nullptr should be added
mamaMsg_updateVectorTime, mamaMsg_updateVectorPrice, mamaDispatcher_getQueue, mama_forceLogVaWithPrefix, mamaMsgField_getVectorBool, mama_logPolicyToString, mama_tryStringToLogPolicy, mamaMsgIterator_end

* Inconsistent prototypes :
mama_status
mamaMsg_updateVectorPrice (
mamaMsg msg,
const char* fname,
mama_fid_t fid,
//should be const mamaPrice priceList[]
=> const mamaPrice* priceList[], =<
mama_size_t numElements);

msgPayload_getPrice, msgPayload_getDateTime should take pointer to result as last parameter
Eg:
typedef mama_status
(*msgPayload_getDateTime) (const msgPayload msg,
const char* name,
mama_fid_t fid,
//should be mamaDateTime * result
=> mamaDateTime result); <=

* There are some pesky macros defined in port.h (close, sleep etc) which causes problems when using other libraries (eg boost). We should be defining them as inline functions instead of using macros


Cheers,
Venkatesan




Confidentiality Note: This e-mail and any attachments are confidential and may
be protected by legal privilege. If you are not the intended recipient, be aware
that any disclosure, copying, distribution or use of this e-mail or any
attachment is prohibited. If you have received this e-mail in error, please
notify us immediately by returning it to the sender and delete this copy from
your system. Thank you for your cooperation.

Join Openmama-dev@lists.openmama.org to automatically receive all group messages.