build: remove warnings during build by pstorz · Pull Request #948 · bareos/bareos

pstorz · 2021-10-01T16:07:38Z

Thank you for contributing to the Bareos Project!

Please check

Short description and the purpose of this PR is present above this paragraph
Your name is present in the AUTHORS file (optional)

If you have any questions or problems, please give a comment in the PR.

Helpful documentation and best practices

Checklist for the reviewer of the PR (will be processed by the Bareos team)

merge the following PR also when merging: https://git.bareos.com/bareos/CD/pulls/37

General

PR name is meaningful
Purpose of the PR is understood
Commit descriptions are understandable and well formatted

Source code quality

Source code changes are understandable
Variable and function names are meaningful
Code comments are correct (logically and spelling)
bareos-check-sources --since-merge does not report any problems
git status should not report modifications in the source tree after building and testing

arogge

still needs some work, some of the read() and write() calls are still not checked thoroughly.
But good progress anyway!

core/src/console/console.cc

arogge · 2021-10-05T08:57:54Z

core/src/console/console.cc

+  while (items && list_index < items_list_size) {
    name = (char*)items->list[list_index];
    list_index++;



The lines following this do a malloc(), followed by a strcpy() and then return the allocated string.
Isn't that exactly what strdup() does?

core/src/lib/bsys.cc

core/src/lib/crypto_cache.cc

arogge · 2021-10-05T09:26:21Z

core/src/stored/btape.cc

+// macros that check for the return value of read() and write()
+// to avoid warnings about unused results.
+#define READ(FD, BUF, COUNT)                                   \
+  if (read(FD, BUF, COUNT) < 0) {                              \
+    BErrNo be;                                                 \
+    Emsg1(M_FATAL, 0, _("read failed: %s\n"), be.bstrerror()); \
+  }
+
+#define WRITE(FD, BUF, COUNT)                                   \
+  if (write(FD, BUF, COUNT) < 0) {                              \
+    BErrNo be;                                                  \
+    Emsg1(M_FATAL, 0, _("write failed: %s\n"), be.bstrerror()); \
+  }
+
+


I think it is a lot easier to do have inline functions than a macro.
Otherwise you'd need to wrap into do { .. } while(0); so it won't behave strangely.

arogge · 2021-10-06T12:33:44Z

core/src/droplet/libdroplet/src/backend/cdmi/replyparser.c

  }

-  children = json_object_object_get(obj, "children");
+  json_object_object_get_ex(obj, "children", &children);


return code is bool and should probably be checked. The docs doesn't state what children will be set to on error, so we should probably assume it can be undefined and not rely on comparing with NULL only.

good point, will fix that

arogge · 2021-10-06T12:43:39Z

core/src/lib/crypto_openssl.cc

-  int status;
+  int status = 0;

+#  if (OPENSSL_VERSION_NUMBER <= 0x10000000L)


You cannot remove that for OpenSSL 1.0.0. Otherwise locking won't work and threading will break.
However, you can probably remove it for OpenSSL 1.1.0, but please make really sure none of the removed calls is required (we have broken that in the past already).

arogge · 2021-10-06T12:50:41Z

core/src/plugins/dird/python/CMakeLists.txt

  install(TARGETS python-dir DESTINATION ${plugindir})
  target_link_libraries(python-dir ${Python2_LIBRARIES} bareos)
-  target_include_directories(python-dir PUBLIC SYSTEM ${Python2_INCLUDE_DIRS})
+  target_include_directories(python-dir SYSTEM PUBLIC ${Python2_INCLUDE_DIRS})


while this will silence the warning for strict-aliasing, we should probably also set -fno-strict-aliasing as a compile option so the compiler will not generate broken code.

arogge · 2021-10-11T10:00:09Z

core/src/droplet/libdroplet/src/utils.c

    *af = AF_INET6;
  } else {
-    strncpy(new_host, host, host_len);
+    strncpy(new_host, host, strlen(new_host));


if new_host points to newly allocated memory, that has been zero initialized, strlen(new_host) will evalutate to 0, which will lead to not copying host at all.

arogge · 2021-10-11T10:02:37Z

core/src/lib/crypto_cache.cc

-    Dmsg1(000, "Write hdr error: ERR=%s\n", be.bstrerror());
-    goto bail_out;
-  }
+      != sizeof(crypto_cache_hdr)) {}


that doesn't look right.

still not right.

arogge · 2021-10-11T10:10:54Z

core/src/stored/btape.cc

+     _("[file_size=n(GB)|nb_file=3|skip_zero|skip_random|skip_raw|skip_block]"
+       " "
       "report drive speed")},


Suggested change

_("[file_size=n(GB)|nb_file=3|skip_zero|skip_random|skip_raw|skip_block]"

" "

"report drive speed")},

_("[file_size=n(GB)|nb_file=3|skip_zero|skip_random|skip_raw|skip_block]"

" report drive speed")},

arogge · 2021-10-11T13:42:03Z

The old Debian 9.0 and Univention 4.4 systems complain about unused functions and the unknown attribute [[maybe-unused]].

arogge · 2021-10-11T14:02:34Z

Then we should probably disable droplet in i386, as it is definitely broken (or we would have to fix a lot of format-strings)

arogge · 2021-10-12T10:38:07Z

core/src/tests/bsock_test_connection_setup.cc


 static void InitSignalHandler()
 {
+#if !HAVE_WIN32


Suggested change

#if !HAVE_WIN32

#if !defined(HAVE_WIN32)

core/src/lmdb/CMakeLists.txt

core/CMakeLists.txt

arogge · 2021-10-27T13:04:22Z

core/src/lib/crypto_cache.cc

-    Dmsg1(000, "Write hdr error: ERR=%s\n", be.bstrerror());
-    goto bail_out;
-  }
+      != sizeof(crypto_cache_hdr)) {}


still not right.

... as it is in all other places to avoid a warning.

Apparently Solaris can produce strings up to 299 bytes for this, so we increase the size of the buffer to 300 bytes.

LMDB utilizes some kind of old-style variable-length-array. This leads to stringop-overflow warnings, which we can ignore as the out-of-bounds access is intentional (and not really out-of-bounds).

This patch removes the openssl thread callbacks when building with OpenSSL 1.1.0 or newer. However, references to the callbacks are kept, so if any of them was required, compiling would fail.

this is a no-op anyway and will yield a compiler warning, so we disable it.

This patch replaces "%F %T" with the equivalent (and more portable) long variant "%Y-%m-%d %H:%M:%S", so that Windows build will show progress, too.

When initializing BareosWinFilePacket* we now use placement new so the struct values are nicely initialized by our constructor instead of forced zero initialization like before.

Previously for win32 'I' was used, but this is not required anymore and will produce a warning on mingw32.

Previously LINK_LIBRARIES for sd_reservation was overspecified which lead to an issue with ODR (symbols from SD and Dir could have been used).

The macro PLATFORM is already used in pyconfig.h on Windows builds, so we rename our macro to BAREOS_PLATFORM to avoid clashing.

The configuration parser had set the undocumented and unused environment variable BAREOS_CFGDIR, which could potentially have been used in scripts and programs generating configuration snippets. However, as this was never documented and is not used by Bareos itself, we remove it and with it the need for setenv() and/or putenv().

Python should autodetect 32-bit and 64-bit Windows, but this is broken when building with GCC[1]. The previous workaround we had in place defined MS_WIN32 or MS_WIN64 depending on what we're building. As MS_WIN32 is always defined, we should only define MS_WIN64 in case we're building 64-bit Windows. [1] https://bugs.python.org/issue4709

As we currently have no warning left, we reduce the quality gates to be strict.

As droplet is now part of the bareos repo.

As we currently have no warning left, we reduce the quality gates to be strict.

pstorz marked this pull request as draft October 1, 2021 16:07

arogge reviewed Oct 6, 2021

View reviewed changes

pstorz force-pushed the dev/pstorz/master/reduce-warnings branch 2 times, most recently from 4ac2d5a to a3c7bd0 Compare October 8, 2021 13:26

arogge reviewed Oct 11, 2021

View reviewed changes

arogge force-pushed the dev/pstorz/master/reduce-warnings branch from 1ddd4e1 to 01390cd Compare October 11, 2021 13:23

arogge force-pushed the dev/pstorz/master/reduce-warnings branch from 01390cd to 8592330 Compare October 11, 2021 15:47

arogge reviewed Oct 12, 2021

View reviewed changes

arogge force-pushed the dev/pstorz/master/reduce-warnings branch 3 times, most recently from a2c16af to 84ce4d0 Compare October 12, 2021 20:31

pstorz force-pushed the dev/pstorz/master/reduce-warnings branch 3 times, most recently from 838ec7a to 3f3c219 Compare October 19, 2021 14:57

pstorz changed the title ~~build: reduce warnings during build~~ build: remove warnings during build Oct 19, 2021

pstorz marked this pull request as ready for review October 19, 2021 19:01

arogge requested changes Oct 27, 2021

View reviewed changes

pstorz requested a review from arogge October 27, 2021 14:37

arogge force-pushed the dev/pstorz/master/reduce-warnings branch 2 times, most recently from 1e365c7 to d893c06 Compare November 2, 2021 09:30

arogge approved these changes Nov 2, 2021

View reviewed changes

pstorz added 7 commits November 2, 2021 11:59

console.cc: avoid sign compare warning

5127ae7

libdroplet: avoid warning about deprecated readdir_r

6f0ba47

butil.h: declare BootStrapRecord as struct

6a6b5e4

... as it is in all other places to avoid a warning.

droplet: initialize variable to avoid warning

cfe7293

bsys.cc: check write() return code

b3713c1

crypto_cache.cc: check return value of write()

efde968

bscrypto: check for results of read() and write()

fc52374

arogge and others added 27 commits November 2, 2021 11:59

ndmp: resize osbuf to fit maximum length possible

146ffcc

Apparently Solaris can produce strings up to 299 bytes for this, so we increase the size of the buffer to 300 bytes.

lmdb: disable stringop-overflow warning

e6e4408

LMDB utilizes some kind of old-style variable-length-array. This leads to stringop-overflow warnings, which we can ignore as the out-of-bounds access is intentional (and not really out-of-bounds).

lib: don't compile unused openssl callbacks

8e1060e

This patch removes the openssl thread callbacks when building with OpenSSL 1.1.0 or newer. However, references to the callbacks are kept, so if any of them was required, compiling would fail.

tests: disable signal-init on win32

0a42f50

this is a no-op anyway and will yield a compiler warning, so we disable it.

cats: refactor backend loading

31a5e6f

dbcopy: use less fancy time format

7225e35

This patch replaces "%F %T" with the equivalent (and more portable) long variant "%Y-%m-%d %H:%M:%S", so that Windows build will show progress, too.

findlib: use placement new in binit()

a30d4e5

When initializing BareosWinFilePacket* we now use placement new so the struct values are nicely initialized by our constructor instead of forced zero initialization like before.

lmdb: use 'z' instead of 'I' to format size_t

905f05b

Previously for win32 'I' was used, but this is not required anymore and will produce a warning on mingw32.

droplet: build only on 64-bit

a8c1523

tests: work around potential strncpy() problem

abced00

tests: avoid ODR violation in sd_reservation

dbc2ab2

Previously LINK_LIBRARIES for sd_reservation was overspecified which lead to an issue with ODR (symbols from SD and Dir could have been used).

cmake: rename PLATFORM -> BAREOS_PLATFORM

0bc60ca

The macro PLATFORM is already used in pyconfig.h on Windows builds, so we rename our macro to BAREOS_PLATFORM to avoid clashing.

lmdb: improve hack for windows FormatMessageA()

693d527

quality gates: fail on 1 warning

86ea945

As we currently have no warning left, we reduce the quality gates to be strict.

build: make all warnings errors

1357fd6

lmdb: skip target_link_options -Wno-stringop-overflow on bsd and darwin

27e7664

build: avoid clang warnings about gcc compile/link options

8e24854

code format: apply check-sources modifications

73d45c2

lmdb: use CMAKE_C_COMPILER_ID to determine gnu compiler

2b7a815

cmake: set -Wall and -Werror in one call

9e095ac

droplet: do not check for droplet submodule

a83f978

As droplet is now part of the bareos repo.

crypto_cache.cc: re-add accidentially removed code

ac75f0b

quality gates: fail on 1 warning

2ce4aef

As we currently have no warning left, we reduce the quality gates to be strict.

docs: fix newly introduced warnings

d4d2dbb

Update CHANGELOG.md

910adfb

arogge force-pushed the dev/pstorz/master/reduce-warnings branch from d893c06 to 910adfb Compare November 2, 2021 11:01

arogge merged commit 4b078a4 into master Nov 2, 2021

arogge deleted the dev/pstorz/master/reduce-warnings branch November 2, 2021 11:01

Conversation

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

Uh oh!

Thank you for contributing to the Bareos Project!

Please check

Helpful documentation and best practices

Checklist for the reviewer of the PR (will be processed by the Bareos team)

General

Source code quality

Uh oh!

arogge left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

arogge Oct 6, 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!

Choose a reason for hiding this comment

Uh oh!

arogge Oct 6, 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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

arogge commented Oct 11, 2021

Uh oh!

arogge commented Oct 11, 2021

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

pstorz commented Oct 1, 2021 •

edited

Loading

arogge Oct 6, 2021 •

edited

Loading

arogge Oct 6, 2021 •

edited

Loading