Change signature provider options callback

sciascid · sciascid · commit 37de57ef269e · 2025-04-03T10:14:21.000+02:00
Change provider options callback to take an instance
provider_options.
Restore method provider_options::intial_options().
diff --git a/include/wsrep/provider.hpp b/include/wsrep/provider.hpp
@@ -521,8 +521,7 @@ namespace wsrep
         static std::unique_ptr<provider> make_provider(
             wsrep::server_state&,
             const std::string& provider_spec,
-            const std::function<std::string()>& provider_options_cb,
-            provider_options& options,
+            const std::function<std::string(provider_options&)>& provider_options_cb,
             const wsrep::provider::services& services
             = wsrep::provider::services());
     protected:
diff --git a/include/wsrep/provider_options.hpp b/include/wsrep/provider_options.hpp
@@ -224,9 +224,8 @@ namespace wsrep
          * Individual options should be accessed through set()/get().
          *
          * @return Provider status code.
-         * @deprecated Provider options are now loaded in provider ctor
          */
-        enum wsrep::provider::status initial_options();
+        enum wsrep::provider::status initial_options(wsrep::provider& provider);
 
         /**
          * Get the option with the given name
@@ -243,8 +242,8 @@ namespace wsrep
          *         not be allocated for the new value.
          */
         enum wsrep::provider::status set(wsrep::provider& provider,
-                                          const std::string& name,
-                                          std::unique_ptr<option_value> value);
+                                         const std::string& name,
+                                         std::unique_ptr<option_value> value);
 
         /**
          * Create a new option with default value.
diff --git a/include/wsrep/server_state.hpp b/include/wsrep/server_state.hpp
@@ -297,16 +297,15 @@ namespace wsrep
          * @return Zero on success, non-zero on error.
          */
         int load_provider(const std::string& provider,
-                          const std::function<std::string()>& provider_options_cb,
+                          const std::function<std::string(provider_options&)>&,
                           const wsrep::provider::services& services
                           = wsrep::provider::services());
 
-
         /**
          * Load WSRep provider.
          *
          * @param provider WSRep provider library to be loaded.
-         * @param provider_options Provider specific options string
+         * @param options Provider specific options string
          *        to be passed for provider during initialization.
          * @param services Application defined services passed to
          *                 the provider.
@@ -316,13 +315,13 @@ namespace wsrep
          * @note Provided for backward compatibility.
          */
         int load_provider(const std::string& provider,
-                          const std::string& provider_options,
+                          const std::string& options,
                           const wsrep::provider::services& services
                           = wsrep::provider::services())
         {
-            return load_provider(provider,
-                                [provider_options]() { return provider_options; },
-                                services);
+            return load_provider(
+                provider, [options](provider_options&) { return options; },
+                services);
         }
 
         using provider_factory_func =
@@ -357,7 +356,7 @@ namespace wsrep
             return *provider_;
         }
 
-        wsrep::provider_options* provider_options()
+        wsrep::provider_options* get_provider_options()
         {
             return &provider_options_;
         }
diff --git a/src/config_service_v1.cpp b/src/config_service_v1.cpp
@@ -179,3 +179,10 @@ int wsrep::config_service_v1_fetch(struct wsrep_st* wsrep,
 
     return 0;
 }
+
+int wsrep::config_service_v1_fetch(wsrep::provider& provider,
+                                   wsrep::provider_options* options)
+{
+  struct wsrep_st* wsrep = (struct wsrep_st*)provider.native();
+  return config_service_v1_fetch(wsrep, options);
+}
diff --git a/src/config_service_v1.hpp b/src/config_service_v1.hpp
@@ -26,6 +26,7 @@ namespace wsrep
 {
     class provider;
     class provider_options;
+    int config_service_v1_fetch(provider& provider, provider_options* opts);
     int config_service_v1_fetch(struct wsrep_st* wsrep, provider_options* opts);
 } // namespace wsrep
 
diff --git a/src/provider.cpp b/src/provider.cpp
@@ -19,6 +19,7 @@
 
 #include "wsrep/provider.hpp"
 #include "wsrep/logger.hpp"
+#include "wsrep/provider_options.hpp"
 
 #include "wsrep_provider_v26.hpp"
 
@@ -29,29 +30,30 @@
 std::unique_ptr<wsrep::provider> wsrep::provider::make_provider(
     wsrep::server_state& server_state,
     const std::string& provider_spec,
-    const std::function<std::string()>& provider_options_cb,
-    provider_options& provider_options,
+    const std::function<std::string(provider_options&)>& provider_options_cb,
     const wsrep::provider::services& services)
 {
     try
     {
         return std::unique_ptr<wsrep::provider>(new wsrep::wsrep_provider_v26(
-            server_state, provider_spec, provider_options_cb, provider_options,
+            server_state, provider_spec, provider_options_cb,
             services));
     }
     catch (const wsrep::runtime_error& e)
     {
+        provider_options opts;
         wsrep::log_error() << "Failed to create a new provider '"
                            << provider_spec << "'"
-                           << " with options '" << provider_options_cb()
+                           << " with options '" << provider_options_cb(opts)
                            << "': " << e.what();
     }
     catch (...)
     {
+        provider_options opts;
         wsrep::log_error() << "Caught unknown exception when trying to "
                            << "create a new provider '"
                            << provider_spec << "'"
-                           << " with options '" << provider_options_cb();
+                           << " with options '" << provider_options_cb(opts);
     }
     return 0;
 }
diff --git a/src/provider_options.cpp b/src/provider_options.cpp
@@ -18,6 +18,7 @@
  */
 
 #include "wsrep/provider_options.hpp"
+#include "wsrep/provider.hpp"
 #include "config_service_v1.hpp"
 #include "wsrep/logger.hpp"
 
@@ -97,9 +98,18 @@ wsrep::provider_options::provider_options()
 {
 }
 
-enum wsrep::provider::status wsrep::provider_options::initial_options()
+enum wsrep::provider::status
+wsrep::provider_options::initial_options(wsrep::provider& provider)
 {
-  return wsrep::provider::error_not_implemented;
+    options_.clear();
+    if (config_service_v1_fetch(provider, this))
+    {
+        return wsrep::provider::error_not_implemented;
+    }
+    else
+    {
+        return wsrep::provider::success;
+    }
 }
 
 const wsrep::provider_options::option*
diff --git a/src/server_state.cpp b/src/server_state.cpp
@@ -500,13 +500,13 @@ static int apply_toi(wsrep::provider& provider,
 
 int wsrep::server_state::load_provider(
     const std::string& provider_spec,
-    const std::function<std::string()>& provider_options_cb,
+    const std::function<std::string(provider_options&)>& provider_options_cb,
     const wsrep::provider::services& services)
 {
     wsrep::log_info() << "Loading provider " << provider_spec
                       << " initial position: " << initial_position_;
     provider_ = provider_factory_(*this, provider_spec, provider_options_cb,
-                                  provider_options_, services);
+                                  services);
     return (provider_ ? 0 : 1);
 }
 
diff --git a/src/wsrep_provider_v26.cpp b/src/wsrep_provider_v26.cpp
@@ -773,8 +773,7 @@ void wsrep::wsrep_provider_v26::deinit_services()
 wsrep::wsrep_provider_v26::wsrep_provider_v26(
     wsrep::server_state& server_state,
     const std::string& provider_spec,
-    const std::function<std::string()>& provider_options_cb,
-    provider_options& options,
+    const std::function<std::string(provider_options&)>& provider_options_cb,
     const wsrep::provider::services& services)
     : provider(server_state)
     , wsrep_()
@@ -794,7 +793,8 @@ wsrep::wsrep_provider_v26::wsrep_provider_v26(
     }
 
     init_services(services);
-    config_service_v1_fetch(wsrep_, &options);
+    provider_options options;
+    config_service_v1_fetch(wsrep_, &options); // TODO check failure
 
     struct wsrep_init_args init_args;
     memset(&init_args, 0, sizeof(init_args));
@@ -803,7 +803,7 @@ wsrep::wsrep_provider_v26::wsrep_provider_v26(
     init_args.node_address = server_state_.address().c_str();
     init_args.node_incoming = server_state_.incoming_address().c_str();
     init_args.data_dir = server_state_.working_dir().c_str();
-    const auto& provider_options = provider_options_cb();
+    const auto& provider_options = provider_options_cb(options);
     init_args.options = provider_options.c_str();
     init_args.proto_ver = server_state.max_protocol_version();
     init_args.state_id = &state_id;
diff --git a/src/wsrep_provider_v26.hpp b/src/wsrep_provider_v26.hpp
@@ -34,8 +34,7 @@ namespace wsrep
         void init_services(const wsrep::provider::services& services);
         void deinit_services();
         wsrep_provider_v26(wsrep::server_state&, const std::string&,
-                           const std::function<std::string()>&,
-                           provider_options& provider_options,
+                           const std::function<std::string(provider_options&)>&,
                            const wsrep::provider::services& services);
         ~wsrep_provider_v26() WSREP_OVERRIDE;
         enum wsrep::provider::status
diff --git a/test/mock_server_state.hpp b/test/mock_server_state.hpp
@@ -261,17 +261,16 @@ namespace wsrep
             , cond_()
             , provider_()
         {
-            set_provider_factory([&](wsrep::server_state&,
-                                     const std::string&,
-                                     const std::function<std::string()>&,
-                                     wsrep::provider_options&,
-                                     const wsrep::provider::services&)
-            {
-                // The provider object is destroyed upon server state
-                // destruction, so using a raw pointer is safe.
-                provider_ = new wsrep::mock_provider(*this);
-                return std::unique_ptr<wsrep::provider>(provider_);
-            });
+            set_provider_factory(
+                [&](wsrep::server_state&, const std::string&,
+                    const std::function<std::string(wsrep::provider_options&)>&,
+                    const wsrep::provider::services&)
+                {
+                    // The provider object is destroyed upon server state
+                    // destruction, so using a raw pointer is safe.
+                    provider_ = new wsrep::mock_provider(*this);
+                    return std::unique_ptr<wsrep::provider>(provider_);
+                });
 
             const int ret WSREP_UNUSED = load_provider("mock", "");
             assert(ret == 0);

Original file line number	Diff line number	Diff line change
`@@ -26,6 +26,7 @@ namespace wsrep`
`26`	`26`	`{`
`27`	`27`	`class provider;`
`28`	`28`	`class provider_options;`
	`29`	`+ int config_service_v1_fetch(provider& provider, provider_options* opts);`
`29`	`30`	`int config_service_v1_fetch(struct wsrep_st* wsrep, provider_options* opts);`
`30`	`31`	`} // namespace wsrep`
`31`	`32`
Original file line number	Diff line number	Diff line change
`@@ -19,6 +19,7 @@`
`19`	`19`
`20`	`20`	`#include "wsrep/provider.hpp"`
`21`	`21`	`#include "wsrep/logger.hpp"`
	`22`	`+#include "wsrep/provider_options.hpp"`
`22`	`23`
`23`	`24`	`#include "wsrep_provider_v26.hpp"`
`24`	`25`
`@@ -29,29 +30,30 @@`
`29`	`30`	`std::unique_ptr<wsrep::provider> wsrep::provider::make_provider(`
`30`	`31`	`wsrep::server_state& server_state,`
`31`	`32`	`const std::string& provider_spec,`
`32`		`- const std::function<std::string()>& provider_options_cb,`
`33`		`- provider_options& provider_options,`
	`33`	`+ const std::function<std::string(provider_options&)>& provider_options_cb,`
`34`	`34`	`const wsrep::provider::services& services)`
`35`	`35`	`{`
`36`	`36`	`try`
`37`	`37`	`{`
`38`	`38`	`return std::unique_ptr<wsrep::provider>(new wsrep::wsrep_provider_v26(`
`39`		`- server_state, provider_spec, provider_options_cb, provider_options,`
	`39`	`+ server_state, provider_spec, provider_options_cb,`
`40`	`40`	`services));`
`41`	`41`	`}`
`42`	`42`	`catch (const wsrep::runtime_error& e)`
`43`	`43`	`{`
	`44`	`+ provider_options opts;`
`44`	`45`	`wsrep::log_error() << "Failed to create a new provider '"`
`45`	`46`	`<< provider_spec << "'"`
`46`		`- << " with options '" << provider_options_cb()`
	`47`	`+ << " with options '" << provider_options_cb(opts)`
`47`	`48`	`<< "': " << e.what();`
`48`	`49`	`}`
`49`	`50`	`catch (...)`
`50`	`51`	`{`
	`52`	`+ provider_options opts;`
`51`	`53`	`wsrep::log_error() << "Caught unknown exception when trying to "`
`52`	`54`	`<< "create a new provider '"`
`53`	`55`	`<< provider_spec << "'"`
`54`		`- << " with options '" << provider_options_cb();`
	`56`	`+ << " with options '" << provider_options_cb(opts);`
`55`	`57`	`}`
`56`	`58`	`return 0;`
`57`	`59`	`}`
Original file line number	Diff line number	Diff line change
`@@ -18,6 +18,7 @@`
`18`	`18`	`*/`
`19`	`19`
`20`	`20`	`#include "wsrep/provider_options.hpp"`
	`21`	`+#include "wsrep/provider.hpp"`
`21`	`22`	`#include "config_service_v1.hpp"`
`22`	`23`	`#include "wsrep/logger.hpp"`
`23`	`24`
`@@ -97,9 +98,18 @@ wsrep::provider_options::provider_options()`
`97`	`98`	`{`
`98`	`99`	`}`
`99`	`100`
`100`		`-enum wsrep::provider::status wsrep::provider_options::initial_options()`
	`101`	`+enum wsrep::provider::status`
	`102`	`+wsrep::provider_options::initial_options(wsrep::provider& provider)`
`101`	`103`	`{`
`102`		`- return wsrep::provider::error_not_implemented;`
	`104`	`+ options_.clear();`
	`105`	`+ if (config_service_v1_fetch(provider, this))`
	`106`	`+ {`
	`107`	`+ return wsrep::provider::error_not_implemented;`
	`108`	`+ }`
	`109`	`+ else`
	`110`	`+ {`
	`111`	`+ return wsrep::provider::success;`
	`112`	`+ }`
`103`	`113`	`}`
`104`	`114`
`105`	`115`	`const wsrep::provider_options::option*`
Original file line number	Diff line number	Diff line change
`@@ -500,13 +500,13 @@ static int apply_toi(wsrep::provider& provider,`
`500`	`500`
`501`	`501`	`int wsrep::server_state::load_provider(`
`502`	`502`	`const std::string& provider_spec,`
`503`		`- const std::function<std::string()>& provider_options_cb,`
	`503`	`+ const std::function<std::string(provider_options&)>& provider_options_cb,`
`504`	`504`	`const wsrep::provider::services& services)`
`505`	`505`	`{`
`506`	`506`	`wsrep::log_info() << "Loading provider " << provider_spec`
`507`	`507`	`<< " initial position: " << initial_position_;`
`508`	`508`	`provider_ = provider_factory_(*this, provider_spec, provider_options_cb,`
`509`		`- provider_options_, services);`
	`509`	`+ services);`
`510`	`510`	`return (provider_ ? 0 : 1);`
`511`	`511`	`}`
`512`	`512`