Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

tests: Run IPC with use-filesystem-sockets active #455

Merged
merged 7 commits into from
Mar 21, 2022

Conversation

chrissie-c
Copy link
Contributor

@chrissie-c chrissie-c commented Jan 12, 2022

(re-opening PR 380 that got lost in the Great Git Transition of 2021)

Provide an LD_PRELOAD library that simulates the presence
of /etc/libqb/use-filesystem-sockets so that we can test
that functionality without actually having the file on
the system and affecting everything else running on the
box.

@chrissie-c chrissie-c force-pushed the test-filesystem-sockets branch from 82996cd to bca3a15 Compare January 13, 2022 08:09
@chrissie-c chrissie-c force-pushed the test-filesystem-sockets branch from 4d32ad0 to 32cfc77 Compare January 24, 2022 13:05
@chrissie-c
Copy link
Contributor Author

retest this please

1 similar comment
@chrissie-c
Copy link
Contributor Author

retest this please

static int (*real_xstat)(int __ver, const char *__filename, void *__stat_buf);

if (!opened) {
dlhandle = dlopen(LIBC_SO, RTLD_NOW);

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Had been doing that in the past with dlopen_fatal(RTLD_NEXT, ...) at least in the assumption it would then get me the symbol coming next in the search order instead of directly jumping to libc.
And instead of keeping the handle in a static variable (when using an explicit library - with RTLD_NEXT there shouldn't be a need) I closed it after getting the symbols I needed.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sry for the above - that is of course dlsym_fatal.
And that again is just my wrapper for dlsym exiting out if it doesn't find the symbol.
The comment in parentheses already points to something being fishy here ;-)

opened = 1;
}

if (strcmp(__filename, FORCESOCKETSFILE) == 0) {
Copy link

@wenningerk wenningerk Mar 17, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

__xstat seems to return -1 with errno==EFAULT in case the filename points to invalid memory instead of segfaulting.
Don't know if we need it that generic here but we might get called by unexpected callers and get confusing results.
Simple approach might be doing the original in any case and just compare and possibly fake result if return -1 and errno!=EFAULT.

@chrissie-c chrissie-c force-pushed the test-filesystem-sockets branch from 13348e8 to 27094cc Compare March 17, 2022 11:31
@chrissie-c
Copy link
Contributor Author

That last force push was just a rebase to fix a covscan error in CI

@knet-ci-bot
Copy link

Build finished.

# so we can test both options without breaking other things
# that might be running on this system
#
if [ "`uname -s`" = "Linux" ]
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

General comment here, also spotted below in other test scripts. It´s best to avoid the command.. format and use instead "$(command..)". When using `` the command is forked in another shell that resets the envvar to default. This could cause unexpected behavior. Similar for pwd etc. forking also uses more resources. With $(cmd) the command is executed within the same shell.

Provide an LD_PRELOAD library that simulates the presence
of /etc/libqb/use-filesystem-sockets so that we can test
that functionality without actually having the file on
the system and affecting everything else running on the
box.
F35 and Centos9 seem to have reverted back to an actual stat()
call rather than a wrapper around __xstat(), so trap both just
in case.

Other small fixes
@chrissie-c chrissie-c force-pushed the test-filesystem-sockets branch from b77f1ae to bcb1103 Compare March 18, 2022 10:34
@chrissie-c chrissie-c force-pushed the test-filesystem-sockets branch from bcb1103 to 8812662 Compare March 18, 2022 10:53
then
if [ -f $(pwd)/.libs/libstat_wrapper.so ]
then
export LD_PRELOAD=$(pwd)/.libs/libstat_wrapper.so
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

best to have:
export LD_PRELOAD="$(pwd)/.libs/libstat_wrapper.so" in case pwd contains spaces... it doesn´t happen in CI or any sane systems.. but it can happen :)

Copy link
Contributor

@kgaillot kgaillot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me too

configure.ac Outdated
@@ -176,7 +176,7 @@ PKG_CHECK_MODULES([libxml], [libxml-2.0])
# don´t build the man pages
if test "x$cross_compiling" = "xno"; then
AM_CONDITIONAL([BUILD_MAN], [true])
DOXYGEN2MAN="\$(abs_builddir)/../doxygen2man/doxygen2man"
DOXYGEN2MAN="\"\$(abs_builddir)/../doxygen2man/doxygen2man\""
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For future-proofing it would be better to leave this alone and quote $(DOXYGEN2MAN) in docs/Makefile.am, otherwise if someone ever uses "$DOXYGEN2MAN" it will fail in a confusing way

@fabbione
Copy link
Member

retest this please

@chrissie-c chrissie-c merged commit 78df90b into ClusterLabs:main Mar 21, 2022
@chrissie-c chrissie-c deleted the test-filesystem-sockets branch March 21, 2022 09:10
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants