Merge pull request #352 from dawagner/fix-showMapping-segfault

Fix a segfault in the showMapping command.
intel · Feb 16, 2016 · 155f46f · 155f46f
2 parents 3d38fee + 4d07de1
commit 155f46f
Show file tree

Hide file tree

Showing 12 changed files with 123 additions and 59 deletions.
diff --git 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
@@ -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
@@ -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
@@ -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
@@ -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,38 +185,38 @@ 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<string> mappings;
     list<const CConfigurableElement *>::const_reverse_iterator it;
     for (it = configurableElementPath.rbegin(); it != configurableElementPath.rend(); ++it) {
 
-        const CInstanceConfigurableElement *pInstanceConfigurableElement =
-            static_cast<const CInstanceConfigurableElement *>(*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
 const CSubsystemObject *CSubsystem::findSubsystemObjectFromConfigurableElement(
     const CInstanceConfigurableElement *pInstanceConfigurableElement) const
 {
 
-    const CSubsystemObject *pSubsystemObject = NULL;
-
     list<CSubsystemObject *>::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(
@@ -277,14 +276,16 @@ string CSubsystem::getMapping(list<const CConfigurableElement *> &configurableEl
     // Get the first element, which is the element containing the amended mapping
     const CInstanceConfigurableElement *pInstanceConfigurableElement =
         static_cast<const CInstanceConfigurableElement *>(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;
 }
@@ -330,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
@@ -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
@@ -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
@@ -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
@@ -31,6 +31,8 @@
 #include "MappingData.h"
 #include "Tokenizer.h"
 #include "InstanceConfigurableElement.h"
+#include "Utility.h"
+#include <list>
 #include <assert.h>
 
 #define base CElement
@@ -141,6 +143,30 @@ CMappingData *CTypeElement::getMappingData()
     return _pMappingData;
 }
 
+std::string CTypeElement::getFormattedMapping(const CTypeElement *predecessor) const
+{
+    std::list<std::string> 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
@@ -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
@@ -547,44 +547,62 @@ struct MappingPF : public ParameterFramework
 {
     MappingPF() : ParameterFramework{getConfig()} { REQUIRE_NOTHROW(start()); }
 
+    struct TestVector
+    {
+        string path;
+        string humanReadable;
+        list<string> valid;
+        list<string> invalid;
+    };
+
+    list<TestVector> 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 = "<BooleanParameter Name='bool' Mapping='bool_map'>";
-        config.subsystemMapping = "subsystem_mapping";
+        config.subsystemMapping = "rootK:rootV";
+        config.components = "<ComponentType   Name='componentType' Mapping='typeK:typeV'        />"
+                            "<ComponentType   Extends='componentType' Name='derivedComponentType' "
+                            "Mapping='derivedK:derivedV' />";
+        config.instances = "<BooleanParameter Name='param'         Mapping='paramK:paramV'      />"
+                           "<Component        Name='component'     Mapping='instanceK:instanceV'  "
+                           "           Type='derivedComponentType'                              />";
         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 = "<ComponentType   Name='componentType' Mapping='typeK:typeV'        />";
-        config.instances = "<BooleanParameter Name='param'         Mapping='paramK:paramV'      />"
-                           "<Component        Name='component'     Mapping='instanceK:instanceV'  "
-                           "           Type='componentType'                                     />";
-        ParameterFramework pf{config};
-        REQUIRE_NOTHROW(pf.start());
-
-        struct TestVector
-        {
-            string path;
-            list<string> valid;
-            list<string> invalid;
-        };
-        list<TestVector> 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<CommandHandlerInterface>(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
@@ -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. */