rpc: Adding a 'logpath' entry to getrpcinfo

darosior commented at 11:20 am on February 26, 2019: member

as discussed in #15438

fanquake added the label RPC/REST/ZMQ on Feb 26, 2019

fanquake commented at 11:52 am on February 26, 2019: member

edit: Also, looks like you are referencing this PR in the title.

This is failing on both Windows builds:

Win32

0  GEN      qt/moc_bantablemodel.cpp
1rpc/server.cpp: In function ‘UniValue getrpcinfo(const JSONRPCRequest&)’:
2rpc/server.cpp:317:72: error: no matching function for call to ‘UniValue::UniValue(UniValue::VType, const value_type*)’
3     UniValue log_path(UniValue::VSTR, LogInstance().m_file_path.c_str());
4                                                                        ^
5In file included from ./rpc/protocol.h:16:0,
6                 from ./rpc/server.h:10,
7                 from rpc/server.cpp:6:
8./univalue/include/univalue.h:46:5: note: candidate: UniValue::UniValue(const char*)

Win64

0  GEN      qt/moc_bantablemodel.cpp
1  GEN      qt/moc_bitcoinaddressvalidator.cpp
2rpc/server.cpp: In function ‘UniValue getrpcinfo(const JSONRPCRequest&)’:
3rpc/server.cpp:317:72: error: no matching function for call to ‘UniValue::UniValue(UniValue::VType, const value_type*)’
4     UniValue log_path(UniValue::VSTR, LogInstance().m_file_path.c_str());
5                                                                        ^
6In file included from ./rpc/protocol.h:16:0,
7                 from ./rpc/server.h:10,
8                 from rpc/server.cpp:6:
9./univalue/include/univalue.h:46:5: note: candidate: UniValue::UniValue(const char*)

darosior force-pushed on Feb 26, 2019

darosior commented at 12:09 pm on February 26, 2019: member

I changed c_str() to native() in order to return a string_type.

MarcoFalke renamed this:
~~Adding a 'logpath' entry to getrpcinfo, as discussed in #15438~~
rpc: Adding a 'logpath' entry to getrpcinfo
on Feb 26, 2019

Sjors commented at 3:17 pm on February 26, 2019: member

Concept ACK. Can you add a test, e.g. in test/functional/interface_rpc.py? You can get the expected log path with os.path.join(self.nodes[0].datadir, 'debug.log')

Using native() didn’t work. Maybe @ken2812221 knows why the win32 and win64 builds fail?

It’s better to remove fixes [#15438](/bitcoin-bitcoin/15438/) from the commit message. It’s already in the pull request description, which automatically gets added to the merge commit.

darosior force-pushed on Feb 26, 2019

in src/rpc/server.cpp:317 in f3777ba214 outdated

313@@ -314,6 +314,10 @@ static UniValue getrpcinfo(const JSONRPCRequest& request)
314     UniValue result(UniValue::VOBJ);
315     result.pushKV("active_commands", active_commands);
316 
317+    const std::string path = LogInstance().m_file_path.native().c_str();

ken2812221 commented at 7:33 pm on February 26, 2019:

Could use m_file_path.string() instead. It might fix the issue.

darosior commented at 9:18 pm on February 26, 2019:

Thanks, it fixed it.

darosior force-pushed on Feb 26, 2019

darosior commented at 8:55 pm on February 26, 2019: member

@Sjors os.path.join(self.nodes[0].datadir, 'debug.log') does not link to the good file. It outputs /tmp/bitcoin_func_test_tj4444vh/node0/debug.log instead of /tmp/bitcoin_func_test_tj4444vh/node0/regtest/debug.log.

in test/functional/interface_rpc.py:25 in a7fdd54a4a outdated

21@@ -21,6 +22,7 @@ def test_getrpcinfo(self):
22         command = info['active_commands'][0]
23         assert_equal(command['method'], 'getrpcinfo')
24         assert_greater_than_or_equal(command['duration'], 0)
25+        assert(info['logpath'] == os.path.join(self.nodes[0].datadir, 'regtest', 'debug.log'))

promag commented at 9:32 pm on February 26, 2019:

Use assert_equal?

MarcoFalke commented at 10:02 pm on February 26, 2019:

Yes, please. Otherwise a failure can not be debugged

darosior commented at 10:33 am on February 27, 2019:

Fixed

DrahtBot commented at 9:37 pm on February 26, 2019: member

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Conflicts

No conflicts as of last run.

promag commented at 9:39 pm on February 26, 2019: member

IMO this is not RPC info and as such should not be added here.

darosior force-pushed on Feb 27, 2019

Sjors commented at 2:11 pm on February 27, 2019: member

tACK 59c91b9, but let’s wait for discussion in #15438 to settle.

The lack of escape characters (e.g. space) is annoying, but I’m not sure if it’s appropriate to add them.

0bitcoin-cli getrpcinfo | jq -r '.logpath'
1/Users/bitcoin/Library/Application Support/Bitcoin/testnet3/debug.log
2
3tail -f "`bitcoin-cli getrpcinfo | jq -r '.logpath'`"

ryanofsky commented at 3:48 pm on February 27, 2019: member

The lack of escape characters (e.g. space) is annoying, but I’m not sure if it’s appropriate to add them.

What kind of escaping would you want to add here? UniValue already adds JSON escaping, so I would think any extra layers of escaping would just break your tail command and be more annoying.

Sjors commented at 4:07 pm on February 27, 2019: member

@ryanofsky only escaping spaces in the path

promag commented at 4:08 pm on March 3, 2019: member

NACK, I think #15493 is a better solution for OP.

DrahtBot added the label Needs rebase on Apr 8, 2019

laanwj commented at 7:20 pm on May 16, 2019: member

What kind of escaping would you want to add here? UniValue already adds JSON escaping, so I would think any extra layers of escaping would just break your tail command and be more annoying.

Definite don’t add any extra escaping. If we add this field, it needs to be usable from other JSON clients as well which don’t do any other unquoting besides parsing JSON. String sanitization is only relevant for potentially hostile input, from, say, the P2P network.

Add a 'logpath' field to getrpcinfo 8a6810d0d2

darosior force-pushed on May 16, 2019

darosior commented at 9:02 pm on May 16, 2019: member

Rebased, but is it still needed or #15493 will be prefered ?

DrahtBot removed the label Needs rebase on May 16, 2019

laanwj commented at 12:35 pm on July 3, 2019: member

I think getting the log path through RPC can be useful (say, if you have some kind of monitoring program such as bitcoind-ncurses that connects to the RPC and needs to show the logs) and #15493 doesn’t entirely replace that use case.

Tested ACK 8a6810d0d2759c69f63b53c48aa79e0cfdd88ffb

laanwj merged this on Jul 3, 2019

laanwj closed this on Jul 3, 2019

laanwj referenced this in commit 9339008a9d on Jul 3, 2019

darosior deleted the branch on Jul 3, 2019

sidhujag referenced this in commit 0c4a164272 on Jul 5, 2019

laanwj referenced this in commit f5a01cf259 on Jul 8, 2019

sidhujag referenced this in commit 5e1b1762d0 on Jul 9, 2019

monstrobishi referenced this in commit f6d1adc90b on Sep 6, 2020

jasonbcox referenced this in commit 876dd2bb48 on Oct 8, 2020

Munkybooty referenced this in commit 000f27ade6 on Nov 4, 2021

Munkybooty referenced this in commit a0a360917b on Nov 4, 2021

DrahtBot locked this on Dec 16, 2021

rpc: Adding a ’logpath’ entry to getrpcinfo #15483

Conflicts