Hey Victor,

In Github projects, forking is generally the best way to work with the project, and is the recommended approach for raising patches for OpenMAMA - you can actually see the forks used by the likes of myself, Frank and other contributors if you go looking.

Regarding some documentation, you can find a bunch in the 'Contributor Guides' section of the OpenMAMA GitHub page, with the 'Submission Process' page probably the most interesting for you: https://openmama.github.io/openmama_submission_process.html

If you still aren't comfortable with the approach once you've given it a try, or if you hit some difficulties, we can apply the patch for you, but I'd recommend you give it a go - it's a much nicer way of working with the project than sending patches around, and should let you use a bunch of GitHub's other functionality as well.

Any questions feel free to reach out to me.

Thanks,

Damian

DAMIAN MAGUIRE 

Senior Sales Engineer 

 

O. +44 289 568 0298  

M. +44 783 584 4770 

dmaguire@... 

 

Adelaide Exchange Building, 2nd Floor, 24-26 Adelaide Street, Belfast, BT2 8GD  

velatradingtech.com | @vela_tt

---

From: Victor Maleyev <imnotmindlin@...>
Sent: 08 November 2017 11:01
To: Frank Quinn
Cc: openmama-dev; Damian Maguire; frank@...
Subject: Re: [Openmama-dev] [PATCH 6.2.1] MAMA C++: Fixed memory leak in MamaMsgField

 

Hi Frank,

I don't know how to make pull requests without forking the project, that's why I sent the patch. Is it possible for you to apply it? Or is it any instruction on how to do pull requests?

08.11.2017, 02:41, "Frank Quinn" <fquinn.ni@...>:

Thanks Victor,

Yes I can see how that wouldn't have behaved as expected and fix looks sound. Are you going to raise a pull request against the "next" branch for this or do you want me to take it from here?

Cheers,
Frank

On Mon, 6 Nov 2017, 09:55 Victor Maleyev, <imnotmindlin@...> wrote:

Not sure if my previous mail reached the maillist. Adding list admins, sorry if this is inapropriate.
---
It is possible to reproduce memory leak using the following:

MamaMsgIterator i;
for(MamaMsgField field = msg.begin(i); ...; field =  *++i) {
    field.getVectorMsg(...);
}

Note that field is not reference but object.

When calling getVectorMsg() the array of pointers (mLastVectorMsg) is allocated by malloc but when we assign a new value to the field (field = *++i) the allocated array becomes unreachable and can't be deleted. Overloaded assignment operator calling clear before assignment can solve this.
Signed-off-by: Victor Maleyev <imnotmindlin@...>

From c036b0726464916b2e49d6b1297a2a82404c0359 Mon Sep 17 00:00:00 2001
From: Victor Maleyev <imnotmindlin@...>
Date: Fri, 3 Nov 2017 00:14:05 +0300
Subject: [PATCH] Fixed memory leak in MamaMsgField

Signed-off-by: Victor Maleyev <imnotmindlin@...>
---
 mama/c_cpp/src/cpp/MamaMsgField.cpp | 15 +++++++++++++++
 mama/c_cpp/src/cpp/mama/MamaMsgField.h | 6 ++++++
 2 files changed, 21 insertions(+)

diff --git a/mama/c_cpp/src/cpp/MamaMsgField.cpp b/mama/c_cpp/src/cpp/MamaMsgField.cpp
index f2f287c..2cf1270 100644
--- a/mama/c_cpp/src/cpp/MamaMsgField.cpp
+++ b/mama/c_cpp/src/cpp/MamaMsgField.cpp
@@ -47,6 +47,21 @@ namespace Wombat
     {
     }

+ MamaMsgField::MamaMsgField (const MamaMsgField& field)
+ : mField (field.mField)
+ , mFieldDesc (NULL)
+ , mLastVectorMsg (NULL)
+ , mLastVectorMsgLen (0)
+ {
+ }
+
+ MamaMsgField& MamaMsgField::operator= (const MamaMsgField& field)
+ {
+ clear();
+ mField = field.mField;
+ return *this;
+ }
+
     void MamaMsgField::clear ()
     {
         if (mFieldDesc)
diff --git a/mama/c_cpp/src/cpp/mama/MamaMsgField.h b/mama/c_cpp/src/cpp/mama/MamaMsgField.h
index b9b9e73..d8f45b1 100644
--- a/mama/c_cpp/src/cpp/mama/MamaMsgField.h
+++ b/mama/c_cpp/src/cpp/mama/MamaMsgField.h
@@ -50,6 +50,12 @@ namespace Wombat
         MamaMsgField (
             mamaMsgField field);

+ MamaMsgField (
+ const MamaMsgField& field);
+
+ MamaMsgField& operator= (
+ const MamaMsgField& field);
+
         /**
          * Clear the field.
          */
--
2.13.3
-------- Конец пересылаемого сообщения --------_______________________________________________
Openmama-dev mailing list
Openmama-dev@...
https://lists.openmama.org/mailman/listinfo/openmama-dev

The information contained in this message may be privileged and confidential and protected from disclosure. If the reader of this message is not the intended recipient, you are hereby notified that any dissemination, distribution or copying of this communication is strictly prohibited. If you have received this communication in error, please notify us immediately by replying to this message and deleting it from your computer. Thank you. Vela Trading Technologies LLC

Thanks Victor,

Yes I can see how that wouldn't have behaved as expected and fix looks sound. Are you going to raise a pull request against the "next" branch for this or do you want me to take it from here?

Cheers,
Frank

On Mon, 6 Nov 2017, 09:55 Victor Maleyev, <imnotmindlin@...> wrote:

Not sure if my previous mail reached the maillist. Adding list admins, sorry if this is inapropriate.
---
It is possible to reproduce memory leak using the following:

MamaMsgIterator i;
for(MamaMsgField field = msg.begin(i); ...; field =  *++i) {
    field.getVectorMsg(...);
}

Note that field is not reference but object.

When calling getVectorMsg() the array of pointers (mLastVectorMsg) is allocated by malloc but when we assign a new value to the field (field = *++i) the allocated array becomes unreachable and can't be deleted. Overloaded assignment operator calling clear before assignment can solve this.
Signed-off-by: Victor Maleyev <imnotmindlin@...>

From c036b0726464916b2e49d6b1297a2a82404c0359 Mon Sep 17 00:00:00 2001
From: Victor Maleyev <imnotmindlin@...>
Date: Fri, 3 Nov 2017 00:14:05 +0300
Subject: [PATCH] Fixed memory leak in MamaMsgField

Signed-off-by: Victor Maleyev <imnotmindlin@...>
---
 mama/c_cpp/src/cpp/MamaMsgField.cpp | 15 +++++++++++++++
 mama/c_cpp/src/cpp/mama/MamaMsgField.h | 6 ++++++
 2 files changed, 21 insertions(+)

diff --git a/mama/c_cpp/src/cpp/MamaMsgField.cpp b/mama/c_cpp/src/cpp/MamaMsgField.cpp
index f2f287c..2cf1270 100644
--- a/mama/c_cpp/src/cpp/MamaMsgField.cpp
+++ b/mama/c_cpp/src/cpp/MamaMsgField.cpp
@@ -47,6 +47,21 @@ namespace Wombat
     {
     }

+ MamaMsgField::MamaMsgField (const MamaMsgField& field)
+ : mField (field.mField)
+ , mFieldDesc (NULL)
+ , mLastVectorMsg (NULL)
+ , mLastVectorMsgLen (0)
+ {
+ }
+
+ MamaMsgField& MamaMsgField::operator= (const MamaMsgField& field)
+ {
+ clear();
+ mField = field.mField;
+ return *this;
+ }
+
     void MamaMsgField::clear ()
     {
         if (mFieldDesc)
diff --git a/mama/c_cpp/src/cpp/mama/MamaMsgField.h b/mama/c_cpp/src/cpp/mama/MamaMsgField.h
index b9b9e73..d8f45b1 100644
--- a/mama/c_cpp/src/cpp/mama/MamaMsgField.h
+++ b/mama/c_cpp/src/cpp/mama/MamaMsgField.h
@@ -50,6 +50,12 @@ namespace Wombat
         MamaMsgField (
             mamaMsgField field);

+ MamaMsgField (
+ const MamaMsgField& field);
+
+ MamaMsgField& operator= (
+ const MamaMsgField& field);
+
         /**
          * Clear the field.
          */
--
2.13.3
-------- Конец пересылаемого сообщения --------_______________________________________________
Openmama-dev mailing list
Openmama-dev@...
https://lists.openmama.org/mailman/listinfo/openmama-dev

The information contained in this message may be privileged and confidential and protected from disclosure. If the reader of this message is not the intended recipient, you are hereby notified that any dissemination, distribution or copying of this communication is strictly prohibited. If you have received this communication in error, please notify us immediately by replying to this message and deleting it from your computer. Thank you. Vela Trading Technologies LLC

On Sep 19, 2017, at 1:10 PM, Duane Pauls <Duane.Pauls@...> wrote:

For Visual Studio, it appears as though the support lifecycle model changed after VS 2010.  While VS2012 and VS2013 are already EOL, VS2008 and VS2010 are still on extended support.  If the intent is to be cautious, it probably makes sense to wait until MS ends extended support before dropping support from OpenMAMA.

 

The end of extended support dates are:

VS2008: 2018-04-10

VS2010: 2020-07-14

 

Cheers,

Duane

 

From: openmama-dev-bounces@... [mailto:openmama-dev-bounces@...] On Behalf Of Frank Quinn
Sent: Tuesday, September 19, 2017 11:17 AM
To: Bill Torpey
Cc: openmama-dev
Subject: Re: [Openmama-dev] OpenMAMA static analysis

 

The question of support for visual studio is one for the community which I'll actually pose on a separate thread with its own subject so that people might read it, but since VS2013 only went EOL from microsoft a year ago, I expect there are a few out there still using it. 

 

Last time we asked, there were still some 2008 users out there too. As you can see, we actually only got rid of 2005 support as recently as 2 years ago (I was keen to do it as 2008 introduced proper msbuild support so we could finally upgrade to newer visual studio project formats). I'm keen to move forward too, but I don't want to alienate our client base either, so any movement forward would need to have 100% community support.

 

Generally for static code analysis, I'm all for it - it tends to prevent issues that weren't even considered. However it would be a lower priority for me behind a few other outstanding CI tasks related to making us more comfortable in merging changes, namely:

• CI and removal of warnings for Java builds
• Unit tests in CI environment for C#
• Windows CI for pull requests (possibly appveyor)
• Set up of jenkins multi platform builds for windows and linux (which is now possible since they both use the same CI script)

It does continue the gradual increase in sophistication for CI we have had in the last couple of years though... first of all we got unit tests working, then we got warnings removed on linux, now they're gone on windows, but yes I definitely want to always be moving towards a better CI environment which includes static analysis... but they tend to be slow burning tasks so it might take a while to be part of our day-to-day maintenance, but I'd happily accept any static code analysis related changes in the interim.

 

Cheers,

Frank

 

On Tue, Sep 19, 2017 at 2:08 PM, Bill Torpey <wallstprog@...> wrote:

Hi Damian:  

 

Thanks for the feedback.  I’ve made some comments below.

 

Regards,

 

Bill

 

On Sep 19, 2017, at 3:17 AM, Damian Maguire <dmagdrums@...> wrote:

 

Hey Bill, 

 

Great to get your feedback here, and welcome to the dev list. Absolutely the right place for this sort of discussion - GitHub is I think are more geared towards individual issues rather than larger project discussions such as this. 

 

Myself and Frank have spent a good bit of time discussing the subject of static analysis in the OpenMAMA codebase actually (some of the other contributors I think are aware of my interest in that area), and I think the community as a whole would be more than happy to see a stronger focus on it going forward. When support was added for building with Clang, we actually added native support for the Clang static analysis tools via scan-build - https://github.com/OpenMAMA/OpenMAMA/blob/master/site_scons/site_tools/scan-build.py - which means you can actually build and analyse within a single step:

 

    scan-build --use-c=/path/to/clang --use-cxx=/path/to/clang++ --use-analyzer=/path/to/clang scons ...

 

I'd love to see similar integration with other tools going forward (including CPPCheck, and the Google Santizer Suite), so if you have experience in that area I'd be happy to review any contributions. 

 

Right now, getting static analysis working with other tools is somewhat dependent on the build tooling.  I use a custom build script that runs scons under bear to create the compilation database required by my scripts.  The build script is somewhat specific to my environment, so I don’t know how useful it would be to others. but I’ve attached it just in case.

 

 

 

However, once you’ve got a compilation database built (and cppcheck installed), you can use the scripts at https://github.com/btorpey/static to run analysis using both clang and cppcheck, e.g.:

 

cc_cppcheck.sh -i common/c_cpp/src/c/ -i mama/c_cpp/src/c/ -c 2>&1 | tee cppcheck.csv

 

Similar scripts exist for clang-check and clang-tidy (although afaict clang-tidy does everything that clang-check does and more).

 

 

Regarding any fixes you want to submit, I say get them raised and get them in - given Frank's Windows warning hunt is coming to an end (for now) tackling some of these feels like a natural next step. 

 

I guess I’m curious about what others think about relative priorities, and/or whether there is are natural “owners” for different parts of the code-base that might want to have input.

 

The only thing I would add, and related to your comment about the scope problems, is any changes will need to take into account the breadth of compilers OpenMAMA has to support and that unfortunately means continuing with the C89/C99 cross breed that older versions of Visual Studio rely on. This isn't so much an OpenMAMA stylistic choice as one that's been forced on us by compiler support, but the wider user community relies on us continuing to adhere to this as a standard (for now ;-)).  

 

Curious if there’s a timetable for requiring VS2013 (the first MS compiler that doesn’t have this problem).  As you’re probably aware, the K&R style is “officially” frowned on, at least in the C++ Core Guidelines (https://github.com/isocpp/CppCoreGuidelines/blob/master/CppCoreGuidelines.md#es21-dont-introduce-a-variable-or-constant-before-you-need-to-use-it).  I notice that even some of Frank’s side projects (zmq, omnm) deviate from K&R style, which is understandable given how archaic it is.

 

In the meantime, it’s easy enough to filter out those warnings.

 

 

Great to hear you're getting to use OpenMAMA as part of your day job as well. Any feedback you have about your experience the community will be delighted to hear, including in the form of blog posts, so work away. If you want someone to review, or have any questions, I'm happy to help as well. 

 

Thanks.  I’ll send out a link when the article is complete — certainly interested in any feedback.  In the meantime, you may want to check out the other articles in the series: http://btorpey.github.io/blog/categories/static-analysis/

 

 

Looking forward to seeing more of your work. 

 

You’ve already seen some, just under another moniker: https://github.com/pulls?utf8=%E2%9C%93&q=author%3Abill-torpey-ullink+

 

 

 

 

 

 

 

 

Thanks, 

 

Damian 

 

On Sat, Sep 16, 2017 at 7:27 PM Bill Torpey <wallstprog@...> wrote:

Hi all:

I've been working with OpenMAMA for a little while now as part of my "day job", and I've also been working with static analysis tools for some time, so I thought it would be fun to put the two together.

As a first step, I ran the OpenMAMA code through cppcheck, which has become my go-to tool for static analysis, and produced the attached reports.  (FWIW, clang is good too, but I find that cppcheck flags more suspect code, and for me more is usually better).

I'm not sure what the rest of the community thinks about the importance of static analysis -- personally I'm a big believer, and I'm happy to get on board with helping to resolve some of these if others agree that would be a Good Thing.

For the record, the analysis was done on the 6.2.1 release, using cppcheck version 1.80, and using the scripts I've previously published as part of an ongoing series of articles about static analysis on [my blog](http://btorpey.github.io/).  The specific command used was:

   cc_cppcheck.sh 2>&1 | grep -v '/cpp' | grep -v test | grep -v examples | grep -v stdout | tee cppcheck-180.out

The idea was to concentrate on the core OpenMAMA C code, so the command explicitly ignores C++ code, test and examples.  (As well as some mysterious occurences of `stdout` which I haven't had time to track down yet).

A few observations:

- At least one of these issues has been [fixed subsequent to the release](https://github.com/OpenMAMA/OpenMAMA/pull/310).
- There are a boat-load of "scope can be reduced" warnings -- in most cases these can probably be explained by the K&R-style of declarations that OpenMAMA uses as a standard.  (Unfortunately, in my opinion ;-(
- Similarly, there are a bunch of "reassigned a value before the old one has been used", likely with the same cause.
- There are also a bunch of "argument ... names different" warnings -- at least one of these has [bitten me in the past](https://github.com/OpenMAMA/OpenMAMA/issues/297).

As mentioned above, I'm very interested in the community's thoughts on where to go from here (if anywhere).

Regards,

Bill Torpey

P.S.  I'm posting this to the mailing list, rather than to the GitHub issues, but please let me know if you think that would be a more appropriate place.

P.P.S.  I'm also writing another article in my series on static analysis in which I'll be using this analysis to discuss the benefits of static analysis in general, and cppcheck in particular.  This should in no way be seen as a criticism of the OpenMAMA code -- I've been working with OpenMama (as well as DataFabric) for quite a while, and it's a truly unique and impressive effort.  So, kudos to all for making OpenMAMA what it is -- but if we can make it even better, that's to everyone's benefit.

_______________________________________________
Openmama-dev mailing list
Openmama-dev@...
https://lists.openmama.org/mailman/listinfo/openmama-dev

 

_______________________________________________
Openmama-dev mailing list
Openmama-dev@...
https://lists.openmama.org/mailman/listinfo/openmama-dev

I have some feedback about the RFC for OpenMAMA Source Discovery page: https://openmama.github.io/openmama_rfc_source_discovery.html#

I’m pretty much in agreement with delivering the source descriptions and notification encoded in MAMA messages, it significantly reduces the work required to deliver an implementation and also simplifies things for application writers.

However, I’d like to suggest that rather than build a new API around a notional “source discovery object” as the RFC suggests, we simply make requests on the existing market data subscription API and differentiate by specifying a “special” source name (similar to how dictionary subscriptions are made ) This will work for both initial “discovered source” messages and updates to source status. This approach works well enough in the Tick42 TREP bridge. The main benefit of this approach is that in its simplest form it requires NO code to be written in the mama infrastructure and avoids having to replicate the API across each of the language bindings.

All the additional code devolves to the bridge implementation – this has to be written anyway. The special source name can be either fixed, hard coded in some way, or configurable through the mama.properties file. Although we would probably all agree as programmers it should be configurable, I can’t really think of a strong reason why J

I think that defining the fields as properties as indicated is necessary as it effectively defines the message schema but there are a couple of potential issues that are not covered by the RFC

• Fid clashes. If the fids are defined as properties by the mama infrastructure there is a risk that they will overlap with fids defined in a dictionary posted by a bridge. This may not necessarily be a problem as the context may resolve any such clashes but nevertheless it might be worth exploring ways to avoid this
• I think the fields need to be typed. Mostly they will be strings but some might be bool or int. This requires another property for each.

Is it possible to attribute a source with a data dictionary? I suspect that currently the assumptions made about field names in some of the MAMDA code effectively limit to a single data dictionary. This is an area that needs to be carefully explored and potentially tidied up.

Best regards

Tom Doust

From: openmama-dev-bounces@... [mailto:openmama-dev-bounces@...] On Behalf Of Frank Quinn
Sent: 13 September 2017 11:53
To: openmama-dev <openmama-dev@...>
Subject: [Openmama-dev] RFC for Source Discovery