Post-review changes

4 files changed, 49 insertions, 52 deletions

diff --git a/mod/jwt.cxx b/mod/jwt.cxx
index 0c9c1f6..6176f27 100644
--- a/mod/jwt.cxx
+++ b/mod/jwt.cxx
@@ -55,7 +55,7 @@ string brep::
 gen_jwt (const options::openssl_options& o,
          const path& pk,
          const string& iss,
-         const chrono::minutes& vp,
+         const chrono::seconds& vp,
          const chrono::seconds& bd)
 {
   // Create the header.
@@ -125,12 +125,11 @@ gen_jwt (const options::openssl_options& o,
     ifdstream err (move (errp.in));
 
     vector<char> bs; // Binary signature (openssl output).
-    string et;       // Openssl stderr text.
     try
     {
       // In case of exception, skip and close input after output.
       //
-      // Note: re-open in/out so that they get automaitcally closed on
+      // Note: re-open in/out so that they get automatically closed on
       // exception.
       //
       ifdstream in (os.in.release (), fdstream_mode::skip);
@@ -155,16 +154,16 @@ gen_jwt (const options::openssl_options& o,
       {
         throw_generic_error (
           e.code ().value (),
-          ("unable to read/write openssl stdout/stdin: " + e.what ()).c_str ());
+          (string ("unable to read/write openssl stdout/stdin: ") +
+                   e.what ()).c_str ());
       }
     }
 
     if (!os.wait ())
     {
-      et = err.read_text ();
-      throw_generic_error (
-        EINVAL,
-        ("non-zero openssl exit status: " + et).c_str ());
+      string et (err.read_text ());
+      throw_generic_error (EINVAL,
+                           ("non-zero openssl exit status: " + et).c_str ());
     }
 
     err.close ();
@@ -175,7 +174,7 @@ gen_jwt (const options::openssl_options& o,
   {
     throw_generic_error (
       e.code ().value (),
-      ("unable to execute openssl: " + e.what ()).c_str ());
+      (string ("unable to execute openssl: ") + e.what ()).c_str ());
   }
   catch (const io_error& e)
   {
@@ -183,7 +182,7 @@ gen_jwt (const options::openssl_options& o,
     //
     throw_generic_error (
       e.code ().value (),
-      ("unable to read openssl stderr : " + e.what ()).c_str ());
+      (string ("unable to read openssl stderr : ") + e.what ()).c_str ());
   }
 
   return h + '.' + p + '.' + s; // Return the token.
diff --git a/mod/jwt.hxx b/mod/jwt.hxx
index 4b5a54f..550649f 100644
--- a/mod/jwt.hxx
+++ b/mod/jwt.hxx
@@ -33,7 +33,7 @@ namespace brep
   gen_jwt (const options::openssl_options&,
            const path& private_key,
            const string& issuer,
-           const std::chrono::minutes& validity_period,
+           const std::chrono::seconds& validity_period,
            const std::chrono::seconds& backdate = std::chrono::seconds (60));
 }
 
diff --git a/mod/mod-ci-github.cxx b/mod/mod-ci-github.cxx
index 8705b17..bd68452 100644
--- a/mod/mod-ci-github.cxx
+++ b/mod/mod-ci-github.cxx
@@ -216,17 +216,19 @@ github_post (T& rs, const string& ep, const brep::strings& hdrs)
     //        stdin because it will cause butl::curl to fail if the method is
     //        POST.
     //
-    // @@ TODO: let's redirect stderr like in JWT.
-    //
+    fdpipe errp (fdopen_pipe ()); // stderr pipe.
+
     curl c (path ("-"),
             path ("-"), // Write response to curl::in.
-            2,
+            process::pipe (errp.in.get (), move (errp.out)),
             curl::post, "https://api.github.com/" + ep,
             "--write-out", "{\"brep_http_status\": %{http_code}}\n",
             "--header", "Accept: application/vnd.github+json",
             "--header", "X-GitHub-Api-Version: 2022-11-28",
             move (hdr_opts));
 
+    ifdstream err (move (errp.in));
+
     // Parse the HTTP response.
     //
     int sc; // Status code.
@@ -268,40 +270,39 @@ github_post (T& rs, const string& ep, const brep::strings& hdrs)
     }
     catch (const brep::io_error& e)
     {
-      // IO failure, child exit status doesn't matter. Just wait for the
-      // process completion and throw.
+      // If the process exits with non-zero status, assume the IO error is due
+      // to that and fall through.
       //
-      c.wait ();
-
-      throw_generic_error (
+      if (c.wait ())
+      {
+        throw_generic_error (
           e.code ().value (),
-          (string ("unable to communicate with curl: ") + e.what ())
-              .c_str ());
+          (string ("unable to read curl stdout: ") + e.what ()).c_str ());
+      }
     }
     catch (const json::invalid_json_input& e)
     {
-      if (!c.wait ())
+      // If the process exits with non-zero status, assume the JSON error is
+      // due to that and fall through.
+      //
+      if (c.wait ())
       {
         throw_generic_error (
-            EINVAL,
-            ("curl failed with " + to_string (*c.exit)).c_str ());
-      }
-
-      throw runtime_error (
+          EINVAL,
           (string ("malformed JSON response from GitHub: ") + e.what ())
-              .c_str ());
+          .c_str ());
+      }
     }
 
-    // @@ TMP The odds of this failing are probably slim given that we parsed
-    //        the JSON output successfully.
-    //
     if (!c.wait ())
     {
-      throw_generic_error (
-        EINVAL,
-        ("curl failed with " + to_string (*c.exit)).c_str ());
+      string et (err.read_text ());
+      throw_generic_error (EINVAL,
+                           ("non-zero curl exit status: " + et).c_str ());
     }
 
+    err.close ();
+
     return sc;
   }
   catch (const process_error& e)
@@ -310,6 +311,14 @@ github_post (T& rs, const string& ep, const brep::strings& hdrs)
         e.code ().value (),
         (string ("unable to execute curl:") + e.what ()).c_str ());
   }
+  catch (const brep::io_error& e)
+  {
+    // Unable to read diagnostics from stderr.
+    //
+    throw_generic_error (
+      e.code ().value (),
+      (string ("unable to read curl stderr : ") + e.what ()).c_str ());
+  }
 }
 
 bool brep::ci_github::
@@ -386,16 +395,6 @@ handle (request& rq, response& rs)
     }
     catch (const json::invalid_json_input& e)
     {
-      // @@ TODO: should we write more detailed diagnostics to log? Maybe we
-      //    should do this for all unsuccessful calls to respond().
-      //
-      // Note: these exceptions end up in the apache error log.
-      //
-      //  @@ TMP Actually I was wrong, these do not end up in any logs. Pretty
-      //         sure I saw them go there but they're definitely not anymore.
-      //
-      //     See how it's done in other modules and do the same.
-      //
       throw invalid_request (400, "malformed JSON in request body");
     }
 
@@ -430,7 +429,7 @@ handle (request& rq, response& rs)
           *options_,
           options_->ci_github_app_private_key (),
           to_string (options_->ci_github_app_id ()),
-          chrono::minutes (options_->ci_github_jwt_validity_period ()),
+          chrono::seconds (options_->ci_github_jwt_validity_period ()),
           chrono::seconds (60));
 
       cout << "JWT: " << jwt << endl;
@@ -471,11 +470,11 @@ handle (request& rq, response& rs)
     }
     catch (const system_error& e)
     {
-      fail << "unable to get installation access token: [" << e.code ()
-           << "] " << e.what ();
+      fail << "unable to get installation access token (errno=" << e.code ()
+           << "): " << e.what ();
     }
 
-    cout << "<installation_access_token>" << endl << iat << endl;
+    cout << endl << "<installation_access_token>" << endl << iat << endl;
 
     return true;
   }
@@ -607,7 +606,6 @@ operator<< (ostream& os, const check_suite_event& cs)
   os << "<check_suite>" << endl << cs.check_suite;
   os << "<repository>" << endl << cs.repository;
   os << "<installation>" << endl << cs.installation;
-  os << endl;
 
   return os;
 }
diff --git a/mod/module.cli b/mod/module.cli
index 5c909f0..a93ff68 100644
--- a/mod/module.cli
+++ b/mod/module.cli
@@ -836,10 +836,10 @@ namespace brep
          the GitHub app's settings."
       }
 
-      uint16_t ci-github-jwt-validity-period = 10 // @@ Seconds.
+      uint16_t ci-github-jwt-validity-period = 600
       {
-        "<minutes>",
-        "The number of minutes a JWT (authentication token) should be valid for.
+        "<seconds>",
+        "The number of seconds a JWT (authentication token) should be valid for.
          The maximum allowed by GitHub is 10 minutes."
       }
     };