rpc: Pass const char* to evhttp_connection_get

Perlover commented at 4:28 pm on November 26, 2021: contributor

The second parameter of evhttp_connection_get_peer in libevent already has type as const char ** The compilation of bitcoind with the fresh libevent occurs errors

Details: https://github.com/bitcoin/bitcoin/issues/23606

MarcoFalke added the label DrahtBot Guix build requested on Nov 26, 2021

MarcoFalke added the label Refactoring on Nov 26, 2021

MarcoFalke removed the label DrahtBot Guix build requested on Nov 26, 2021

Perlover commented at 5:59 pm on November 26, 2021: contributor

I tested my compilation with last commit - everything was compiled! So I think this patch is OK If you need I can remake this patch to one single commit (in Monday)

hebasto commented at 6:15 pm on November 26, 2021: member

The second parameter of evhttp_connection_get_peer in libevent already has type as const char ** The compilation of bitcoind with the fresh libevent occurs errors

There were no upstream releases for now since the commit https://github.com/libevent/libevent/commit/a18301a2bb160ff7c3ffaf5b7653c39ffe27b385 has been merged, right?

Perlover commented at 11:48 pm on November 26, 2021: contributor

@hebasto I used the master branch of libevent so this commit definitely merged. I am not sure about new releases of libevent with this commit. The old versions have char ** type for the second parameter. So I patched through preprocessor of C++. I see that all tests are passed. I compiled the bitcoind too with these patches.

Perlover force-pushed on Nov 29, 2021

Perlover commented at 10:09 am on November 29, 2021: contributor

I rebased these patches to the one patch and rebased on current master branch

Perlover force-pushed on Nov 29, 2021

in src/httpserver.cpp:605 in 7e6efdc576 outdated

600@@ -597,7 +601,12 @@ CService HTTPRequest::GetPeer() const
601         // evhttp retains ownership over returned address string
602         const char* address = "";
603         uint16_t port = 0;
604-        evhttp_connection_get_peer(con, (char**)&address, &port);
605+
606+        #if HAVE_EVHTTP_CONNECTION_GET_PEER_CONST_CHAR

laanwj commented at 4:05 pm on November 29, 2021:

Please align #’s to the left.

in src/httpserver.cpp:606 in 7e6efdc576 outdated

600@@ -597,7 +601,12 @@ CService HTTPRequest::GetPeer() const
601         // evhttp retains ownership over returned address string
602         const char* address = "";
603         uint16_t port = 0;
604-        evhttp_connection_get_peer(con, (char**)&address, &port);
605+
606+        #if HAVE_EVHTTP_CONNECTION_GET_PEER_CONST_CHAR
607+        evhttp_connection_get_peer(con, (const char **)&address, &port);

laanwj commented at 4:07 pm on November 29, 2021:

Is any cast still needed in this case? &address is const char**, right?

laanwj commented at 4:09 pm on November 29, 2021: member

Please update the PR title to be clearer. “evhttp_connection_get_peer of libevent” is not a good summary of the change.

Perlover force-pushed on Nov 30, 2021

Perlover renamed this:
~~evhttp_connection_get_peer of libevent~~
Improved evhttp_connection_get_peer function call
on Nov 30, 2021

Perlover commented at 2:44 pm on November 30, 2021: contributor

Please update the PR title to be clearer. “evhttp_connection_get_peer of libevent” is not a good summary of the change.

My English is not native, I tried to give a more precise definition for this PR. I also applied the changes recommended by you and made a rebase relative to the current master branch.

in configure.ac:1515 in 40f4aa8a61 outdated

1509@@ -1510,6 +1510,13 @@ if test x$build_bitcoin_cli$build_bitcoind$bitcoin_enable_qt$use_tests$use_bench
1510   fi
1511 fi
1512 
1513+AC_MSG_CHECKING([for evhttp_connection_get_peer])
1514+AC_COMPILE_IFELSE([AC_LANG_PROGRAM([[#include <event2/http.h>]],
1515+  [[ evhttp_connection_get_peer((evhttp_connection*) nullptr,(const char**) nullptr,(uint16_t *) nullptr); ]])],

luke-jr commented at 10:59 pm on November 30, 2021:

Please specify valid arguments in case libevent eventually adds nonnull attributes or something

Perlover commented at 11:14 am on December 1, 2021:

If I understand correctly, the macro AC_COMPILE_IF ELSE only checks the possibility of compiling, not running the program. Therefore, I do not understand why to change the parameters as nullptr to any fictitious ones (as in your final example). By and large, I created the code using examples from the same configuration file as this one, for example:

0...
1AC_COMPILE_IFELSE([AC_LANG_PROGRAM([[#include <ctime>]],
2  [[ gmtime_r((const time_t *) nullptr, (struct tm *) nullptr); ]])],
3...

luke-jr commented at 7:52 pm on December 1, 2021:

If libevent some day marks the arguments as nonnull, the compiler may error if nullptr is passed

in configure.ac:1513 in 40f4aa8a61 outdated

1509@@ -1510,6 +1510,13 @@ if test x$build_bitcoin_cli$build_bitcoind$bitcoin_enable_qt$use_tests$use_bench
1510   fi
1511 fi
1512 
1513+AC_MSG_CHECKING([for evhttp_connection_get_peer])

luke-jr commented at 11:00 pm on November 30, 2021:

0AC_MSG_CHECKING([if evhttp_connection_get_peer expects const char *])

luke-jr changes_requested

luke-jr commented at 0:26 am on December 1, 2021: member

Also, please prepend your commit message with a shortish summary (~80 chars max, not a hard limit) and a blank line :)

luke-jr commented at 1:22 am on December 1, 2021: member

Further issues:

In this scope, libevent may not even be installed. (Put the check inside the above if block that checks for libevent)
libevent may require certain CFLAGS to be used. (CXXFLAGS="$CXXFLAGS $EVENT_CFLAGS"; don’t forget TEMP_CXXFLAGS stuff)
uint16_t needs <cstdint> included

Suggested fix for these and prior issues: https://gist.github.com/luke-jr/f8df9d7173a3f673b82b4f0c26854807

Perlover commented at 11:25 am on December 1, 2021: contributor

Further issues:

1. In this scope, libevent may not even be installed. (Put the check inside the above `if` block that checks for libevent)

There is such a code just above my code. Doesn’t it guarantee that configure will stop before my code is executed? It seems to me that it is redundant to check the installation of libevent, if it is already checked above.

0if test x$build_bitcoin_cli$build_bitcoind$bitcoin_enable_qt$use_tests$use_bench != xnonononono; then
1  PKG_CHECK_MODULES([EVENT], [libevent >= 2.0.21], [use_libevent=yes], [AC_MSG_ERROR([libevent version 2.0.21 or greater not found.])])
2  if test x$TARGET_OS != xwindows; then
3    PKG_CHECK_MODULES([EVENT_PTHREADS], [libevent_pthreads >= 2.0.21],, [AC_MSG_ERROR([libevent_pthreads version 2.0.21 or greater not found.])])
4  fi
5
6  if test x$suppress_external_warnings != xno; then
7    EVENT_CFLAGS=SUPPRESS_WARNINGS($EVENT_CFLAGS)
8  fi
9fi

2. libevent may require certain CFLAGS to be used. (`CXXFLAGS="$CXXFLAGS $EVENT_CFLAGS"`; don't forget `TEMP_CXXFLAGS` stuff)

Is it really that important to use this complex construction with CXXFLAGS here? All 15 checks performed on github pass.

3. `uint16_t` needs `<cstdint>` included

It’s probably better for me to just replace the arguments with those used in libevent: uint16_t -> ev_uint16_t

Suggested fix for these and prior issues: https://gist.github.com/luke-jr/f8df9d7173a3f673b82b4f0c26854807

Perlover commented at 11:48 am on December 1, 2021: contributor

Suggested fix for these and prior issues: https://gist.github.com/luke-jr/f8df9d7173a3f673b82b4f0c26854807

0[ AC_MSG_RESULT([no])]

If I understand your code correctly, then there will be no constants defined here and I will have to add additional #ifdef or #if defined(...) checks to my code.

The evhttp_connection_get_peer function from libevent changes the type of the second parameter. Fixing the problem. 091ccc38c2

Perlover force-pushed on Dec 1, 2021

Perlover commented at 12:15 pm on December 1, 2021: contributor

@luke-jr, I applied several of your suggestions - description lines in configure.ac, short name commit, added include <cstdio>. I also corrected the style of pointers in accordance with the widely used style inside the bitcoin code. At the same time, I have not yet checked for installing libevent (it seems to me that the check is already being performed above and it is superfluous), and I also did not add the CXXFLAGS options - to be honest, it seems to me superfluous for a small test function that checks the type of parameters. I am not in favor of complicating things that can not be complicated. In addition, the code passed all automatic checks. I did not replace the parameters with nullptr with others, since the macro only checks the possibility of compilation. I don’t see the point in that. I used the nullptr in a similar way, since this usage is already widely used in configure.ac. And I did rebase on current master branch of bitcoin code.

luke-jr commented at 1:33 pm on December 1, 2021: member

Right now, your PR will still break on systems:

without libevent
where libevent is installed outside the compiler’s default include path
when/if the compiler is some day strict about disallowing nullptr where it is invalid

(Also note you should ideally not rebase unless there are conflicts)

luke-jr commented at 7:55 pm on December 1, 2021: member

There is such a code just above my code. Doesn’t it guarantee that configure will stop before my code is executed?

The check above only runs with that if test x$build_bitcoin_cli$build_bitcoind$bitcoin_enable_qt$use_tests$use_bench != xnonononono; then conditional. Your code currently runs even if this libevent check is skipped. I am suggesting moving it up inside the if block so it only runs when libevent is used.

Is it really that important to use this complex construction with CXXFLAGS here? All 15 checks performed on github pass.

If libevent is installed outside the default compiler include path, EVENT_CFLAGS will contain necessary -I... parameters for the compiler.

luke-jr commented at 7:57 pm on December 1, 2021: member

If I understand your code correctly, then there will be no constants defined here and I will have to add additional #ifdef or #if defined(…) checks to my code.

This is how other similar checks are currently done.

DrahtBot commented at 3:44 am on December 5, 2021: member

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

Conflicts

Reviewers, this pull request conflicts with the following ones:

#23810 (refactor: destroy all C-style casts; use modern C++ casts, enforce via -Wold-style-cast by PastaPastaPasta)

If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.

Necessary improvements to make configure work without libevent installed c62d763fc3

Perlover commented at 4:03 pm on December 7, 2021: contributor

@luke-jr , I did your suggestions in next commit. I hope that they will pass all the tests. Thanks!

luke-jr approved

luke-jr commented at 8:39 pm on December 9, 2021: member

tACK c62d763fc313585d79ad833c9d729f6acf2652aa

MarcoFalke added the label DrahtBot Guix build requested on Dec 10, 2021

DrahtBot commented at 3:21 am on December 12, 2021: member

Guix builds

File	commit 65b49f60a4cf521889297b2006f66efa11d769c5(master)	commit 383c6030c38abe8396c1538b716d2df0ec22c021(master and this pull)
SHA256SUMS.part	`f2102a714602d9bd...`	`23a6facfefe85c33...`
*-aarch64-linux-gnu-debug.tar.gz	`67c3b97d6163b225...`	`2a29c2e02b5b8420...`
*-aarch64-linux-gnu.tar.gz	`e552d9da566925f4...`	`2706283034f7a941...`
*-arm-linux-gnueabihf-debug.tar.gz	`e52d3c29b7546837...`	`c77d96f9b09f276d...`
*-arm-linux-gnueabihf.tar.gz	`5201b83feddbf102...`	`25fcbb996f51f63c...`
*-osx-unsigned.dmg	`67b8b536e48321ee...`	`62c492d46577140a...`
*-osx-unsigned.tar.gz	`9f31ce521d457521...`	`cda910ebd389d024...`
*-osx64.tar.gz	`bd224f3b2eeeee98...`	`790d930136583c76...`
*-powerpc64-linux-gnu-debug.tar.gz	`0e5dbb28a25f0484...`	`c98e54d4c4b65ad6...`
*-powerpc64-linux-gnu.tar.gz	`9842f1ffc99e9ae2...`	`5d88e3fe9ff10be1...`
*-powerpc64le-linux-gnu-debug.tar.gz	`a6c0ab66439628ba...`	`6c602bb8d1e42f44...`
*-powerpc64le-linux-gnu.tar.gz	`4d5737306cc7f99c...`	`0fd4542ef01a8bea...`
*-riscv64-linux-gnu-debug.tar.gz	`13779da8f21ad301...`	`10c055e05b34d4ef...`
*-riscv64-linux-gnu.tar.gz	`1cf094bc9c76f65d...`	`5b0382a9b5bb175f...`
*-win-unsigned.tar.gz	`bebdbc3ae69316e4...`	`497c983709fba9e1...`
*-win64-debug.zip	`608930112868dfe0...`	`6f86e28486e7fa15...`
*-win64-setup-unsigned.exe	`4af8bda27825b20c...`	`fd6486a51f74444f...`
*-win64.zip	`da75132626898ec9...`	`abaf2105b856c8b8...`
*-x86_64-linux-gnu-debug.tar.gz	`a96d1470d69d0e21...`	`82480e4c58ed57c1...`
*-x86_64-linux-gnu.tar.gz	`a18e334698275ccd...`	`20ff9ee8005b0454...`
*.tar.gz	`9ff29826c029c5c7...`	`8d761fdbbfed030b...`
guix_build.log	`4900477e70c6cf02...`	`83db0dc9c7470304...`
guix_build.log.diff		`eedb0745898e3007...`

DrahtBot removed the label DrahtBot Guix build requested on Dec 12, 2021

luke-jr referenced this in commit d3e534f4a5 on Dec 14, 2021

luke-jr referenced this in commit ff831bbd9a on Dec 14, 2021

laanwj commented at 3:36 pm on January 13, 2022: member

Code review ACK c62d763fc313585d79ad833c9d729f6acf2652aa

laanwj renamed this:
~~Improved evhttp_connection_get_peer function call~~
net: Pass const char* to evhttp_connection_get_peer for new libevent
on Jan 13, 2022

laanwj renamed this:
~~net: Pass const char* to evhttp_connection_get_peer for new libevent~~
rpc: Pass const char* to evhttp_connection_get_peer for new libevent
on Jan 13, 2022

laanwj added the label RPC/REST/ZMQ on Jan 13, 2022

laanwj removed the label Refactoring on Jan 13, 2022

laanwj merged this on Jan 13, 2022

laanwj closed this on Jan 13, 2022

sidhujag referenced this in commit b8dfbdb946 on Jan 14, 2022

DrahtBot locked this on Jan 13, 2023

rpc: Pass const char* to evhttp_connection_get_peer for new libevent #23607

Conflicts

Guix builds