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

Perlover · 2021-11-26T16:28:22Z

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: #23606

Perlover · 2021-11-26T17:59:59Z

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 · 2021-11-26T18:15:44Z

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 libevent/libevent@a18301a has been merged, right?

Perlover · 2021-11-26T23:48:47Z

@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 · 2021-11-29T10:09:57Z

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

laanwj · 2021-11-29T16:05:49Z

src/httpserver.cpp

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


Please align #'s to the left.

laanwj · 2021-11-29T16:07:10Z

src/httpserver.cpp

-        evhttp_connection_get_peer(con, (char**)&address, &port);
+
+        #if HAVE_EVHTTP_CONNECTION_GET_PEER_CONST_CHAR
+        evhttp_connection_get_peer(con, (const char **)&address, &port);


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

laanwj · 2021-11-29T16:09:29Z

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

Perlover · 2021-11-30T14:44:20Z

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.

luke-jr · 2021-11-30T22:59:47Z

configure.ac

@@ -1510,6 +1510,13 @@ if test x$build_bitcoin_cli$build_bitcoind$bitcoin_enable_qt$use_tests$use_bench
  fi
 fi

+AC_MSG_CHECKING([for evhttp_connection_get_peer])
+AC_COMPILE_IFELSE([AC_LANG_PROGRAM([[#include <event2/http.h>]],
+  [[ evhttp_connection_get_peer((evhttp_connection*) nullptr,(const char**) nullptr,(uint16_t *) nullptr); ]])],


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

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:

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

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

luke-jr · 2021-11-30T23:00:19Z

configure.ac

@@ -1510,6 +1510,13 @@ if test x$build_bitcoin_cli$build_bitcoind$bitcoin_enable_qt$use_tests$use_bench
  fi
 fi

+AC_MSG_CHECKING([for evhttp_connection_get_peer])


Suggested change

AC_MSG_CHECKING([for evhttp_connection_get_peer])

AC_MSG_CHECKING([if evhttp_connection_get_peer expects const char *])

luke-jr · 2021-12-01T00:26:48Z

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

luke-jr · 2021-12-01T01:22:23Z

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 · 2021-12-01T11:25:49Z

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.

if test x$build_bitcoin_cli$build_bitcoind$bitcoin_enable_qt$use_tests$use_bench != xnonononono; then
  PKG_CHECK_MODULES([EVENT], [libevent >= 2.0.21], [use_libevent=yes], [AC_MSG_ERROR([libevent version 2.0.21 or greater not found.])])
  if test x$TARGET_OS != xwindows; then
    PKG_CHECK_MODULES([EVENT_PTHREADS], [libevent_pthreads >= 2.0.21],, [AC_MSG_ERROR([libevent_pthreads version 2.0.21 or greater not found.])])
  fi

  if test x$suppress_external_warnings != xno; then
    EVENT_CFLAGS=SUPPRESS_WARNINGS($EVENT_CFLAGS)
  fi
fi

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 · 2021-12-01T11:48:56Z

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

[ 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.

…e of the second parameter. Fixing the problem.

Perlover · 2021-12-01T12:15:48Z

@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 · 2021-12-01T13:33:52Z

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 · 2021-12-01T19:55:58Z

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 · 2021-12-01T19:57:37Z

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 · 2021-12-05T03:44:40Z

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.

Perlover · 2021-12-07T16:03:29Z

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

luke-jr

tACK c62d763

DrahtBot · 2021-12-12T03:21:02Z

Guix builds

File	commit `65b49f6` (master)	commit `383c603` (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...`

…e of the second parameter. Fixing the problem. Github-Pull: bitcoin#23607 Rebased-From: 091ccc3

Github-Pull: bitcoin#23607 Rebased-From: c62d763

laanwj · 2022-01-13T15:36:00Z

Code review ACK c62d763

…eer for new libevent c62d763 Necessary improvements to make configure work without libevent installed (Perlover) 091ccc3 The evhttp_connection_get_peer function from libevent changes the type of the second parameter. Fixing the problem. (Perlover) Pull request description: 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: bitcoin#23606 ACKs for top commit: laanwj: Code review ACK c62d763 luke-jr: tACK c62d763 Tree-SHA512: d1c8062d90bd0d55c582dae2c3a7e5ee1b6c7ca872bf4aa7fe6f45a52ac4a8f59464215759d961f8efde0efbeeade31b08daf9387d7d50d7622baa1c06992d83

maflcko added DrahtBot Guix build requested Refactoring and removed DrahtBot Guix build requested labels Nov 26, 2021

Perlover force-pushed the patch-1 branch from 76db38f to b87ff7b Compare November 29, 2021 10:08

Perlover force-pushed the patch-1 branch 2 times, most recently from 09c85b9 to 7e6efdc Compare November 29, 2021 10:26

laanwj reviewed Nov 29, 2021

View reviewed changes

Perlover force-pushed the patch-1 branch from 7e6efdc to 40f4aa8 Compare November 30, 2021 14:39

Perlover changed the title ~~evhttp_connection_get_peer of libevent~~ Improved evhttp_connection_get_peer function call Nov 30, 2021

luke-jr suggested changes Nov 30, 2021

View reviewed changes

The evhttp_connection_get_peer function from libevent changes the typ…

091ccc3

…e of the second parameter. Fixing the problem.

Perlover force-pushed the patch-1 branch from 40f4aa8 to 091ccc3 Compare December 1, 2021 12:07

DrahtBot mentioned this pull request Dec 5, 2021

build: Post-pr23489 small cleanups #23675

Merged

Necessary improvements to make configure work without libevent installed

c62d763

luke-jr approved these changes Dec 9, 2021

View reviewed changes

maflcko added the DrahtBot Guix build requested label Dec 10, 2021

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

luke-jr pushed a commit to bitcoinknots/bitcoin that referenced this pull request Dec 14, 2021

The evhttp_connection_get_peer function from libevent changes the typ…

d3e534f

…e of the second parameter. Fixing the problem. Github-Pull: bitcoin#23607 Rebased-From: 091ccc3

luke-jr pushed a commit to bitcoinknots/bitcoin that referenced this pull request Dec 14, 2021

Necessary improvements to make configure work without libevent installed

ff831bb

Github-Pull: bitcoin#23607 Rebased-From: c62d763

DrahtBot mentioned this pull request Dec 18, 2021

docs: avoid C-style casts; use modern C++ casts #23810

Merged

laanwj changed the title ~~Improved evhttp_connection_get_peer function call~~ net: Pass const char* to evhttp_connection_get_peer for new libevent Jan 13, 2022

laanwj changed the title ~~net: Pass const char* to evhttp_connection_get_peer for new libevent~~ rpc: Pass const char* to evhttp_connection_get_peer for new libevent Jan 13, 2022

laanwj added RPC/REST/ZMQ and removed Refactoring labels Jan 13, 2022

laanwj merged commit 767ee2e into bitcoin:master Jan 13, 2022

bitcoin locked and limited conversation to collaborators Jan 13, 2023

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

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

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

Uh oh!

Conversation

Perlover commented Nov 26, 2021 • edited by laanwj Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Perlover commented Nov 26, 2021

Uh oh!

hebasto commented Nov 26, 2021

Uh oh!

Perlover commented Nov 26, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Perlover commented Nov 29, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

laanwj Nov 29, 2021

Choose a reason for hiding this comment

Uh oh!

laanwj Nov 29, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

laanwj commented Nov 29, 2021

Uh oh!

Perlover commented Nov 30, 2021

Uh oh!

luke-jr Nov 30, 2021

Choose a reason for hiding this comment

Uh oh!

Perlover Dec 1, 2021

Choose a reason for hiding this comment

Uh oh!

luke-jr Dec 1, 2021

Choose a reason for hiding this comment

Uh oh!

luke-jr Nov 30, 2021

Choose a reason for hiding this comment

Uh oh!

luke-jr commented Dec 1, 2021

Uh oh!

luke-jr commented Dec 1, 2021

Uh oh!

Perlover commented Dec 1, 2021

Uh oh!

Perlover commented Dec 1, 2021

Uh oh!

Perlover commented Dec 1, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

luke-jr commented Dec 1, 2021

Uh oh!

luke-jr commented Dec 1, 2021

Uh oh!

luke-jr commented Dec 1, 2021

Uh oh!

DrahtBot commented Dec 5, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Conflicts

Uh oh!

Perlover commented Dec 7, 2021

Uh oh!

luke-jr left a comment

Choose a reason for hiding this comment

Uh oh!

DrahtBot commented Dec 12, 2021

Guix builds

Uh oh!

laanwj commented Jan 13, 2022

Uh oh!

Uh oh!

Perlover commented Nov 26, 2021 •

edited by laanwj

Loading

Perlover commented Nov 26, 2021 •

edited

Loading

Perlover commented Nov 29, 2021 •

edited

Loading

laanwj Nov 29, 2021 •

edited

Loading

Perlover commented Dec 1, 2021 •

edited

Loading

DrahtBot commented Dec 5, 2021 •

edited

Loading