address review comments and change error handling for libcurl

Signed-off-by: William Woodall <[email protected]>
ros · Dec 14, 2024 · 4ffb1bc · 4ffb1bc
1 parent 7c30250
commit 4ffb1bc
Show file tree

Hide file tree

Showing 6 changed files with 41 additions and 22 deletions.
diff --git a/resource_retriever/include/resource_retriever/plugins/curl_retriever.hpp b/resource_retriever/include/resource_retriever/plugins/curl_retriever.hpp
@@ -42,6 +42,10 @@ namespace resource_retriever::plugins
 class RESOURCE_RETRIEVER_PUBLIC CurlRetriever : public RetrieverPlugin
 {
 public:
+  /// Construct a CurlRetriever plugin and initialize libcurl.
+  /**
+   * \throws std::runtime_error if libcurl fails to initialize
+   */
   CurlRetriever();
   ~CurlRetriever() override;
 

diff --git a/resource_retriever/src/plugins/curl_retriever.cpp b/resource_retriever/src/plugins/curl_retriever.cpp
@@ -30,10 +30,9 @@
 
 #include <curl/curl.h>
 
-#include <array>
 #include <cstring>
-#include <iostream>
 #include <memory>
+#include <stdexcept>
 #include <string>
 #include <utility>
 #include <vector>
@@ -42,16 +41,13 @@
 
 namespace
 {
-
 class CURLStaticInit
 {
 public:
   CURLStaticInit()
   {
-    CURLcode ret = curl_global_init(CURL_GLOBAL_ALL);
-    if (ret != 0) {
-      std::cerr << "Error initializing libcurl! retcode = " << ret;
-    } else {
+    ret_ = curl_global_init(CURL_GLOBAL_ALL);
+    if (ret_ == CURLE_OK) {
       initialized_ = true;
     }
   }
@@ -63,8 +59,19 @@ class CURLStaticInit
     }
   }
 
+  /// Check that libcurl is globally initialized, otherwise throw.
+  void check_if_initialized() const
+  {
+    if (!this->initialized_) {
+      throw std::runtime_error(
+        "curl_global_init(CURL_GLOBAL_ALL) failed (" + std::to_string(ret_) + "): " +
+        curl_easy_strerror(ret_));
+    }
+  }
+
 private:
   bool initialized_ {false};
+  CURLcode ret_ = CURLE_OK;
 };
 CURLStaticInit g_curl_init;
 }  // namespace
@@ -86,8 +93,14 @@ size_t curlWriteFunc(void * buffer, size_t size, size_t nmemb, void * userp)
   return size * nmemb;
 }
 
+CURL * do_curl_easy_init()
+{
+  ::g_curl_init.check_if_initialized();
+  return curl_easy_init();
+}
+
 CurlRetriever::CurlRetriever()
-:curl_handle_(curl_easy_init())
+: curl_handle_(do_curl_easy_init())
 {
 }
 
@@ -116,10 +129,11 @@ std::string CurlRetriever::name()
 
 bool CurlRetriever::can_handle(const std::string & url)
 {
-  return  url.find("package://") == 0 ||
-         url.find("file://") == 0 ||
-         url.find("http://") == 0 ||
-         url.find("https://") == 0;
+  return
+    url.find("package://") == 0 ||
+    url.find("file://") == 0 ||
+    url.find("http://") == 0 ||
+    url.find("https://") == 0;
 }
 
 MemoryResourcePtr CurlRetriever::get(const std::string & url)

diff --git a/resource_retriever/src/plugins/filesystem_retriever.cpp b/resource_retriever/src/plugins/filesystem_retriever.cpp
@@ -28,12 +28,9 @@
 
 #include "resource_retriever/plugins/filesystem_retriever.hpp"
 
-#include <array>
-#include <cstring>
 #include <fstream>
 #include <memory>
 #include <string>
-#include <utility>
 #include <vector>
 
 #include "resource_retriever/exception.hpp"

diff --git a/resource_retriever/src/plugins/retriever_plugin.cpp b/resource_retriever/src/plugins/retriever_plugin.cpp
@@ -28,7 +28,8 @@
 
 #include "resource_retriever/plugins/retriever_plugin.hpp"
 
-#include <cstring>
+#include <string>
+#include <string_view>
 
 #include "resource_retriever/exception.hpp"
 
@@ -59,12 +60,15 @@ std::string escape_spaces(const std::string & url)
 
 std::string expand_package_url(const std::string & url)
 {
+  constexpr std::string_view package_url_prefix = "package://";
   std::string mod_url = url;
-  if (url.find("package://") == 0) {
-    mod_url.erase(0, strlen("package://"));
+  if (url.find(package_url_prefix) == 0) {
+    mod_url.erase(0, package_url_prefix.length());
     size_t pos = mod_url.find('/');
     if (pos == std::string::npos) {
-      throw Exception(url, "Could not parse package:// format into file:// format");
+      throw Exception(
+        url,
+        "Could not parse " + std::string(package_url_prefix) + " format into file:// format");
     }
 
     std::string package = mod_url.substr(0, pos);

diff --git a/resource_retriever/src/retriever.cpp b/resource_retriever/src/retriever.cpp
@@ -28,11 +28,9 @@
 
 #include "resource_retriever/retriever.hpp"
 
-#include <cstring>
 #include <memory>
 #include <string>
 #include <utility>
-#include <vector>
 
 #include "resource_retriever/exception.hpp"
 #include "resource_retriever/plugins/curl_retriever.hpp"

diff --git a/resource_retriever/test/test.cpp b/resource_retriever/test/test.cpp
@@ -26,8 +26,10 @@
 // ARISING IN ANY WAY OUT OF THE USE OF THIS SOFTWARE, EVEN IF ADVISED OF THE
 // POSSIBILITY OF SUCH DAMAGE.
 
-#include <string>
 #include <exception>
+#include <memory>
+#include <string>
+#include <vector>
 
 #include "gtest/gtest.h"