Merge pull request #121 from mihaip/upstream-settings

Add support for specifying properties of dynamically-registered devices via the command line

devices/common/pci/pcihost.cpp

@@ -119,9 +119,8 @@ PCIBase *PCIHost::attach_pci_device(const std::string& dev_name, int slot_id, co
     }
 
     // attempt to create device object
+    MachineFactory::register_device_settings(dev_name);
     auto desc = DeviceRegistry::get_descriptor(dev_name);
-    map<string, string> settings;
-    MachineFactory::get_device_settings(desc, settings);
     auto dev_obj = desc.m_create_func();
 
     if (!dev_obj->supports_type(HWCompType::PCI_DEV)) {

machines/machinefactory.cpp

@@ -203,7 +203,7 @@ void MachineFactory::list_device_settings(DeviceDescription& dev)
     print_settings(dev.properties);
 }
 
-void MachineFactory::print_settings(PropMap& prop_map)
+void MachineFactory::print_settings(const PropMap& prop_map)
 {
     string help;
 
@@ -237,27 +237,25 @@ void MachineFactory::print_settings(PropMap& prop_map)
     }
 }
 
-void MachineFactory::get_device_settings(DeviceDescription& dev, map<string, string> &settings)
+void MachineFactory::register_device_settings(const std::string& name)
 {
+    auto dev = DeviceRegistry::get_descriptor(name);
     for (auto& d : dev.subdev_list) {
-        get_device_settings(DeviceRegistry::get_descriptor(d), settings);
+        register_device_settings(d);
     }
 
+    register_settings(dev.properties);
+    if (!dev.properties.empty()) {
+        std::cout << "Device " << name << " Settings" << endl
+                  << std::string(36, '-') << endl;
         for (auto& p : dev.properties) {
-        if (settings.count(p.first)) {
+            std::cout << std::setw(24) << std::right << p.first << " : "
-            // This is a hack. Need to implement hierarchical paths and per device properties.
+                      << gMachineSettings[p.first]->get_string() << endl;
-            LOG_F(ERROR, "Duplicate setting \"%s\".", p.first.c_str());
-        }
-        else {
-            settings[p.first] = p.second->get_string();
-
-            // populate dynamic machine settings from presets
-            gMachineSettings[p.first] = unique_ptr<BasicProperty>(p.second->clone());
         }
     }
 }
 
-int MachineFactory::get_machine_settings(const string& id, map<string, string> &settings)
+int MachineFactory::register_machine_settings(const std::string& id)
 {
     auto it = get_registry().find(id);
     if (it != get_registry().end()) {
@@ -265,15 +263,17 @@ int MachineFactory::get_machine_settings(const string& id, map<string, string> &
 
         gMachineSettings.clear();
 
-        for (auto& p : props) {
+        register_settings(props);
-            settings[p.first] = p.second->get_string();
 
-            // populate dynamic machine settings from presets
+        std::cout << endl << "Machine " << id << " Settings" << endl
-            gMachineSettings[p.first] = unique_ptr<BasicProperty>(p.second->clone());
+                   << std::string(36, '-') << endl;
+        for (auto& p : props) {
+            std::cout << std::setw(24) << std::right << p.first << " : "
+                      << gMachineSettings[p.first]->get_string() << endl;
         }
 
         for (auto& dev : it->second.devices) {
-            get_device_settings(DeviceRegistry::get_descriptor(dev), settings);
+            register_device_settings(dev);
         }
     } else {
         LOG_F(ERROR, "Unknown machine id %s", id.c_str());
@@ -283,17 +283,22 @@ int MachineFactory::get_machine_settings(const string& id, map<string, string> &
     return 0;
 }
 
-void MachineFactory::set_machine_settings(map<string, string> &settings) {
+void MachineFactory::register_settings(const PropMap& props) {
-    for (auto& s : settings) {
+    for (auto& p : props) {
-        gMachineSettings.at(s.first)->set_string(s.second);
+        if (gMachineSettings.count(p.first)) {
+            // This is a hack. Need to implement hierarchical paths and per device properties.
+            LOG_F(ERROR, "Duplicate setting \"%s\".", p.first.c_str());
+        }
+        else {
+            auto clone = p.second->clone();
+            auto override_value = get_setting_value(p.first);
+            if (override_value) {
+                clone->set_string(*override_value);
+            }
+            gMachineSettings[p.first] = std::unique_ptr<BasicProperty>(clone);
+        }
     }
 
-    // print machine settings summary
-    cout << endl << "Machine settings summary: " << endl;
-
-    for (auto& p : gMachineSettings) {
-        cout << p.first << " : " << p.second->get_string() << endl;
-    }
 }
 
 string MachineFactory::machine_name_from_rom(string& rom_filepath) {
@@ -412,3 +417,5 @@ int MachineFactory::create_machine_for_id(string& id, string& rom_filepath) {
 
     return 0;
 }
+
+GetSettingValueFunc MachineFactory::get_setting_value;

machines/machinefactory.h

@@ -29,7 +29,9 @@ along with this program.  If not, see <https://www.gnu.org/licenses/>.
 
 #include <machines/machineproperties.h>
 
+#include <functional>
 #include <map>
+#include <optional>
 #include <string>
 #include <vector>
 
@@ -45,6 +47,8 @@ struct MachineDescription {
     function<int(string&)>  init_func;
 };
 
+typedef std::function<std::optional<std::string>(const std::string&)> GetSettingValueFunc;
+
 class MachineFactory
 {
 public:
@@ -57,18 +61,20 @@ public:
     static int create(string& mach_id);
     static int create_machine_for_id(string& id, string& rom_filepath);
 
-    static void get_device_settings(DeviceDescription& dev, map<string, string> &settings);
+    static void register_device_settings(const std::string &name);
-    static int get_machine_settings(const string& id, map<string, string> &settings);
+    static int  register_machine_settings(const std::string& id);
-    static void set_machine_settings(map<string, string> &settings);
 
     static void list_machines();
     static void list_properties();
 
+    static GetSettingValueFunc get_setting_value;
+
 private:
     static void create_device(string& dev_name, DeviceDescription& dev);
-    static void print_settings(PropMap& p);
+    static void print_settings(const PropMap& p);
     static void list_device_settings(DeviceDescription& dev);
     static int  load_boot_rom(string& rom_filepath);
+    static void register_settings(const PropMap& p);
 
     static map<string, MachineDescription> & get_registry() {
         static map<string, MachineDescription> machine_registry;

main.cpp

@@ -162,21 +162,30 @@ int main(int argc, char** argv) {
         }
     }
 
-    /* handle overriding of machine settings from command line */
+    // Hook to allow properties to be read from the command-line, regardless
-    map<string, string> settings;
+    // of when they are registered.
-    if (MachineFactory::get_machine_settings(machine_str, settings) < 0) {
+    MachineFactory::get_setting_value = [&](const std::string& name) -> std::optional<std::string> {
-        return 1;
-    }
-
         CLI::App sa;
         sa.allow_extras();
 
-    for (auto& s : settings) {
+        std::string value;
-        sa.add_option("--" + s.first, s.second);
+        sa.add_option("--" + name, value);
+        try {
+            sa.parse(app.remaining_for_passthrough());
+        } catch (const CLI::Error& e) {
+            ABORT_F("Cannot parse CLI: %s", e.get_name().c_str());
         }
-    sa.parse(app.remaining_for_passthrough()); /* TODO: handle exceptions! */
 
-    MachineFactory::set_machine_settings(settings);
+        if (sa.count("--" + name) > 0) {
+            return value;
+        } else {
+            return std::nullopt;
+        }
+    };
+
+    if (MachineFactory::register_machine_settings(machine_str) < 0) {
+        return 1;
+    }
 
     cout << "BootROM path: " << bootrom_path << endl;
     cout << "Execution mode: " << execution_mode << endl;