Skip to content

common: disable journald logging backend if struct msghdr is not found#40607

Merged
tchaikov merged 1 commit intoceph:masterfrom
tchaikov:wip-journald-if-msghdr
Apr 7, 2021
Merged

common: disable journald logging backend if struct msghdr is not found#40607
tchaikov merged 1 commit intoceph:masterfrom
tchaikov:wip-journald-if-msghdr

Conversation

@tchaikov
Copy link
Contributor

@tchaikov tchaikov commented Apr 6, 2021

  • cmake/modules/CephChecks.cmake: detect the existence of struct msghdr,
    define HAVE_MSGHDR if it is found
  • src/common/CMakeLists.txt: do not compile journald.cc if HAVE_MSGHDR
    is FALSE. as in that case, we cannot use sendmsg() to write to
    journald unix domain socket
  • src/common/Journald.h: define a dummy JournaldLogger and a dummy
    JournaldClusterLogger when HAVE_MSGHDR is not defined, in order to
    minimize the change in Log.h and Log.cc, otherwise the source code of
    Log.h and Log.cc would be segmented into smaller chunks by
    ifdef HAVE_MSGHDR macros.
  • src/include/config-h.in.cmake: define a new macro named
    HAVE_MSGHDR.

Signed-off-by: Kefu Chai [email protected]

Checklist

  • References tracker ticket
  • Updates documentation if necessary
  • Includes tests for new functionality or reproducer for bug
Show available Jenkins commands
  • jenkins retest this please
  • jenkins test classic perf
  • jenkins test crimson perf
  • jenkins test signed
  • jenkins test make check
  • jenkins test make check arm64
  • jenkins test submodules
  • jenkins test dashboard
  • jenkins test api
  • jenkins test docs
  • jenkins render docs
  • jenkins test ceph-volume all
  • jenkins test ceph-volume tox

@tchaikov tchaikov mentioned this pull request Apr 6, 2021
Merged
3 tasks
@tchaikov tchaikov force-pushed the wip-journald-if-msghdr branch from 2053592 to f7a354e Compare April 6, 2021 05:48
@github-actions github-actions bot added the tests label Apr 6, 2021
@tchaikov
Copy link
Contributor Author

tchaikov commented Apr 6, 2021

@petrutlucian94 sorry for breaking the win32 build. this change should fix it.

@petrutlucian94
Copy link
Contributor

petrutlucian94 commented Apr 6, 2021

@petrutlucian94 sorry for breaking the win32 build. this change should fix it.

thanks for solving the issue!

@tchaikov
common: disable journald logging backend if struct msghdr is not found
80c4a1c 
* cmake/modules/CephChecks.cmake: detect the existence of struct msghdr,
  define HAVE_MSGHDR if it is found
* src/common/CMakeLists.txt: do not compile journald.cc if HAVE_MSGHDR
  is FALSE. as in that case, we cannot use sendmsg() to write to
  journald unix domain socket
* src/test/CMakeLists.txt, src/test/common/CMakeLists.txt: disable test
  exercising journald logging backend if HAVE_MSGHDR is not defined
* src/common/Journald.h: define a dummy JournaldLogger and a dummy
  JournaldClusterLogger when HAVE_MSGHDR is not defined, in order to
  minimize the change in Log.h and Log.cc, otherwise the source code of
  Log.h and Log.cc would be segmented into smaller chunks by
  `ifdef HAVE_MSGHDR` macros.
* src/include/config-h.in.cmake: define a new macro named
  HAVE_MSGHDR.

Signed-off-by: Kefu Chai <[email protected]>
@tchaikov tchaikov force-pushed the wip-journald-if-msghdr branch from f7a354e to 80c4a1c Compare April 6, 2021 06:47
@tchaikov
Copy link
Contributor Author

tchaikov commented Apr 6, 2021

https://shaman.ceph.com/builds/ceph/wip-journald-if-msghdr/80c4a1cd37298c2b63de01de9615025ce9f734ce/

@tchaikov
Copy link
Contributor Author

tchaikov commented Apr 7, 2021

https://pulpito.ceph.com/kchai-2021-04-06_14:10:15-rados-wip-kefu-testing-2021-04-06-1610-distro-basic-gibba/

failures tracked by

@tchaikov tchaikov merged commit 2b8551a into ceph:master Apr 7, 2021
@tchaikov tchaikov deleted the wip-journald-if-msghdr branch April 7, 2021 07:06
@wjwithagen
Copy link
Contributor

wjwithagen commented Apr 7, 2021

@tchaikov
Would it be possible to use a different test or struct for this?
FreeBSD does have struct msghdr but no journald and compilation fails on all sorts of undefined constants.
So either we make the tests: HAVE_MSGHDR AND NOT FREEBSD
Or we find something different smart??

@tchaikov
Copy link
Contributor Author

tchaikov commented Apr 8, 2021

@tchaikov
Would it be possible to use a different test or struct for this?
FreeBSD does have struct msghdr but no journald and compilation fails on all sorts of undefined constants.

could you be more specific on "all sorts of undefined constants" ?

So either we make the tests: HAVE_MSGHDR AND NOT FREEBSD
Or we find something different smart??

i'd suggest check on some specific libraries/header/struct instead of distro or kernel, unless some certain feature really depends on distro/kernel.

@wjwithagen
Copy link
Contributor

wjwithagen commented Apr 8, 2021

@tchaikov
Would it be possible to use a different test or struct for this?
FreeBSD does have struct msghdr but no journald and compilation fails on all sorts of undefined constants.

could you be more specific on "all sorts of undefined constants" ?

/home/jenkins/workspace/ceph-master-compile/src/common/Journald.cc:169:45: error: use of undeclared identifier 'MFD_ALLOW_SEALING'
  int memfd = memfd_create("ceph-journald", MFD_ALLOW_SEALING | MFD_CLOEXEC);
                                            ^
/home/jenkins/workspace/ceph-master-compile/src/common/Journald.cc:169:65: error: use of undeclared identifier 'MFD_CLOEXEC'
  int memfd = memfd_create("ceph-journald", MFD_ALLOW_SEALING | MFD_CLOEXEC);
                                                                ^
/home/jenkins/workspace/ceph-master-compile/src/common/Journald.cc:175:30: error: use of undeclared identifier 'O_TMPFILE'
  memfd = open(mem_file_dir, O_TMPFILE | O_EXCL | O_CLOEXEC, S_IRUSR | S_IWUSR);
                             ^
/home/jenkins/workspace/ceph-master-compile/src/common/Journald.cc:188:42: error: use of undeclared identifier 'MFD_ALLOW_SEALING'
    return memfd_create("ceph-journald", MFD_ALLOW_SEALING | MFD_CLOEXEC);
                                         ^
/home/jenkins/workspace/ceph-master-compile/src/common/Journald.cc:188:62: error: use of undeclared identifier 'MFD_CLOEXEC'
    return memfd_create("ceph-journald", MFD_ALLOW_SEALING | MFD_CLOEXEC);
                                                             ^
/home/jenkins/workspace/ceph-master-compile/src/common/Journald.cc:190:31: error: use of undeclared identifier 'O_TMPFILE'
    return open(mem_file_dir, O_TMPFILE | O_EXCL | O_CLOEXEC, S_IRUSR | S_IWUSR);
                              ^
/home/jenkins/workspace/ceph-master-compile/src/common/Journald.cc:250:28: error: use of undeclared identifier 'F_ADD_SEALS'
    ret = fcntl(buffer_fd, F_ADD_SEALS, F_SEAL_SHRINK | F_SEAL_GROW | F_SEAL_WRITE | F_SEAL_SEAL);
                           ^
/home/jenkins/workspace/ceph-master-compile/src/common/Journald.cc:250:41: error: use of undeclared identifier 'F_SEAL_SHRINK'
    ret = fcntl(buffer_fd, F_ADD_SEALS, F_SEAL_SHRINK | F_SEAL_GROW | F_SEAL_WRITE | F_SEAL_SEAL);
                                        ^
/home/jenkins/workspace/ceph-master-compile/src/common/Journald.cc:250:57: error: use of undeclared identifier 'F_SEAL_GROW'
    ret = fcntl(buffer_fd, F_ADD_SEALS, F_SEAL_SHRINK | F_SEAL_GROW | F_SEAL_WRITE | F_SEAL_SEAL);
                                                        ^
/home/jenkins/workspace/ceph-master-compile/src/common/Journald.cc:250:71: error: use of undeclared identifier 'F_SEAL_WRITE'
    ret = fcntl(buffer_fd, F_ADD_SEALS, F_SEAL_SHRINK | F_SEAL_GROW | F_SEAL_WRITE | F_SEAL_SEAL);
                                                                      ^
/home/jenkins/workspace/ceph-master-compile/src/common/Journald.cc:250:86: error: use of undeclared identifier 'F_SEAL_SEAL'
    ret = fcntl(buffer_fd, F_ADD_SEALS, F_SEAL_SHRINK | F_SEAL_GROW | F_SEAL_WRITE | F_SEAL_SEAL);

So either we make the tests: HAVE_MSGHDR AND NOT FREEBSD
Or we find something different smart??

i'd suggest check on some specific libraries/header/struct instead of distro or kernel, unless some certain feature really depends on distro/kernel.

So perhaps a test on the availability MFD_CLOEXEC would be workable since that indicates the lack of memfd??
A beter test would be a real indication of journald/systemd, but for me that is Linux specific so that would be an alternative.

@tchaikov
Copy link
Contributor Author

tchaikov commented Apr 8, 2021

i see, i will replace the HAVE_MSGHDR check with WITH_SYSTEMD

@wjwithagen
Copy link
Contributor

wjwithagen commented Apr 8, 2021

@tchaikov
That would definitly work.

@tchaikov
Copy link
Contributor Author

tchaikov commented Apr 8, 2021

@wjwithagen #40658

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@liewegas liewegas liewegas approved these changes

@ivancich ivancich ivancich approved these changes

@neha-ojha neha-ojha Awaiting requested review from neha-ojha

@jdurgin jdurgin Awaiting requested review from jdurgin

Assignees

No one assigned

Labels

build/ops common needs-qa tests wip-kefu-testing

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@tchaikov @petrutlucian94 @wjwithagen @liewegas @ivancich