From 398f561c7bc38f5491e84dd17a76445d80cac1b3 Mon Sep 17 00:00:00 2001 From: David Wagner Date: Mon, 15 Feb 2016 14:14:26 +0100 Subject: [PATCH 1/2] findSubsystemObjectFromConfigurableElement: return nullptr if no object found This function was improperly returning the latest candidate in case of failure instead of returning a nullptr. This led the caller to use the returned pointer even if it was not correct. Signed-off-by: David Wagner --- parameter/Subsystem.cpp | 9 ++++----- 1 file changed, 4 insertions(+), 5 deletions(-) diff --git a/parameter/Subsystem.cpp b/parameter/Subsystem.cpp index caeef7149..27d57ac90 100644 --- a/parameter/Subsystem.cpp +++ b/parameter/Subsystem.cpp @@ -203,21 +203,20 @@ const CSubsystemObject *CSubsystem::findSubsystemObjectFromConfigurableElement( const CInstanceConfigurableElement *pInstanceConfigurableElement) const { - const CSubsystemObject *pSubsystemObject = NULL; - list::const_iterator it; for (it = _subsystemObjectList.begin(); it != _subsystemObjectList.end(); ++it) { // Check if one of the SubsystemObjects is associated with a ConfigurableElement // corresponding to the expected one - pSubsystemObject = *it; + const CSubsystemObject *pSubsystemObject = *it; + if (pSubsystemObject->getConfigurableElement() == pInstanceConfigurableElement) { - break; + return pSubsystemObject; } } - return pSubsystemObject; + return nullptr; } void CSubsystem::findSubsystemLevelMappingKeyValue( From 4d07de178fe766ec4acb7610836485b0211da7ad Mon Sep 17 00:00:00 2001 From: David Wagner Date: Fri, 12 Feb 2016 18:01:44 +0100 Subject: [PATCH 2/2] Fix and test the 'showMapping' command It was segfaulting due to an erroneous static_cast. Simplify the code related to showMapping by taking into account the fact that all ConfigurableElements may have a mapping (SystemClass currently doesn't have any but it can simply return an empty mapping). The leaf element is a special case because the instantiation mapping may be formatted using "amends" - other mappings may not. Signed-off-by: David Wagner --- parameter/ComponentInstance.cpp | 9 +-- parameter/ComponentType.cpp | 9 +-- parameter/ConfigurableElement.h | 5 ++ parameter/InstanceConfigurableElement.h | 2 +- parameter/Subsystem.cpp | 31 +++++--- parameter/Subsystem.h | 1 + parameter/SystemClass.cpp | 5 ++ parameter/SystemClass.h | 1 + parameter/TypeElement.cpp | 26 +++++++ parameter/TypeElement.h | 11 +++ test/functional-tests/Handle.cpp | 72 ++++++++++++------- .../include/ParameterFramework.hpp | 1 + 12 files changed, 119 insertions(+), 54 deletions(-) diff --git a/parameter/ComponentInstance.cpp b/parameter/ComponentInstance.cpp index 3489e1e89..22fcdcc86 100644 --- a/parameter/ComponentInstance.cpp +++ b/parameter/ComponentInstance.cpp @@ -73,14 +73,7 @@ bool CComponentInstance::hasMappingData() const std::string CComponentInstance::getFormattedMapping() const { - // Try myself first then associated component type - std::string strValue = base::getFormattedMapping(); - if (_pComponentType) { - - strValue += _pComponentType->getFormattedMapping(); - } - - return strValue; + return base::getFormattedMapping(_pComponentType); } bool CComponentInstance::fromXml(const CXmlElement &xmlElement, diff --git a/parameter/ComponentType.cpp b/parameter/ComponentType.cpp index 3bd92849b..76fb21190 100644 --- a/parameter/ComponentType.cpp +++ b/parameter/ComponentType.cpp @@ -65,14 +65,7 @@ bool CComponentType::hasMappingData() const std::string CComponentType::getFormattedMapping() const { - // Try myself first then associated component type - std::string strValue = base::getFormattedMapping(); - if (_pExtendsComponentType) { - - strValue += _pExtendsComponentType->getFormattedMapping(); - } - - return strValue; + return base::getFormattedMapping(_pExtendsComponentType); } bool CComponentType::fromXml(const CXmlElement &xmlElement, diff --git a/parameter/ConfigurableElement.h b/parameter/ConfigurableElement.h index 697c1ea26..e511447a5 100644 --- a/parameter/ConfigurableElement.h +++ b/parameter/ConfigurableElement.h @@ -154,6 +154,11 @@ class PARAMETER_EXPORT CConfigurableElement : public CElement * @return true if @p strKey is found in the object's mapping, false if not */ virtual bool getMappingData(const std::string &strKey, const std::string *&pStrValue) const = 0; + /** Get the string representation of the mapping + * + * If applicable, amend values are applied to the leaf element. + */ + virtual std::string getFormattedMapping() const = 0; // XML configuration settings parsing virtual bool serializeXmlSettings( diff --git a/parameter/InstanceConfigurableElement.h b/parameter/InstanceConfigurableElement.h index b32763bfd..0b4a7f95f 100644 --- a/parameter/InstanceConfigurableElement.h +++ b/parameter/InstanceConfigurableElement.h @@ -67,7 +67,7 @@ class PARAMETER_EXPORT CInstanceConfigurableElement : public CConfigurableElemen * * @return A std::string containing the formatted mapping */ - std::string getFormattedMapping() const; + std::string getFormattedMapping() const override; // From CElement virtual std::string getKind() const; diff --git a/parameter/Subsystem.cpp b/parameter/Subsystem.cpp index 27d57ac90..43e633dd4 100644 --- a/parameter/Subsystem.cpp +++ b/parameter/Subsystem.cpp @@ -42,7 +42,6 @@ using std::string; using std::list; -using std::ostringstream; CSubsystem::CSubsystem(const string &strName, core::log::Logger &logger) : base(strName), _pComponentLibrary(new CComponentLibrary), @@ -186,16 +185,17 @@ string CSubsystem::formatMappingDataList( { // The list is parsed in reverse order because it has been filled from the leaf to the trunk // of the tree. When formatting the mapping, we want to start from the subsystem level - ostringstream ossStream; + std::list mappings; list::const_reverse_iterator it; for (it = configurableElementPath.rbegin(); it != configurableElementPath.rend(); ++it) { - const CInstanceConfigurableElement *pInstanceConfigurableElement = - static_cast(*it); - - ossStream << pInstanceConfigurableElement->getFormattedMapping() << ", "; + auto maybeMapping = (*it)->getFormattedMapping(); + if (not maybeMapping.empty()) { + mappings.push_back(maybeMapping); + } } - return ossStream.str(); + + return utility::asString(mappings, ", "); } // Find the CSubystemObject containing a specific CInstanceConfigurableElement @@ -276,14 +276,16 @@ string CSubsystem::getMapping(list &configurableEl // Get the first element, which is the element containing the amended mapping const CInstanceConfigurableElement *pInstanceConfigurableElement = static_cast(configurableElementPath.front()); - configurableElementPath.pop_front(); - // Now the list only contains elements whose mapping are related to the context // Format context mapping data string strValue = formatMappingDataList(configurableElementPath); // Print the mapping of the first node, which corresponds to a SubsystemObject - strValue += getFormattedSubsystemMappingData(pInstanceConfigurableElement); + auto subsystemObjectAmendedMapping = + getFormattedSubsystemMappingData(pInstanceConfigurableElement); + if (not subsystemObjectAmendedMapping.empty()) { + strValue += ", " + subsystemObjectAmendedMapping; + } return strValue; } @@ -329,6 +331,15 @@ bool CSubsystem::getMappingData(const std::string &strKey, const std::string *&p return false; } +// Returns the formatted mapping +std::string CSubsystem::getFormattedMapping() const +{ + if (!_pMappingData) { + return ""; + } + return _pMappingData->asString(); +} + // Mapping generic context handling bool CSubsystem::handleMappingContext(const CConfigurableElement *pConfigurableElement, CMappingContext &context, string &strError) const diff --git a/parameter/Subsystem.h b/parameter/Subsystem.h index 34ea85d9f..af7e1fb87 100644 --- a/parameter/Subsystem.h +++ b/parameter/Subsystem.h @@ -76,6 +76,7 @@ class PARAMETER_EXPORT CSubsystem : public CConfigurableElement, private IMapper virtual std::string getKind() const; virtual bool getMappingData(const std::string &strKey, const std::string *&pStrValue) const; + std::string getFormattedMapping() const override; /** * Fetch mapping data of an element. diff --git a/parameter/SystemClass.cpp b/parameter/SystemClass.cpp index e668a6262..21c7a86c8 100644 --- a/parameter/SystemClass.cpp +++ b/parameter/SystemClass.cpp @@ -87,6 +87,11 @@ bool CSystemClass::getMappingData(const std::string & /*strKey*/, return false; } +string CSystemClass::getFormattedMapping() const +{ + return ""; +} + bool CSystemClass::loadSubsystems(string &strError, const CSubsystemPlugins *pSubsystemPlugins, bool bVirtualSubsystemFallback) { diff --git a/parameter/SystemClass.h b/parameter/SystemClass.h index 483ec1489..43a581b68 100644 --- a/parameter/SystemClass.h +++ b/parameter/SystemClass.h @@ -84,6 +84,7 @@ class CSystemClass final : public CConfigurableElement virtual std::string getKind() const; bool getMappingData(const std::string &strKey, const std::string *&pStrValue) const override; + std::string getFormattedMapping() const override; private: CSystemClass(const CSystemClass &); diff --git a/parameter/TypeElement.cpp b/parameter/TypeElement.cpp index bd4fa9515..e6b927642 100644 --- a/parameter/TypeElement.cpp +++ b/parameter/TypeElement.cpp @@ -31,6 +31,8 @@ #include "MappingData.h" #include "Tokenizer.h" #include "InstanceConfigurableElement.h" +#include "Utility.h" +#include #include #define base CElement @@ -141,6 +143,30 @@ CMappingData *CTypeElement::getMappingData() return _pMappingData; } +std::string CTypeElement::getFormattedMapping(const CTypeElement *predecessor) const +{ + std::list mappings; + std::string mapping; + + // Try predecessor type first, then myself (in order to have higher-level + // mappings displayed first) + if (predecessor) { + mapping = predecessor->getFormattedMapping(); + if (not mapping.empty()) { + mappings.push_back(mapping); + } + } + + // Explicitly call the root implementation instead of calling it virtually + // (otherwise, it will infinitely recurse). + mapping = CTypeElement::getFormattedMapping(); + if (not mapping.empty()) { + mappings.push_back(mapping); + } + + return utility::asString(mappings, ", "); +} + std::string CTypeElement::getFormattedMapping() const { if (_pMappingData) { diff --git a/parameter/TypeElement.h b/parameter/TypeElement.h index 58d9a98c8..9d8f46f39 100644 --- a/parameter/TypeElement.h +++ b/parameter/TypeElement.h @@ -84,6 +84,17 @@ class PARAMETER_EXPORT CTypeElement : public CElement protected: // Object creation virtual void populate(CElement *pElement) const; + /** @Returns the mapping associated to the current type and its predecessor + * + * The meaning of predecessor depends on the TypeElement type: e.g. for a + * component instance, the predecessor is the ComponentType; for a + * ComponentType, the predecessor is its base type. + * + * The predecessor's mapping comes first, then the current type's mapping. + * + * @param[in] predecessor A pointer to the predecessor or NULL. + */ + std::string getFormattedMapping(const CTypeElement *predecessor) const; private: CTypeElement(const CTypeElement &); diff --git a/test/functional-tests/Handle.cpp b/test/functional-tests/Handle.cpp index c309fe41e..d7aafb9ab 100644 --- a/test/functional-tests/Handle.cpp +++ b/test/functional-tests/Handle.cpp @@ -547,44 +547,62 @@ struct MappingPF : public ParameterFramework { MappingPF() : ParameterFramework{getConfig()} { REQUIRE_NOTHROW(start()); } + struct TestVector + { + string path; + string humanReadable; + list valid; + list invalid; + }; + + list testVectors = { + // clang-format off + {"/test/test", + {"rootK:rootV"}, + {"root"}, + {"param", "type", "instance", "derived"}}, + {"/test/test/param", + {"rootK:rootV, paramK:paramV"}, + {"root", "param"}, + {"type", "derived", "instance"}}, + {"/test/test/component", + {"rootK:rootV, typeK:typeV, derivedK:derivedV, instanceK:instanceV"}, + {"root", "type", "derived", "instance"}, + {"param"}} + // clang-format on + }; + Config getConfig() { Config config; - config.instances = ""; - config.subsystemMapping = "subsystem_mapping"; + config.subsystemMapping = "rootK:rootV"; + config.components = "" + ""; + config.instances = "" + ""; return config; } }; -SCENARIO("Mapping handle access", "[handler][mapping]") +SCENARIO_METHOD(MappingPF, "showMapping command", "[mapping]") { - GIVEN ("A PF with mappings") { - Config config; - config.subsystemMapping = "rootK:rootV"; - config.components = ""; - config.instances = "" - ""; - ParameterFramework pf{config}; - REQUIRE_NOTHROW(pf.start()); - - struct TestVector - { - string path; - list valid; - list invalid; - }; - list testVectors = { - // clang-format off - {"/test/test", {"root"}, {"param", "type", "instance"}}, - {"/test/test/param", {"root", "param"}, {"type", "instance"}}, - {"/test/test/component", {"root", "type", "instance"}, {"param"}} - // clang-format on - }; + auto cmdHandler = std::unique_ptr(createCommandHandler()); + for (auto &testVector : testVectors) { + string output; + CHECK(cmdHandler->process("showMapping", {testVector.path}, output)); + CHECK(output == testVector.humanReadable); + } +} + +SCENARIO_METHOD(MappingPF, "Mapping handle access", "[handler][mapping]") +{ + GIVEN ("A PF with mappings") { for (auto &test : testVectors) { GIVEN ("An element handle of " + test.path) { - ElementHandle handle(pf, test.path); + ElementHandle handle(*this, test.path); for (auto &valid : test.valid) { THEN ("The following mapping should exist: " + valid) { diff --git a/test/functional-tests/include/ParameterFramework.hpp b/test/functional-tests/include/ParameterFramework.hpp index 80d481555..f6f36a870 100644 --- a/test/functional-tests/include/ParameterFramework.hpp +++ b/test/functional-tests/include/ParameterFramework.hpp @@ -87,6 +87,7 @@ class ParameterFramework : private parameterFramework::ConfigFiles, using PF::isTuningModeOn; using PF::isAutoSyncOn; using PF::setLogger; + using PF::createCommandHandler; /** @} */ /** Wrap PF::setValidateSchemasOnStart to throw an exception on failure. */