{"action":"created","issue":{"url":"https://api.github.com/repos/01org/parameter-framework/issues/158","labels_url":"https://api.github.com/repos/01org/parameter-framework/issues/158/labels{/name}","comments_url":"https://api.github.com/repos/01org/parameter-framework/issues/158/comments","events_url":"https://api.github.com/repos/01org/parameter-framework/issues/158/events","html_url":"https://github.com/01org/parameter-framework/pull/158","id":100264646,"number":158,"title":"Fix PFW core KlocWork issues","user":{"login":"wzairix","id":13729748,"avatar_url":"https://avatars.githubusercontent.com/u/13729748?v=3","gravatar_id":"","url":"https://api.github.com/users/wzairix","html_url":"https://github.com/wzairix","followers_url":"https://api.github.com/users/wzairix/followers","following_url":"https://api.github.com/users/wzairix/following{/other_user}","gists_url":"https://api.github.com/users/wzairix/gists{/gist_id}","starred_url":"https://api.github.com/users/wzairix/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/wzairix/subscriptions","organizations_url":"https://api.github.com/users/wzairix/orgs","repos_url":"https://api.github.com/users/wzairix/repos","events_url":"https://api.github.com/users/wzairix/events{/privacy}","received_events_url":"https://api.github.com/users/wzairix/received_events","type":"User","site_admin":false},"labels":[],"state":"open","locked":false,"assignee":null,"milestone":null,"comments":2,"created_at":"2015-08-11T09:05:53Z","updated_at":"2015-08-27T08:12:49Z","closed_at":null,"pull_request":{"url":"https://api.github.com/repos/01org/parameter-framework/pulls/158","html_url":"https://github.com/01org/parameter-framework/pull/158","diff_url":"https://github.com/01org/parameter-framework/pull/158.diff","patch_url":"https://github.com/01org/parameter-framework/pull/158.patch"},"body":"PFW core contains some critical KW issues\r\nof type NPD.FUNC.MUST .\r\n\r\nThis patch fixes these KW issues by adding a check\r\non pointers address before their use.\r\n\r\nSigned-off-by: wzairix <wajdix.zairi@intel.com>\n<a href='#crh-start'></a><a href='#crh-data-%7B%22processed%22%3A%20%5B%22https%3A//github.com/01org/parameter-framework/pull/158%23discussion_r36745935%22%2C%20%22https%3A//github.com/01org/parameter-framework/pull/158%23discussion_r36833459%22%2C%20%22https%3A//github.com/01org/parameter-framework/pull/158%23discussion_r37314583%22%2C%20%22https%3A//github.com/01org/parameter-framework/pull/158%23discussion_r37395648%22%2C%20%22https%3A//github.com/01org/parameter-framework/pull/158%23discussion_r37395836%22%2C%20%22https%3A//github.com/01org/parameter-framework/pull/158%23discussion_r37396309%22%2C%20%22https%3A//github.com/01org/parameter-framework/pull/158%23issuecomment-133064019%22%2C%20%22https%3A//github.com/01org/parameter-framework/pull/158%23issuecomment-135335771%22%5D%2C%20%22comments%22%3A%20%7B%22Pull%20f47e16dd4fd50204618c6fa423c57611837e6ebc%20parameter/DomainConfiguration.cpp%2073%22%3A%20%7B%22html_url%22%3A%20%22https%3A//github.com/01org/parameter-framework/pull/158%23discussion_r37395648%22%2C%20%22comments%22%3A%20%5B%7B%22body%22%3A%20%22As%20far%20as%20y%20know%2C%20replacing%20the%20assert%20by%20abort%20as%20you%20did%20should%20remove%20the%20need%20for%20those%20%60pChildAreaConfiguration%20%21%3D%20NULL%60%20tests.%20Could%20you%20confirm/infirm%20%3F%22%2C%20%22created_at%22%3A%20%222015-08-19T09%3A28%3A51Z%22%2C%20%22user%22%3A%20%7B%22avatar_url%22%3A%20%22https%3A//avatars.githubusercontent.com/u/6862950%3Fv%3D3%22%2C%20%22html_url%22%3A%20%22https%3A//github.com/krocard%22%7D%7D%5D%2C%20%22title%22%3A%20%22File%3A%20parameter/DomainConfiguration.cpp%3AL498-510%22%7D%2C%20%22General%20Comment%22%3A%20%7B%22html_url%22%3A%20%22https%3A//github.com/01org/parameter-framework/pull/158%23issuecomment-133064019%22%2C%20%22comments%22%3A%20%5B%7B%22body%22%3A%20%22%3A%2B1%3A%20Thanks%20for%20this%20pull%20request.%20Will%20be%20merged%20after%20%40OznOg%20review.%22%2C%20%22created_at%22%3A%20%222015-08-20T16%3A15%3A22Z%22%2C%20%22user%22%3A%20%7B%22avatar_url%22%3A%20%22https%3A//avatars.githubusercontent.com/u/6862950%3Fv%3D3%22%2C%20%22html_url%22%3A%20%22https%3A//github.com/krocard%22%7D%7D%2C%20%7B%22body%22%3A%20%22%3A%2B1%3A%20%22%2C%20%22created_at%22%3A%20%222015-08-27T08%3A12%3A49Z%22%2C%20%22user%22%3A%20%7B%22avatar_url%22%3A%20%22https%3A//avatars.githubusercontent.com/u/820313%3Fv%3D3%22%2C%20%22html_url%22%3A%20%22https%3A//github.com/OznOg%22%7D%7D%5D%2C%20%22title%22%3A%20%22General%20Comment%22%7D%2C%20%22Pull%20f47e16dd4fd50204618c6fa423c57611837e6ebc%20parameter/DomainConfiguration.cpp%2091%22%3A%20%7B%22html_url%22%3A%20%22https%3A//github.com/01org/parameter-framework/pull/158%23discussion_r37395836%22%2C%20%22comments%22%3A%20%5B%7B%22body%22%3A%20%22To%20my%20opinion%20those%202%20assert%20-%3E%20abort%20substitution%20should%20be%20enough%20to%20avoid%20klockwork%20critical.%22%2C%20%22created_at%22%3A%20%222015-08-19T09%3A31%3A14Z%22%2C%20%22user%22%3A%20%7B%22avatar_url%22%3A%20%22https%3A//avatars.githubusercontent.com/u/6862950%3Fv%3D3%22%2C%20%22html_url%22%3A%20%22https%3A//github.com/krocard%22%7D%7D%5D%2C%20%22title%22%3A%20%22File%3A%20parameter/DomainConfiguration.cpp%3AL604-611%22%7D%2C%20%22Pull%20ffd0bde8bc5c3dee558526a3323741711463b58f%20parameter/DomainConfiguration.cpp%206%22%3A%20%7B%22html_url%22%3A%20%22https%3A//github.com/01org/parameter-framework/pull/158%23discussion_r36745935%22%2C%20%22comments%22%3A%20%5B%7B%22body%22%3A%20%22I%20must%20say%20I%20do%20not%20really%20like%20this%20way%20of%20fixing%20this.%20With%20your%20way%2C%20instead%20of%20segfaulting%20the%20code%20will%20behave%20erratically%20in%20case%20of%20error.%20Could%20getAreaConfiguration%20avoid%20returning%20NULL%20%3F%20Maybe%20we%20could%20call%20explicitly%20%60abort%28%29%60%20instead%20of%20%60assert%280%29%60%20in%20getAreaConfiguration%20%3F%22%2C%20%22created_at%22%3A%20%222015-08-11T13%3A46%3A56Z%22%2C%20%22user%22%3A%20%7B%22avatar_url%22%3A%20%22https%3A//avatars.githubusercontent.com/u/6862950%3Fv%3D3%22%2C%20%22html_url%22%3A%20%22https%3A//github.com/krocard%22%7D%7D%2C%20%7B%22body%22%3A%20%22Refer%20to%20getAreaConfiguration%28%29%20function%20implementation%20we%20cannot%20avoid%20the%20NULL%20return%20since%20the%20function%20should%20retrun%20an%20address.%20But%20i%20agree%20with%20you%20%20about%20system%20behaviour%20in%20error%20case.%20%20We%20can%20abort%20in%20%20to%20getAreaConfiguration%28%29%20avoid%20this%20case.%22%2C%20%22created_at%22%3A%20%222015-08-12T07%3A38%3A47Z%22%2C%20%22user%22%3A%20%7B%22avatar_url%22%3A%20%22https%3A//avatars.githubusercontent.com/u/13729748%3Fv%3D3%22%2C%20%22html_url%22%3A%20%22https%3A//github.com/wzairix%22%7D%7D%2C%20%7B%22body%22%3A%20%22I%20think%20that%20if%20we%20abort%2C%20or%20use%20some%20kind%20of%20%5C%22allways_assert%5C%22.%20There%20is%20some%20patches%20in%20the%20pipe%20to%20introduce%20such%20assert.%20Management%20was%20very%20clear%20not%20to%20introduce%20workaround%20for%20klockwork%20false%20positive.%20Ie%20adding%20an%20assert%20%28or%20an%20initialization%29%20is%20OK%20but%20modifying%20the%20code%20flow%20should%20be%20avoided.%22%2C%20%22created_at%22%3A%20%222015-08-18T15%3A42%3A45Z%22%2C%20%22user%22%3A%20%7B%22avatar_url%22%3A%20%22https%3A//avatars.githubusercontent.com/u/6862950%3Fv%3D3%22%2C%20%22html_url%22%3A%20%22https%3A//github.com/krocard%22%7D%7D%5D%2C%20%22title%22%3A%20%22File%3A%20parameter/DomainConfiguration.cpp%3AL397-407%22%7D%2C%20%22Pull%20f47e16dd4fd50204618c6fa423c57611837e6ebc%20parameter/DomainConfiguration.cpp%204%22%3A%20%7B%22html_url%22%3A%20%22https%3A//github.com/01org/parameter-framework/pull/158%23discussion_r37396309%22%2C%20%22comments%22%3A%20%5B%7B%22body%22%3A%20%22%60%60%60%7B.cpp%7D%5Cr%5Cn%23include%20%3Ccstdlib%3E%5Cr%5Cn%60%60%60%22%2C%20%22created_at%22%3A%20%222015-08-19T09%3A36%3A39Z%22%2C%20%22user%22%3A%20%7B%22avatar_url%22%3A%20%22https%3A//avatars.githubusercontent.com/u/6862950%3Fv%3D3%22%2C%20%22html_url%22%3A%20%22https%3A//github.com/krocard%22%7D%7D%5D%2C%20%22title%22%3A%20%22File%3A%20parameter/DomainConfiguration.cpp%3AL37-44%22%7D%7D%7D'></a>\n<a href='https://www.codereviewhub.com/'><img src='http://www.codereviewhub.com/site/github-bar.png' height=40></a>\n- [ ] <a href='#crh-comment-Pull ffd0bde8bc5c3dee558526a3323741711463b58f parameter/DomainConfiguration.cpp 6'></a> <img src='http://www.codereviewhub.com/site/github-remaining.png' height=16 width=60>&nbsp;<b><a href='https://github.com/01org/parameter-framework/pull/158#discussion_r36745935'>File: parameter/DomainConfiguration.cpp:L397-407</a></b>\n- <a href='https://github.com/krocard'><img border=0 src='https://avatars.githubusercontent.com/u/6862950?v=3' height=16 width=16'></a> I must say I do not really like this way of fixing this. With your way, instead of segfaulting the code will behave erratically in case of error. Could getAreaConfiguration avoid returning NULL ? Maybe we could call explicitly `abort()` instead of `assert(0)` in getAreaConfiguration ?\n- <a href='https://github.com/wzairix'><img border=0 src='https://avatars.githubusercontent.com/u/13729748?v=3' height=16 width=16'></a> Refer to getAreaConfiguration() function implementation we cannot avoid the NULL return since the function should retrun an address. But i agree with you  about system behaviour in error case.  We can abort in  to getAreaConfiguration() avoid this case.\n- <a href='https://github.com/krocard'><img border=0 src='https://avatars.githubusercontent.com/u/6862950?v=3' height=16 width=16'></a> I think that if we abort, or use some kind of \"allways_assert\". There is some patches in the pipe to introduce such assert. Management was very clear not to introduce workaround for klockwork false positive. Ie adding an assert (or an initialization) is OK but modifying the code flow should be avoided.\n- [ ] <a href='#crh-comment-Pull f47e16dd4fd50204618c6fa423c57611837e6ebc parameter/DomainConfiguration.cpp 73'></a> <img src='http://www.codereviewhub.com/site/github-remaining.png' height=16 width=60>&nbsp;<b><a href='https://github.com/01org/parameter-framework/pull/158#discussion_r37395648'>File: parameter/DomainConfiguration.cpp:L498-510</a></b>\n- <a href='https://github.com/krocard'><img border=0 src='https://avatars.githubusercontent.com/u/6862950?v=3' height=16 width=16'></a> As far as y know, replacing the assert by abort as you did should remove the need for those `pChildAreaConfiguration != NULL` tests. Could you confirm/infirm ?\n- [ ] <a href='#crh-comment-Pull f47e16dd4fd50204618c6fa423c57611837e6ebc parameter/DomainConfiguration.cpp 91'></a> <img src='http://www.codereviewhub.com/site/github-remaining.png' height=16 width=60>&nbsp;<b><a href='https://github.com/01org/parameter-framework/pull/158#discussion_r37395836'>File: parameter/DomainConfiguration.cpp:L604-611</a></b>\n- <a href='https://github.com/krocard'><img border=0 src='https://avatars.githubusercontent.com/u/6862950?v=3' height=16 width=16'></a> To my opinion those 2 assert -> abort substitution should be enough to avoid klockwork critical.\n- [ ] <a href='#crh-comment-Pull f47e16dd4fd50204618c6fa423c57611837e6ebc parameter/DomainConfiguration.cpp 4'></a> <img src='http://www.codereviewhub.com/site/github-remaining.png' height=16 width=60>&nbsp;<b><a href='https://github.com/01org/parameter-framework/pull/158#discussion_r37396309'>File: parameter/DomainConfiguration.cpp:L37-44</a></b>\n- <a href='https://github.com/krocard'><img border=0 src='https://avatars.githubusercontent.com/u/6862950?v=3' height=16 width=16'></a> \n```{.cpp}\n#include <cstdlib>\n```\n- [x] <a href='#crh-comment-General Comment'></a> <img src='http://www.codereviewhub.com/site/github-completed.png' height=16 width=60>&nbsp;<b><a href='https://github.com/01org/parameter-framework/pull/158#issuecomment-133064019'>General Comment</a></b>\n- <a href='https://github.com/krocard'><img border=0 src='https://avatars.githubusercontent.com/u/6862950?v=3' height=16 width=16'></a> :+1: Thanks for this pull request. Will be merged after @OznOg review.\n\n\n<a href='https://www.codereviewhub.com/01org/parameter-framework/pull/158?mark_as_completed=1'><img src='http://www.codereviewhub.com/site/github-mark-as-completed.png' height=26></a>&nbsp;<a href='https://www.codereviewhub.com/01org/parameter-framework/pull/158?approve=1'><img src='http://www.codereviewhub.com/site/github-approve.png' height=26></a>&nbsp;<a href='https://github.com/01org/parameter-framework/pull/158'><img src='http://www.codereviewhub.com/site/github-refresh.png' height=26></a>\n<a href='#crh-end'></a>"},"comment":{"url":"https://api.github.com/repos/01org/parameter-framework/issues/comments/135335771","html_url":"https://github.com/01org/parameter-framework/pull/158#issuecomment-135335771","issue_url":"https://api.github.com/repos/01org/parameter-framework/issues/158","id":135335771,"user":{"login":"OznOg","id":820313,"avatar_url":"https://avatars.githubusercontent.com/u/820313?v=3","gravatar_id":"","url":"https://api.github.com/users/OznOg","html_url":"https://github.com/OznOg","followers_url":"https://api.github.com/users/OznOg/followers","following_url":"https://api.github.com/users/OznOg/following{/other_user}","gists_url":"https://api.github.com/users/OznOg/gists{/gist_id}","starred_url":"https://api.github.com/users/OznOg/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/OznOg/subscriptions","organizations_url":"https://api.github.com/users/OznOg/orgs","repos_url":"https://api.github.com/users/OznOg/repos","events_url":"https://api.github.com/users/OznOg/events{/privacy}","received_events_url":"https://api.github.com/users/OznOg/received_events","type":"User","site_admin":false},"created_at":"2015-08-27T08:12:49Z","updated_at":"2015-08-27T08:12:49Z","body":":+1: "}}