[LevelDB] Plug leveldb logs to bitcoin logs #9999

NicolasDorier commented at 9:45 AM on March 15, 2017: contributor

Implementation ~copy paste~ inspired by https://github.com/bitcoin/bitcoin/blob/57b34599b2deb179ff1bd97ffeab91ec9f904d85/src/leveldb/util/posix_logger.h#L28

ping @laanwj

laanwj commented at 10:07 AM on March 15, 2017: member

Nice! utACK https://github.com/bitcoin/bitcoin/pull/9999/commits/9d21779d9e18546c027d6fe7064289d0e7491efa

in src/dbwrapper.cpp:None in 9d21779d9e outdated

  40 | +
  41 | +        // Print the message
  42 | +        if (p < limit) {
  43 | +            va_list backup_ap;
  44 | +            va_copy(backup_ap, ap);
  45 | +            p += vsnprintf(p, limit - p, format, backup_ap);

laanwj commented at 10:10 AM on March 15, 2017:

nit: I'd prefer not to use vsnprintf here. We've purged this from the code when introducing tinyformat. Wonder if we can do something based on our strprintf. Though bridging between C varargs and C++ is likely going to be difficult, and this solution as-taken-from LevelDB should be fine. It's just fairly ugly code and should not be taken as a hint to contributors that using system v?s?n?printf is okay in other places.

laanwj commented at 10:10 AM on March 15, 2017:

Maybe just add a comment in that regard, mentioning where the code comes from and why and explaining the above.

NicolasDorier force-pushed on Mar 15, 2017

NicolasDorier commented at 10:25 AM on March 15, 2017: contributor

Added some comments about the ugly vsprintf. Bridging varargs to use strprintf is difficult and error prone. I do not think it is worth it.

in src/dbwrapper.h:None in 75fac8689a outdated

  44 | @@ -44,6 +45,11 @@ const std::vector<unsigned char>& GetObfuscateKey(const CDBWrapper &w);
  45 |  
  46 |  };
  47 |  
  48 | +class CBitcoinLogger : public leveldb::Logger {

laanwj commented at 11:00 AM on March 15, 2017:

CBitcoinLevelDBLogger maybe? The name is too general otherwise, we should reserve this class name for ourselves.

NicolasDorier force-pushed on Mar 15, 2017

NicolasDorier commented at 2:24 PM on March 15, 2017: contributor

Renamed. Can Travis be restarted ? I do not think the error is related to this PR.

in src/dbwrapper.h:None in 00bd8662a6 outdated

  44 | @@ -44,6 +45,11 @@ const std::vector<unsigned char>& GetObfuscateKey(const CDBWrapper &w);
  45 |  
  46 |  };
  47 |  
  48 | +class CBitcoinLevelDBLogger : public leveldb::Logger {

sipa commented at 2:26 PM on March 15, 2017:

Why is this class in the .h file? It seems to be only used internally in dbwrapper.cpp.

NicolasDorier commented at 3:22 PM on March 15, 2017:

well, h for declaration and cpp for implementation, at least that is what I always thought. You mean just putting the class directly in the cpp ?

sipa commented at 3:44 PM on March 15, 2017:

.h is for the externally accessible parts of a cpp file. This class doesn't need to be exposed to anything, so it can just go inside the cpp file.

laanwj commented at 6:36 PM on March 15, 2017:

Right, .h is for interface declaration. Although with C++ it's not always clear what should be part of the interface and what not (especially when tests are involved), this one could just as well be private to the .cpp.

[LevelDB] Plug leveldb logs to bitcoin logs cfce581d11

in src/init.cpp:None in 00bd8662a6 outdated

 429 | @@ -430,7 +430,7 @@ std::string HelpMessage(HelpMessageMode mode)
 430 |          strUsage += HelpMessageOpt("-limitdescendantsize=<n>", strprintf("Do not accept transactions if any ancestor would have more than <n> kilobytes of in-mempool descendants (default: %u).", DEFAULT_DESCENDANT_SIZE_LIMIT));
 431 |          strUsage += HelpMessageOpt("-bip9params=deployment:start:end", "Use given start/end times for specified BIP9 deployment (regtest-only)");
 432 |      }
 433 | -    std::string debugCategories = "addrman, alert, bench, cmpctblock, coindb, db, http, libevent, lock, mempool, mempoolrej, net, proxy, prune, rand, reindex, rpc, selectcoins, tor, zmq"; // Don't translate these and qt below
 434 | +    std::string debugCategories = "addrman, alert, bench, cmpctblock, coindb, db, http, libevent, lock, mempool, mempoolrej, net, proxy, prune, rand, reindex, rpc, selectcoins, tor, zmq, leveldb"; // Don't translate these and qt below

jnewbery commented at 5:04 PM on March 15, 2017:

Please retain alphabetical ordering (leveldb should be between http and libevent)

NicolasDorier force-pushed on Mar 16, 2017

NicolasDorier commented at 2:15 AM on March 16, 2017: contributor

fixed nits

laanwj merged this on Mar 18, 2017

laanwj closed this on Mar 18, 2017

laanwj referenced this in commit a328904480 on Mar 18, 2017

codablock referenced this in commit e229f4b658 on Sep 27, 2017

codablock referenced this in commit f812ee4fde on Oct 12, 2017

codablock referenced this in commit 249db27761 on Oct 24, 2017

UdjinM6 referenced this in commit b83da14564 on Nov 8, 2017

MarcoFalke locked this on Sep 8, 2021