From 75b62e52600a44b42693944b50638bf580a2c86e Mon Sep 17 00:00:00 2001 From: John Ericson Date: Thu, 18 Jun 2020 17:54:16 +0000 Subject: [PATCH] Avoid `fmt` when constructor already does it There is a correctnes issue here, but #3724 will fix that. This is just a cleanup for brevity's sake. --- src/libfetchers/mercurial.cc | 4 +-- src/libmain/shared.cc | 22 +++++++------- src/libstore/filetransfer.cc | 13 ++++---- src/libstore/local-store.cc | 6 ++-- src/libstore/sqlite.cc | 57 ++++++++++++++++++++++-------------- src/libstore/sqlite.hh | 16 ++++++++-- src/libutil/util.cc | 4 +-- src/nix/repl.cc | 2 +- 8 files changed, 73 insertions(+), 51 deletions(-) diff --git a/src/libfetchers/mercurial.cc b/src/libfetchers/mercurial.cc index 1beb8b944..5c5671681 100644 --- a/src/libfetchers/mercurial.cc +++ b/src/libfetchers/mercurial.cc @@ -36,7 +36,7 @@ static std::string runHg(const Strings & args, const std::optional auto res = runProgram(std::move(opts)); if (!statusOk(res.first)) - throw ExecError(res.first, fmt("hg %1%", statusToString(res.first))); + throw ExecError(res.first, "hg %1%", statusToString(res.first)); return res.second; } @@ -273,7 +273,7 @@ struct MercurialInputScheme : InputScheme runHg({ "recover", "-R", cacheDir }); runHg({ "pull", "-R", cacheDir, "--", actualUrl }); } else { - throw ExecError(e.status, fmt("'hg pull' %s", statusToString(e.status))); + throw ExecError(e.status, "'hg pull' %s", statusToString(e.status)); } } } else { diff --git a/src/libmain/shared.cc b/src/libmain/shared.cc index 562d1b414..31454e49d 100644 --- a/src/libmain/shared.cc +++ b/src/libmain/shared.cc @@ -60,37 +60,37 @@ void printMissing(ref store, const StorePathSet & willBuild, { if (!willBuild.empty()) { if (willBuild.size() == 1) - printMsg(lvl, fmt("this derivation will be built:")); + printMsg(lvl, "this derivation will be built:"); else - printMsg(lvl, fmt("these %d derivations will be built:", willBuild.size())); + printMsg(lvl, "these %d derivations will be built:", willBuild.size()); auto sorted = store->topoSortPaths(willBuild); reverse(sorted.begin(), sorted.end()); for (auto & i : sorted) - printMsg(lvl, fmt(" %s", store->printStorePath(i))); + printMsg(lvl, " %s", store->printStorePath(i)); } if (!willSubstitute.empty()) { const float downloadSizeMiB = downloadSize / (1024.f * 1024.f); const float narSizeMiB = narSize / (1024.f * 1024.f); if (willSubstitute.size() == 1) { - printMsg(lvl, fmt("this path will be fetched (%.2f MiB download, %.2f MiB unpacked):", + printMsg(lvl, "this path will be fetched (%.2f MiB download, %.2f MiB unpacked):", downloadSizeMiB, - narSizeMiB)); + narSizeMiB); } else { - printMsg(lvl, fmt("these %d paths will be fetched (%.2f MiB download, %.2f MiB unpacked):", + printMsg(lvl, "these %d paths will be fetched (%.2f MiB download, %.2f MiB unpacked):", willSubstitute.size(), downloadSizeMiB, - narSizeMiB)); + narSizeMiB); } for (auto & i : willSubstitute) - printMsg(lvl, fmt(" %s", store->printStorePath(i))); + printMsg(lvl, " %s", store->printStorePath(i)); } if (!unknown.empty()) { - printMsg(lvl, fmt("don't know how to build these paths%s:", - (settings.readOnlyMode ? " (may be caused by read-only store access)" : ""))); + printMsg(lvl, "don't know how to build these paths%s:", + (settings.readOnlyMode ? " (may be caused by read-only store access)" : "")); for (auto & i : unknown) - printMsg(lvl, fmt(" %s", store->printStorePath(i))); + printMsg(lvl, " %s", store->printStorePath(i)); } } diff --git a/src/libstore/filetransfer.cc b/src/libstore/filetransfer.cc index c46262299..529a41891 100644 --- a/src/libstore/filetransfer.cc +++ b/src/libstore/filetransfer.cc @@ -443,14 +443,13 @@ struct curlFileTransfer : public FileTransfer : httpStatus != 0 ? FileTransferError(err, std::move(response), - fmt("unable to %s '%s': HTTP error %d ('%s')", - request.verb(), request.uri, httpStatus, statusMsg) - + (code == CURLE_OK ? "" : fmt(" (curl error: %s)", curl_easy_strerror(code))) - ) + "unable to %s '%s': HTTP error %d%s", + request.verb(), request.uri, httpStatus, + code == CURLE_OK ? "" : fmt(" (curl error: %s)", curl_easy_strerror(code))) : FileTransferError(err, std::move(response), - fmt("unable to %s '%s': %s (%d)", - request.verb(), request.uri, curl_easy_strerror(code), code)); + "unable to %s '%s': %s (%d)", + request.verb(), request.uri, curl_easy_strerror(code), code); /* If this is a transient error, then maybe retry the download after a while. If we're writing to a @@ -704,7 +703,7 @@ struct curlFileTransfer : public FileTransfer auto s3Res = s3Helper.getObject(bucketName, key); FileTransferResult res; if (!s3Res.data) - throw FileTransferError(NotFound, {}, "S3 object '%s' does not exist", request.uri); + throw FileTransferError(NotFound, "S3 object '%s' does not exist", request.uri); res.data = std::move(*s3Res.data); callback(std::move(res)); #else diff --git a/src/libstore/local-store.cc b/src/libstore/local-store.cc index d77fff963..ece5bb5ef 100644 --- a/src/libstore/local-store.cc +++ b/src/libstore/local-store.cc @@ -482,18 +482,18 @@ void LocalStore::openDB(State & state, bool create) SQLiteStmt stmt; stmt.create(db, "pragma main.journal_mode;"); if (sqlite3_step(stmt) != SQLITE_ROW) - throwSQLiteError(db, "querying journal mode"); + SQLiteError::throw_(db, "querying journal mode"); prevMode = std::string((const char *) sqlite3_column_text(stmt, 0)); } if (prevMode != mode && sqlite3_exec(db, ("pragma main.journal_mode = " + mode + ";").c_str(), 0, 0, 0) != SQLITE_OK) - throwSQLiteError(db, "setting journal mode"); + SQLiteError::throw_(db, "setting journal mode"); /* Increase the auto-checkpoint interval to 40000 pages. This seems enough to ensure that instantiating the NixOS system derivation is done in a single fsync(). */ if (mode == "wal" && sqlite3_exec(db, "pragma wal_autocheckpoint = 40000;", 0, 0, 0) != SQLITE_OK) - throwSQLiteError(db, "setting autocheckpoint interval"); + SQLiteError::throw_(db, "setting autocheckpoint interval"); /* Initialise the database schema, if necessary. */ if (create) { diff --git a/src/libstore/sqlite.cc b/src/libstore/sqlite.cc index 1d82b4ab1..32d2fc021 100644 --- a/src/libstore/sqlite.cc +++ b/src/libstore/sqlite.cc @@ -8,22 +8,35 @@ namespace nix { -[[noreturn]] void throwSQLiteError(sqlite3 * db, const FormatOrString & fs) +template +SQLiteError::SQLiteError(const char *path, int errNo, int extendedErrNo, const Args & ... args) + : Error(""), path(path), errNo(errNo), extendedErrNo(extendedErrNo) +{ + auto hf = hintfmt(args...); + err.msg = hintfmt("%s: %s (in '%s')", + normaltxt(hf.str()), + sqlite3_errstr(extendedErrNo), + path ? path : "(in-memory)"); +} + +template +[[noreturn]] void SQLiteError::throw_(sqlite3 * db, const std::string & fs, const Args & ... args) { int err = sqlite3_errcode(db); int exterr = sqlite3_extended_errcode(db); auto path = sqlite3_db_filename(db, nullptr); - if (!path) path = "(in-memory)"; if (err == SQLITE_BUSY || err == SQLITE_PROTOCOL) { - throw SQLiteBusy( + auto exp = SQLiteBusy(path, err, exterr, fs, args...); + exp.err.msg = hintfmt( err == SQLITE_PROTOCOL - ? fmt("SQLite database '%s' is busy (SQLITE_PROTOCOL)", path) - : fmt("SQLite database '%s' is busy", path)); - } - else - throw SQLiteError("%s: %s (in '%s')", fs.s, sqlite3_errstr(exterr), path); + ? "SQLite database '%s' is busy (SQLITE_PROTOCOL)" + : "SQLite database '%s' is busy", + path ? path : "(in-memory)"); + throw exp; + } else + throw SQLiteError(path, err, exterr, fs, args...); } SQLite::SQLite(const Path & path, bool create) @@ -37,7 +50,7 @@ SQLite::SQLite(const Path & path, bool create) throw Error("cannot open SQLite database '%s'", path); if (sqlite3_busy_timeout(db, 60 * 60 * 1000) != SQLITE_OK) - throwSQLiteError(db, "setting timeout"); + SQLiteError::throw_(db, "setting timeout"); exec("pragma foreign_keys = 1"); } @@ -46,7 +59,7 @@ SQLite::~SQLite() { try { if (db && sqlite3_close(db) != SQLITE_OK) - throwSQLiteError(db, "closing database"); + SQLiteError::throw_(db, "closing database"); } catch (...) { ignoreException(); } @@ -62,7 +75,7 @@ void SQLite::exec(const std::string & stmt) { retrySQLite([&]() { if (sqlite3_exec(db, stmt.c_str(), 0, 0, 0) != SQLITE_OK) - throwSQLiteError(db, format("executing SQLite statement '%s'") % stmt); + SQLiteError::throw_(db, "executing SQLite statement '%s'", stmt); }); } @@ -76,7 +89,7 @@ void SQLiteStmt::create(sqlite3 * db, const std::string & sql) checkInterrupt(); assert(!stmt); if (sqlite3_prepare_v2(db, sql.c_str(), -1, &stmt, 0) != SQLITE_OK) - throwSQLiteError(db, fmt("creating statement '%s'", sql)); + SQLiteError::throw_(db, "creating statement '%s'", sql); this->db = db; this->sql = sql; } @@ -85,7 +98,7 @@ SQLiteStmt::~SQLiteStmt() { try { if (stmt && sqlite3_finalize(stmt) != SQLITE_OK) - throwSQLiteError(db, fmt("finalizing statement '%s'", sql)); + SQLiteError::throw_(db, "finalizing statement '%s'", sql); } catch (...) { ignoreException(); } @@ -109,7 +122,7 @@ SQLiteStmt::Use & SQLiteStmt::Use::operator () (std::string_view value, bool not { if (notNull) { if (sqlite3_bind_text(stmt, curArg++, value.data(), -1, SQLITE_TRANSIENT) != SQLITE_OK) - throwSQLiteError(stmt.db, "binding argument"); + SQLiteError::throw_(stmt.db, "binding argument"); } else bind(); return *this; @@ -119,7 +132,7 @@ SQLiteStmt::Use & SQLiteStmt::Use::operator () (const unsigned char * data, size { if (notNull) { if (sqlite3_bind_blob(stmt, curArg++, data, len, SQLITE_TRANSIENT) != SQLITE_OK) - throwSQLiteError(stmt.db, "binding argument"); + SQLiteError::throw_(stmt.db, "binding argument"); } else bind(); return *this; @@ -129,7 +142,7 @@ SQLiteStmt::Use & SQLiteStmt::Use::operator () (int64_t value, bool notNull) { if (notNull) { if (sqlite3_bind_int64(stmt, curArg++, value) != SQLITE_OK) - throwSQLiteError(stmt.db, "binding argument"); + SQLiteError::throw_(stmt.db, "binding argument"); } else bind(); return *this; @@ -138,7 +151,7 @@ SQLiteStmt::Use & SQLiteStmt::Use::operator () (int64_t value, bool notNull) SQLiteStmt::Use & SQLiteStmt::Use::bind() { if (sqlite3_bind_null(stmt, curArg++) != SQLITE_OK) - throwSQLiteError(stmt.db, "binding argument"); + SQLiteError::throw_(stmt.db, "binding argument"); return *this; } @@ -152,14 +165,14 @@ void SQLiteStmt::Use::exec() int r = step(); assert(r != SQLITE_ROW); if (r != SQLITE_DONE) - throwSQLiteError(stmt.db, fmt("executing SQLite statement '%s'", sqlite3_expanded_sql(stmt.stmt))); + SQLiteError::throw_(stmt.db, fmt("executing SQLite statement '%s'", sqlite3_expanded_sql(stmt.stmt))); } bool SQLiteStmt::Use::next() { int r = step(); if (r != SQLITE_DONE && r != SQLITE_ROW) - throwSQLiteError(stmt.db, fmt("executing SQLite query '%s'", sqlite3_expanded_sql(stmt.stmt))); + SQLiteError::throw_(stmt.db, fmt("executing SQLite query '%s'", sqlite3_expanded_sql(stmt.stmt))); return r == SQLITE_ROW; } @@ -185,14 +198,14 @@ SQLiteTxn::SQLiteTxn(sqlite3 * db) { this->db = db; if (sqlite3_exec(db, "begin;", 0, 0, 0) != SQLITE_OK) - throwSQLiteError(db, "starting transaction"); + SQLiteError::throw_(db, "starting transaction"); active = true; } void SQLiteTxn::commit() { if (sqlite3_exec(db, "commit;", 0, 0, 0) != SQLITE_OK) - throwSQLiteError(db, "committing transaction"); + SQLiteError::throw_(db, "committing transaction"); active = false; } @@ -200,7 +213,7 @@ SQLiteTxn::~SQLiteTxn() { try { if (active && sqlite3_exec(db, "rollback;", 0, 0, 0) != SQLITE_OK) - throwSQLiteError(db, "aborting transaction"); + SQLiteError::throw_(db, "aborting transaction"); } catch (...) { ignoreException(); } diff --git a/src/libstore/sqlite.hh b/src/libstore/sqlite.hh index 99f0d56ce..72ec302e1 100644 --- a/src/libstore/sqlite.hh +++ b/src/libstore/sqlite.hh @@ -96,10 +96,20 @@ struct SQLiteTxn }; -MakeError(SQLiteError, Error); -MakeError(SQLiteBusy, SQLiteError); +struct SQLiteError : Error +{ + const char *path; + int errNo, extendedErrNo; -[[noreturn]] void throwSQLiteError(sqlite3 * db, const FormatOrString & fs); + template + [[noreturn]] static void throw_(sqlite3 * db, const std::string & fs, const Args & ... args); + +protected: + template + SQLiteError(const char *path, int errNo, int extendedErrNo, const Args & ... args); +}; + +MakeError(SQLiteBusy, SQLiteError); void handleSQLiteBusy(const SQLiteBusy & e); diff --git a/src/libutil/util.cc b/src/libutil/util.cc index c075a14b4..d4379a0cf 100644 --- a/src/libutil/util.cc +++ b/src/libutil/util.cc @@ -1062,7 +1062,7 @@ std::string runProgram(Path program, bool searchPath, const Strings & args, auto res = runProgram(RunOptions {.program = program, .searchPath = searchPath, .args = args, .input = input}); if (!statusOk(res.first)) - throw ExecError(res.first, fmt("program '%1%' %2%", program, statusToString(res.first))); + throw ExecError(res.first, "program '%1%' %2%", program, statusToString(res.first)); return res.second; } @@ -1190,7 +1190,7 @@ void runProgram2(const RunOptions & options) if (source) promise.get_future().get(); if (status) - throw ExecError(status, fmt("program '%1%' %2%", options.program, statusToString(status))); + throw ExecError(status, "program '%1%' %2%", options.program, statusToString(status)); } diff --git a/src/nix/repl.cc b/src/nix/repl.cc index 1f9d4fb4e..ec8a57a8e 100644 --- a/src/nix/repl.cc +++ b/src/nix/repl.cc @@ -119,7 +119,7 @@ std::string runNix(Path program, const Strings & args, }); if (!statusOk(res.first)) - throw ExecError(res.first, fmt("program '%1%' %2%", program, statusToString(res.first))); + throw ExecError(res.first, "program '%1%' %2%", program, statusToString(res.first)); return res.second; }