vec[1] is equivalent to vec[vec.size()] at that point and thus not a valid
element of the vector, making the use of operator[] undefined behavior here.
With some compiler flags (like those used in package builds on RHEL and
similar), the compiler (rightfully) aborts the program on this out of bounds
access:
68/178 Test #68: base-base_string/vector_move ............................................***Failed 0.01 sec
/usr/include/c++/14/bits/stl_vector.h:1130: std::vector<_Tp, _Alloc>::reference std::vector<_Tp, _Alloc>::operator[](size_type) [with _Tp = icinga::String; _Alloc = std::allocator<icinga::String>; reference = icinga::String&; size_type = long unsigned int]: Assertion '__n < this->size()' failed.
Running 1 test case...
unknown location(0): fatal error: in "base_string/vector_move": signal: SIGABRT (application abort requested)
/builds/packages/icinga2/packaging/fedora/41/BUILD/icinga2-2.14.5+467.g206d7cda1-build/icinga2-2.14.5+467.g206d7cda1/test/base-string.cpp(120): last checkpoint
*** 1 failure is detected in the test module "icinga2"
This commit fixes this by taking the indirection through .data() and using
plain pointer arithmetic instead.
The move `String(Value&&)` constructor tries to partially move `String`
values from a `Value` type. However, since there was no an appropriate
`Value::Get<T>()` implementation that binds to the requested move
operation, the compiler will actually not move the value but copy it
instead as the only available implementation of `Value::Get<T>()`
returns a const reference `const T&`. This commit adds a new overload
that returns a non-const reference and allows to optionally move the string
value of a Value type.
The Icinga DB code performs intensive operations on certain STL containers,
primarily on `std::vector<String>`. Specifically, it inserts 2-3 new elements
at the beginning of a vector containing thousands of elements. Without this commit,
all the existing elements would be unnecessarily copied just to accommodate the new
elements at the front. By making this change, the compiler is able to optimize STL
operations like `push_back`, `emplace_back`, and `insert`, enabling it to prefer the
move constructor over copy operations, provided it is guaranteed that no exceptions
will be thrown.
With a 32-bit time_t, two checks in the FormatDateTime test case didn't work
properly so far:
1. Every time_t value can be represented by struct tm, hence the test makes no
sense on such platforms and is now disabled there similar to how it's
already done with other checks in the same function.
2. std::nextafter(2147483647, +double_limit::infinity())) results in something
like 2147483647.000000238 which simply results in the limit when cast back
to an integer type, so it didn't actually test the overflow. This is fixed
by an additional std::ceil()/std::floor().
This was mistakenly introduced with PR #7686 due to too many open
connections (#7680). This was wrong in the sense that closing the
connection is simply out of place here and should have been handled
differently. After we revised the RPC connection disconnect procedure
with `v2.14.4`, it becomes clear why it is wrong, because the connection
is closed abruptly before the corresponding response (`result`) has
even been written. Now if you remove the disconnect here, shouldn't the
issue #7680 occur again, you ask? The answer is no, because we now also
have a maximum timeout of `10s` for anonymous connections, after which
they are automatically closed. Thanks to the introduction of this
timeout by @julianbrost in #8479, this `Disconnect()` call has become
superfluous.
In fact, this is already done for the outer loop (for each bulk), just not yet for the inner one (for each message of a bulk). So once the remote signals EOF, don't try to process the remaining queue until write error (which can't be associated with a particular message anyway, due to buffering), but just let the peer go. Flush already half-written messages, though, if possible.
