Skip to content

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

Merged
chrissie-c merged 7 commits intoClusterLabs:mainfrom
chrissie-c:test-filesystem-sockets
Mar 21, 2022
Merged

tests: Run IPC with use-filesystem-sockets active#455
chrissie-c merged 7 commits intoClusterLabs:mainfrom
chrissie-c:test-filesystem-sockets

Conversation

@chrissie-c
Copy link
Copy Markdown
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
Copy Markdown
Contributor Author

retest this please

1 similar comment
@chrissie-c
Copy link
Copy Markdown
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);
Copy link
Copy Markdown

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.

Copy link
Copy Markdown

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
Copy Markdown

@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
Copy Markdown
Contributor Author

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

@knet-ci-bot
Copy link
Copy Markdown

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
Copy Markdown
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
Copy Markdown
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
Copy Markdown

@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
if test "x$cross_compiling" = "xno"; then
AM_CONDITIONAL([BUILD_MAN], [true])
DOXYGEN2MAN="\$(abs_builddir)/../doxygen2man/doxygen2man"
DOXYGEN2MAN="\"\$(abs_builddir)/../doxygen2man/doxygen2man\""
Copy link
Copy Markdown

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
Copy Markdown
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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants