Render numa detection optional with a global configuration statement
'no numa-cpu-mapping'. This can be used if the applied affinity of the
algorithm is not optimal. Also complete the documentation with this new
keyword.
Use the platform independent type hap_cpuset for the cpu-map statement
parsing. This allow to address CPU index greater than LONGBITS.
Update the documentation to reflect the removal of this limit except for
platforms without cpu_set_t type or equivalent.
Since commit 3f12887 ("MINOR: mworker: don't use children variable
anymore"), the oldpids array is not used anymore to generate the new -sf
parameters. So we don't need to set nb_oldpids to 0 during the first
start of the master process.
This patch fixes a bug when 2 masters process tries to synchronize their
peers, there is a small chances that it won't work because nb_oldpids
equals 0.
Should be backported as far as 2.0.
This bug affects the peers synchronisation code which rely on the
nb_oldpids variable to synchronize the peer from the old PID.
In the case the process is not started in master-worker mode and tries
to synchronize using the peers, there is a small chance that won't work
because nb_oldpids equals 0.
Fix the bug by setting the variable to 0 only in the case of the
master-worker when not reloaded.
It could also be a problem when trying to synchronize the peers between
2 masters process which should be fixed in another patch.
Bug exists since commit 8a361b5 ("BUG/MEDIUM: mworker: don't reuse PIDs
passed to the master").
Sould be backported as far as 1.8.
The application of a cpu-map statement with both process and threads
is broken (P-Q/1 or 1/P-Q notation).
For example, before the fix, when using P-Q/1, proc_t1 would be updated.
Then it would be AND'ed with thread which is still 0 and thus does
nothing.
Another problem is when using 1/1[-Q], thread[0] is defined. But if
there is multiple processes, every processes will use this define
affinity even if it should be applied only to 1st process.
The solution to the fix is a little bit too complex for my taste and
there is maybe a simpler solution but I did not wish to break the
storage of global.cpu_map, as it is quite painful to test all the
use-cases. Besides, this code will probably be clean up when
multiprocess support removed on the future version.
Let's try to explain my logic.
* either haproxy runs in multiprocess or multithread mode. If on
multiprocess, we should consider proc_t1 (P-Q/1 notation). If on
multithread, we should consider thread (1/P-Q notation). However
during parsing, the final number of processes or threads is unknown,
thus we have to consider the two possibilities.
* there is a special case for the first thread / first process which is
present in both execution modes. And as a matter of fact cpu-map 1 or
1/1 notation represents the same thing. Thus, thread[0] and proc_t1[0]
represents the same thing. To solve this problem, only thread[0] is
used for this special case.
This fix must be backported up to 2.0.
Config information has been added into the logsrv struct. The filename
is duplicated and should be freed on exit.
Introduced in the current release.
This does not need to be backported.
The current "ADD" vs "ADDQ" is confusing because when thinking in terms
of appending at the end of a list, "ADD" naturally comes to mind, but
here it does the opposite, it inserts. Several times already it's been
incorrectly used where ADDQ was expected, the latest of which was a
fortunate accident explained in 6fa922562 ("CLEANUP: stream: explain
why we queue the stream at the head of the server list").
Let's use more explicit (but slightly longer) names now:
LIST_ADD -> LIST_INSERT
LIST_ADDQ -> LIST_APPEND
LIST_ADDED -> LIST_INLIST
LIST_DEL -> LIST_DELETE
The same is true for MT_LISTs, including their "TRY" variant.
LIST_DEL_INIT keeps its short name to encourage to use it instead of the
lazier LIST_DELETE which is often less safe.
The change is large (~674 non-comment entries) but is mechanical enough
to remain safe. No permutation was performed, so any out-of-tree code
can easily map older names to new ones.
The list doc was updated.
The time initialization was made a bit complex because we rely on a
dummy negative argument to reset all fields, leaving no distinction
between process-level initialization and thread-level initialization.
This patch changes this by introducing two functions, one for the
process and the second one for the threads. This removes ambigous
test and makes sure that the relevant fields are always initialized
exactly once. This also offers a better solution to the bug fixed in
commit b48e7c001 ("BUG/MEDIUM: time: make sure to always initialize
the global tick") as there is no more special values for global_now_ms.
It's simple enough to be backported if any other time-related issues
are encountered in stable versions in the future.
Some variables are mostly read (mostly pointers) but they tend to be
merged with other ones in the same cache line, slowing their access down
in multi-thread setups. This patch declares an empty, aligned variable
in a section called "read_mostly". This will force a cache-line alignment
on this section so that any variable declared in it will be certain to
avoid false sharing with other ones. The section will be eliminated at
link time if not used.
A __read_mostly attribute was added to compiler.h to ease use of this
section.
The fetch_and_xxx variant is often missing for add/sub/and/or. In fact
it was only provided for ADD under the name XADD which corresponds to
the x86 instruction name. But for destructive operations like AND and
OR it's missing even more as it's not possible to know the value before
modifying it.
This patch explicitly adds HA_ATOMIC_FETCH_{OR,AND,ADD,SUB} which
cover these standard operations, and renames XADD to FETCH_ADD (there
were only 6 call places).
In the future, backport of fixes involving such operations could simply
remap FETCH_ADD(x) to XADD(x), FETCH_SUB(x) to XADD(-x), and for the
OR/AND if needed, these could possibly be done using BTS/BTR.
It's worth noting that xchg could have been renamed to fetch_and_store()
but xchg already has well understood semantics and it wasn't needed to
go further.
Currently our atomic ops return a value but it's never known whether
the fetch is done before or after the operation, which causes some
confusion each time the value is desired. Let's create an explicit
variant of these operations suffixed with _FETCH to explicitly mention
that the fetch occurs after the operation, and make use of it at the
few call places.
Define MODE_DIAG which is used to run haproxy in diagnostic mode. This
mode is used to output extra warnings about possible configuration
blunder or sub-optimal usage. It can be activated with argument '-dD'.
A new output function ha_diag_warning is implemented reserved for
diagnostic output. It serves to standardize the format of diagnostic
messages.
A macro HA_DIAG_WARN_COND is also available to automatically check if
diagnostic mode is on before executing the diagnostic check.
Move all liberation code related to a proxy in a dedicated function
free_proxy in proxy.c. For now, this function is only called in
haproxy.c. In the future, it will be used to free the lua proxy.
This helps to clean up haproxy.c.
Move deinit_acl_cond and deinit_act_rules from haproxy.c respectively in
acl.c and action.c. The name of the functions has been slightly altered,
replacing the prefix deinit_* by free_* to reflect their purpose more
clearly.
This change has been made in preparation to the implementation of a free
proxy function. As a side-effect, it helps to clean up haproxy.c.
Create a new module init which contains code related to REGISTER_*
macros for initcalls. init.h is included in api.h to make init code
available to all modules.
It's a step to clean up a bit haproxy.c/global.h.
Declaring a master CLI socket without activating the master-worker mode
is likely a user error, so we issue a warning.
This patch can be backported as far as 1.8.
There were still a very small list of functions, variables and fields
called "stats_" while they were really purely CLI-centric. There's the
frontend called "stats_fe" in the global section, which instantiates a
"cli_applet" called "<CLI>" so it was renamed "cli_fe".
The "alloc_stats_fe" function cas renamed to "cli_alloc_fe" which also
better matches the naming convention of all cli-specific functions.
Finally the "stats_permission_denied_msg" used to return an error on
the CLI was renamed "cli_permission_denied_msg".
Now there's no more "stats_something" that designates the CLI.
The recent default runqueue size reduction appeared to have significantly
lowered performance on low-thread count configs. Testing various values
runqueue values on different workloads under thread counts ranging from
1 to 64, it appeared that lower values are more optimal for high thread
counts and conversely. It could even be drawn that the optimal value for
various workloads sits around 280/sqrt(nbthread), and probably has to do
with both the L3 cache usage and how to optimally interlace the threads'
activity to minimize contention. This is much easier to optimally
configure, so let's do this by default now.
There are multiple per-thread lists in the listeners, which isn't the
most efficient in terms of cache, and doesn't easily allow to store all
the per-thread stuff.
Now we introduce an srv_per_thread structure which the servers will have an
array of, and place the idle/safe/avail conns tree heads into. Overall this
was a fairly mechanical change, and the array is now always initialized for
all servers since we'll put more stuff there. It's worth noting that the Lua
code still has to deal with its own deinit by itself despite being in a
global list, because its server is not dynamically allocated.
It's a real pain not to have access to the list of all registered servers,
because whenever there is a need to late adjust their configuration, only
those attached to regular proxies are seen, but not the peers, lua, logs
nor DNS.
What this patch does is that new_server() will automatically add the newly
created server to a global list, and it does so as well for the 1 or 2
statically allocated servers created for Lua. This way it will be possible
to iterate over all of them.
This makes the code more readable and less prone to copy-paste errors.
In addition, it allows to place some __builtin_constant_p() predicates
to trigger a link-time error in case the compiler knows that the freed
area is constant. It will also produce compile-time error if trying to
free something that is not a regular pointer (e.g. a function).
The DEBUG_MEM_STATS macro now also defines an instance for ha_free()
so that all these calls can be checked.
178 occurrences were converted. The vast majority of them were handled
by the following Coccinelle script, some slightly refined to better deal
with "&*x" or with long lines:
@ rule @
expression E;
@@
- free(E);
- E = NULL;
+ ha_free(&E);
It was verified that the resulting code is the same, more or less a
handful of cases where the compiler optimized slightly differently
the temporary variable that holds the copy of the pointer.
A non-negligible amount of {free(str);str=NULL;str_len=0;} are still
present in the config part (mostly header names in proxies). These
ones should also be cleaned for the same reasons, and probably be
turned into ist strings.
RAND_keep_random_devices_open is OpenSSL specific function, not
implemented in LibreSSL and BoringSSL. Let us define guard
HAVE_SSL_RAND_KEEP_RANDOM_DEVICES_OPEN in include/haproxy/openssl-compat.h
That guard does not depend anymore on HA_OPENSSL_VERSION
Now default proxies are stored into a dedicated tree, sorted by name.
Only unnamed entries are not kept upon new section creation. The very
first call to cfg_parse_listen() will automatically allocate a dummy
defaults section which corresponds to the previous static one, since
the code requires to have one at a few places.
The first immediately visible benefit is that it allows to reuse
alloc_new_proxy() to allocate a defaults section instead of doing it by
hand. And the secret goal is to allow to keep multiple named defaults
section in memory to reuse them from various proxies.
We don't want to expose this one anymore as we'll soon keep multiple
default proxies. Let's move it inside the parser which is the only
place which still uses it, and initialize it on the fly once needed
instead of doing it at boot time.
init_default_instance() was still left in cfgparse.c which is not the
best place to pre-initialize a proxy. Let's place it in proxy.c just
after init_new_proxy(), take this opportunity for renaming it to
proxy_preset_defaults() and taking out init_new_proxy() from it, and
let's pass it the pointer to the default proxy to be initialized instead
of implicitly assuming defproxy. We'll soon be able to exploit this.
Only two call places had to be updated.
The server idle/safe/available connection lists are replaced with ebmb-
trees. This is used to store backend connections, with the new field
connection hash as the key. The hash is a 8-bytes size field, used to
reflect specific connection parameters.
This is a preliminary work to be able to reuse connection with SNI,
explicit src/dst address or PROXY protocol.
The HAPROXY_CFGFILES env variable is built using a static trash chunk, via a
call to get_trash_chunk() function. This chunk is reserved during the whole
configuration parsing. It is far too large to guarantee it will not be
reused during the configuration parsing. And in fact, it happens in the lua
code since the commit f67442efd ("BUG/MINOR: lua: warn when registering
action, conv, sf, cli or applet multiple times"), when a lua script is
loaded.
To fix the bug, we now use a dynamic buffer instead. And we call memprintf()
function to handle both the allocation and the formatting. Allocation errors
at this stage are fatal.
This patch should fix the issue #1041. It must be backported as far as 2.0.
The strict-limits global option was introduced with commit 0fec3ab7b
("MINOR: init: always fail when setrlimit fails"). When used in
conjuction with master-worker, haproxy will not fail when a setrlimit
fails. This happens because we only exit() if master-worker isn't used.
This patch removes all tests for master-worker mode for all cases covered
by strict-limits scope.
This should be backported from 2.1 onward.
This should fix issue #1042.
Reviewed by William Dauchy <wdauchy@gmail.com>
GitHub Issue #1037 Reported a memory leak in deinit() caused by an
allocation made in sa2str() that was stored in srv_set_addr_desc().
When destroying each server for a proxy in deinit, include freeing the
memory in the key of server->addr_node.
The leak was introduced in commit 92149f9a8 ("MEDIUM: stick-tables: Add
srvkey option to stick-table") which is not in any released version so
no backport is needed.
Cc: Tim Duesterhus <tim@bastelstu.be>
This is from the output of codespell. It's done at once over a bunch
of files and only affects comments, so there is nothing user-visible.
No backport needed.
Released version 2.4-dev5 with the following main changes :
- BUG/MEDIUM: mux_h2: Add missing braces in h2_snd_buf()around trace+wakeup
- BUILD: hpack: hpack-tbl-t.h uses VAR_ARRAY but does not include compiler.h
- MINOR: time: increase the minimum wakeup interval to 60s
- MINOR: check: do not ignore a connection header for http-check send
- REGTESTS: complete http-check test
- CI: travis-ci: drop coverity scan builds
- MINOR: atomic: don't use ; to separate instruction on aarch64.
- IMPORT: xxhash: update to v0.8.0 that introduces stable XXH3 variant
- MEDIUM: xxhash: use the XXH3 functions to generate 64-bit hashes
- MEDIUM: xxhash: use the XXH_INLINE_ALL macro to inline all functions
- CLEANUP: xxhash: remove the unused src/xxhash.c
- MINOR: sample: add the xxh3 converter
- REGTESTS: add tests for the xxh3 converter
- MINOR: protocol: Create proto_quic QUIC protocol layer.
- MINOR: connection: Attach a "quic_conn" struct to "connection" struct.
- MINOR: quic: Redefine control layer callbacks which are QUIC specific.
- MINOR: ssl_sock: Initialize BIO and SSL objects outside of ssl_sock_init()
- MINOR: connection: Add a new xprt to connection.
- MINOR: ssl: Export definitions required by QUIC.
- MINOR: cfgparse: Do not modify the QUIC xprt when parsing "ssl".
- MINOR: tools: Add support for QUIC addresses parsing.
- MINOR: quic: Add definitions for QUIC protocol.
- MINOR: quic: Import C source code files for QUIC protocol.
- MINOR: listener: Add QUIC info to listeners and receivers.
- MINOR: server: Add QUIC definitions to servers.
- MINOR: ssl: SSL CTX initialization modifications for QUIC.
- MINOR: ssl: QUIC transport parameters parsing.
- MINOR: quic: QUIC socket management finalization.
- MINOR: cfgparse: QUIC default server transport parameters init.
- MINOR: quic: Enable the compilation of QUIC modules.
- MAJOR: quic: Make usage of ebtrees to store QUIC ACK ranges.
- MINOR: quic: Attempt to make trace more readable
- MINOR: quic: Make usage of the congestion control window.
- MINOR: quic: Flag RX packet as ack-eliciting from the generic parser.
- MINOR: quic: Code reordering to help in reviewing/modifying.
- MINOR: quic: Add traces to congestion avoidance NewReno callback.
- MINOR: quic: Display the SSL alert in ->ssl_send_alert() callback.
- MINOR: quic: Update the initial salt to that of draft-29.
- MINOR: quic: Add traces for in flght ack-eliciting packet counter.
- MINOR: quic: make a packet build fails when qc_build_frm() fails.
- MINOR: quic: Add traces for quic_packet_encrypt().
- MINOR: cache: Refactoring of secondary_key building functions
- MINOR: cache: Avoid storing responses whose secondary key was not correctly calculated
- BUG/MINOR: cache: Manage multiple headers in accept-encoding normalization
- MINOR: cache: Add specific secondary key comparison mechanism
- MINOR: http: Add helper functions to trim spaces and tabs
- MEDIUM: cache: Manage a subset of encodings in accept-encoding normalizer
- REGTESTS: cache: Simplify vary.vtc file
- REGTESTS: cache: Add a specific test for the accept-encoding normalizer
- MINOR: cache: Remove redundant test in http_action_req_cache_use
- MINOR: cache: Replace the "process-vary" option's expected values
- CI: GitHub Actions: enable daily Coverity scan
- BUG/MEDIUM: cache: Fix hash collision in `accept-encoding` handling for `Vary`
- MEDIUM: stick-tables: Add srvkey option to stick-table
- REGTESTS: add test for stickiness using "srvkey addr"
- BUILD: Makefile: disable -Warray-bounds until it's fixed in gcc 11
- BUG/MINOR: sink: Return an allocation failure in __sink_new if strdup() fails
- BUG/MINOR: lua: Fix memory leak error cases in hlua_config_prepend_path
- MINOR: lua: Use consistent error message 'memory allocation failed'
- CLEANUP: Compare the return value of `XXXcmp()` functions with zero
- CLEANUP: Apply the coccinelle patch for `XXXcmp()` on include/
- CLEANUP: Apply the coccinelle patch for `XXXcmp()` on contrib/
- MINOR: qpack: Add static header table definitions for QPACK.
- CLEANUP: qpack: Wrong comment about the draft for QPACK static header table.
- CLEANUP: quic: Remove useless QUIC event trace definitions.
- BUG/MINOR: quic: Possible CRYPTO frame building errors.
- MINOR: quic: Pass quic_conn struct to frame parsers.
- BUG/MINOR: quic: Wrong STREAM frames parsing.
- MINOR: quic: Drop packets with STREAM frames with wrong direction.
- CLEANUP: ssl: Remove useless loop in tlskeys_list_get_next()
- CLEANUP: ssl: Remove useless local variable in tlskeys_list_get_next()
- MINOR: ssl: make tlskeys_list_get_next() take a list element
- Revert "BUILD: Makefile: disable -Warray-bounds until it's fixed in gcc 11"
- BUG/MINOR: cfgparse: Fail if the strdup() for `rule->be.name` for `use_backend` fails
- CLEANUP: mworker: remove duplicate pointer tests in cfg_parse_program()
- CLEANUP: Reduce scope of `header_name` in http_action_store_cache()
- CLEANUP: Reduce scope of `hdr_age` in http_action_store_cache()
- CLEANUP: spoe: fix typo on `var_check_arg` comment
- BUG/MINOR: tcpcheck: Report a L7OK if the last evaluated rule is a send rule
- CI: github actions: build several popular "contrib" tools
- DOC: Improve the message printed when running `make` w/o `TARGET`
- BUG/MEDIUM: server: srv_set_addr_desc() crashes when a server has no address
- REGTESTS: add unresolvable servers to srvkey-addr
- BUG/MINOR: stats: Make stat_l variable used to dump a stat line thread local
- BUG/MINOR: quic: NULL pointer dereferences when building post handshake frames.
- SCRIPTS: improve announce-release to support different tag and versions
- SCRIPTS: make announce release support preparing announces before tag exists
- CLEANUP: assorted typo fixes in the code and comments
- BUG/MINOR: srv: do not init address if backend is disabled
- BUG/MINOR: srv: do not cleanup idle conns if pool max is null
- CLEANUP: assorted typo fixes in the code and comments
- CLEANUP: few extra typo and fixes over last one ("ot" -> "to")
This option is now ignored because I/O check buffers are now allocated using the
buffer pool. Thus, it is marked as deprecated in the documentation and ignored
during the configuration parsing. The field is also removed from the global
structure.
Because this option is ignored since a recent fix, backported as fare as 2.2,
this patch should be backported too. Especially because it updates the
documentation.
"RAND_keep_random_devices_open" is OpenSSL specific, does not present
in other OpenSSL variants like LibreSSL or BoringSSL. BoringSSL recently
"updated" its internal openssl version to 1.1.1, we temporarily set it
back to 1.1.0, as we are going to remove that hack, let us add proper
guarding.
Functions registered to release memory per-thread have no return value. But the
registering function and the function pointer in per_thread_free_fct structure
specify it should return an integer. This patch fixes it.
This patch may be backported as far as 2.0.
per-proxy and per-server post-check callback functions must be skipped for
disabled proxies because most of the configuration validity check is skipped for
these proxies.
This patch must be backported as far as 2.1.
This is an anticipation of finer grained locking for the queues. For now
all lock places take a write lock so that there is no difference at all
with previous code.
As previously discussed, nbproc usage is bad, deprecated, and scheduled
for removal in 2.5.
If "nbproc" is found with more than one process while nbthread is not
set, a warning will be emitted encouraging to remove it or to migrate
to nbthread instead. This makes sure the user has an opportunity to
both see the message and silence it.
It was previously a spinlock, and it happens that a number of LB algos
only lock it for lookups, without performing any modification. Let's
first turn it to an rwlock and w-lock it everywhere. This is strictly
identical.
It was carefully checked that every HA_SPIN_LOCK() was turned to
HA_RWLOCK_WRLOCK() and that HA_SPIN_UNLOCK() was turned to
HA_RWLOCK_WRUNLOCK() on this lock. _INIT and _DESTROY were updated too.
Now we define a new sock_accept_iocb() for socket-based stream protocols
and use it as a wrapper for listener_accept() which now takes a listener
and not an FD anymore. This will allow the receiver's I/O cb to be
redefined during registration, and more specifically to get rid of the
hard-coded hacks in protocol_bind_all() made for syslog.
The previous ->accept() callback in the protocol was removed since it
doesn't have anything to do with accept() anymore but is more generic.
A few places where listener_accept() was compared against the FD's IO
callback for debugging purposes on the CLI were updated.
When running a pure config check (haproxy -c) we go through the deinit
phase without having allocated fdtab, so we can't blindly dereference
it. The issue was added by recent commit ae7bc4a23 ("MEDIUM: deinit:
close all receivers/listeners before scanning proxies"), no backport is
needed.
On some operating systems, RLIM_INFINITY is set to -1 so that when the
hard limit on the number of FDs is set to unlimited, taking the MAX
of both values keeps rlim_fd_cur and everything works. But on other
systems this values is defined as the highest positive integer. This
is what was observed on a 32-bit AIX 5.1. The effect is that maxsock
becomes 2^31-1 and that fdtab allocation fails.
Note that a simple workaround consists in manually setting maxconn in
the global section.
Let's ignore unlimited as soon as we retrieve rlim_fd_max so that all
systems behave consistently.
This may be backported as far as 2.0, though it doesn't seem like it
has annoyed anyone.
When factoring out the pause/resume error messages in commit 775e00158
("MAJOR: signals: use protocol_pause_all() and protocol_resume_all()")
I forgot that ha_warning() and send_log() take a format string and not
just a const string. No backport is needed, this is 2.3-dev.
When temporarily pausing the listeners with SIG_TTOU, we now pause
all listeners via the protocols instead of the proxies. This has the
benefits that listeners are paused regardless of whether or not they
belong to a visible proxy. And for resuming via SIG_TTIN we do the
same, which allows to report binding conflicts and address them,
since the operation can be repeated on a per-listener basis instead
of a per-proxy basis.
While in appearance all cases were properly handled, it's impossible
to completely rule out the possibility that something broken used to
work by luck due to the scan ordering which is naturally different,
hence the major tag.
The two functions don't need to be distinguished anymore since they have
all the necessary info to act as needed on their listeners. Let's just
pass via stop_proxy() and make it check for each listener which one to
close or not.
Its sole remaining purpose was to display "proxy foo started", which
has little benefit and pollutes output for those with plenty of proxies.
Let's remove it now.
The VTCs were updated to reflect this, because many of them had explicit
counts of dropped lines to match this message.
This is tagged as MEDIUM because some users may be surprized by the
loss of this quite old message.
The remaining proxy states were only used to distinguish an enabled
proxy from a disabled one. Due to the initialization order, both
PR_STNEW and PR_STREADY were equivalent after startup, and they
would only differ from PR_STSTOPPED when the proxy is disabled or
shutdown (which is effectively another way to disable it).
Now we just have a "disabled" field which allows to distinguish them.
It's becoming obvious that start_proxies() is only used to print a
greeting message now, that we'd rather get rid of. Probably that
zombify_proxy() and stop_proxy() should be merged once their
differences move to the right place.
Instead of looking at listeners in proxies in PR_STNEW state, we'd
rather check for listeners in those not in PR_STSTOPPED as it's only
this state which indicates the proxy was disabled. And let's check
the listeners count instead of testing the list's head.
Because of the zombie state, proxies have a skewed vision of the state
of listeners, which explains why there are hacks switching the state
from ZOMBIE to INIT in the proxy cleaning loop. This is particularly
complicated and not needed, as all the information is now available
in the protocol list and the fdtab.
What we do here instead is to first close all active listeners or
receivers by protocol and clean their protocol parts. Then we scan the
fdtab to get rid of remaining ones that were necessarily in INIT state
after a previous invocation of delete_listener(). From this point, we
know the listeners are cleaned, the can safely be freed by scanning the
proxies.
During the startup process we don't have any fdtab nor fd_updt for quite
a long time, and as such some operations on the listeners are not
permitted, such as fd_want_*/fd_stop_* or fd_delete(). The latter is of
particular concern because it's used when stopping a disabled frontend,
and it's performed very early during check_config_validity() while there
is no fdtab yet. The trick till now relies on the listener's state which
is a bit brittle.
There is absolutely no valid reason for stopping a proxy's listeners this
early, we can postpone it after init_pollers() which will at least have
allocated fdtab.
Old processes didn't die if a log foward section is declared and
a soft stop is requested.
This patch fix this issue and should be backpored in banches including
the log forward feature.
This is executed on startup with the registered statistics module. The
existing statistics have been merged in a list containing all
statistics for each domain. This is useful to print all available
statistics in a generic way.
Allocate extra counters for all proxies/servers/listeners instances.
These counters are allocated with the counters from the stats modules
registered on startup.
We use chunk_initstr() to store the program name as the default log-tag.
If we use the log-tag directive in the config file, this chunk will be
destroyed and replaced. chunk_initstr() sets the chunk size to 0 so we
will free the chunk itself, but not its content.
This happens for a global section and also for a proxy.
We fix this by using chunk_initlen() instead of chunk_initstr().
We also check that the memory allocation was successfull, otherwise we quit.
This fixes github issue #850.
It can be backported as far as 1.9, with minor adjustments to includes.
The code dealing with zombie proxies in soft_stop() is bogus, it uses
close() instead of fd_delete(), leaving a live entry in the fdtab with
a dangling pointer to a free memory location. The FD might be reassigned
for an outgoing connection for the time it takes the proxy to completely
stop, or could be dumped on the CLI's "show fd" command. In addition,
the listener's FD was not even reset, leaving doubts about whether or
not it will happen again in deinit().
And in deinit(), the loop in charge of closing zombie FDs is particularly
unsafe because it closes the fd then calls unbind_listener() then
delete_listener() hoping none of them will touch it again. Since it
requires some mental efforts to figure what's done there, let's correctly
reset the fd here as well and close it using fd_delete() to eliminate any
remaining doubts.
It's uncertain whether this should be backported. Zombie proxies are rare
and the situations capable of triggering such issues are not trivial to
setup. However it's easy to imagine how things could go wrong if backported
too far. Better wait for any matching report if at all (this code has been
there since 1.8 without anobody noticing).
The listening socket is represented by its file descriptor, which is
generic to all receivers and not just listeners, so it must move to
the rx struct.
It's worth noting that in order to extend receivers and listeners to
other protocols such as QUIC, we'll need other handles than file
descriptors here, and that either a union or a cast to uintptr_t
will have to be used. This was not done yet and the field was
preserved under the name "fd" to avoid adding confusion.
Changes performed using the following coccinelle patch:
@@
type T;
expression E;
expression t;
@@
(
t = calloc(E, sizeof(*t))
|
- t = calloc(E, sizeof(T))
+ t = calloc(E, sizeof(*t))
)
Looking through the commit history, grepping for coccinelle shows that the same
replacement with a different patch was already performed in the past in commit
02779b6263.
This allocation technically is always reachable and cannot leak, however other
global variables such as `oldpids` are already being freed. This is in an
attempt to get HAProxy to a state where there are zero live allocations after a
clean exit.
Given the following example configuration:
listen http
bind *:80
mode http
stats scope .
Running a configuration check with valgrind reports:
==16341== 26 (24 direct, 2 indirect) bytes in 1 blocks are definitely lost in loss record 3 of 13
==16341== at 0x4C2FB55: calloc (in /usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so)
==16341== by 0x571C2E: stats_add_scope (uri_auth.c:296)
==16341== by 0x46CE29: cfg_parse_listen (cfgparse-listen.c:1901)
==16341== by 0x45A112: readcfgfile (cfgparse.c:2078)
==16341== by 0x50A0F5: init (haproxy.c:1828)
==16341== by 0x418248: main (haproxy.c:3012)
After this patch is applied the leak is gone as expected.
This is a very minor leak that can only be observed if deinit() is called,
shortly before the OS will free all memory of the process anyway. No
backport needed.
It was reported in bug #837 that haproxy -s causes a 100% CPU.
However this option does not exist and haproxy must exit with the
usage message.
The parser was not handling the case where -s is not followed by 't' or
'f' which are the only two valid cases.
This bug was introduced by df6c5a ("BUG/MEDIUM: mworker: fix the copy of
options in copy_argv()") which was backported as far as 1.8.
This fix must be backported as far as 1.8.
Ever since the protocols were added in 1.3.13, listeners used to be
started twice:
- once by start_proxies(), which iteratees over all proxies then all
listeners ;
- once by protocol_bind_all() which iterates over all protocols then
all listeners ;
It's a real mess because error reporting is not even consistent, and
more importantly now that some protocols do not appear in regular
proxies (peers, logs), there is no way to retry their binding should
it fail on the last step.
What this patch does is to make sure that listeners are exclusively
started by protocols. The failure to start a listener now causes the
emission of an error indicating the proxy's name (as it used to be
the case per proxy), and retryable failures are silently ignored
during all but last attempts.
The start_proxies() function was kept solely for setting the proxy's
state to READY and emitting the "Proxy started" message and log that
some have likely got used to seeking in their logs.
The new function was called sock_get_old_sockets() and was left as-is
except a minimum amount of style lifting to make it more readable. It
will never be awesome anyway since it's used very early in the boot
sequence and needs to perform socket I/O without any external help.
The function now makes it clear that it's independent on the socket
type and solely relies on the address family. Note that it supports
both IPv4 and IPv6 as we don't seem to need it per-family.
get_old_sockets() mistakenly sets ret=0 instead of ret2=0 before leaving
when the old process announces zero FD. So it will return an error
instead of success. This must be particularly rare not to have a
single socket to offer though!
A few comments were added to make it more obvious what to expect in
return.
This must be backported to 1.8 since the bug has always been there.
During a reload operation, we used to send listener options associated
with each passed file descriptor. These were passed as binary contents
for the size of the "options" field in the struct listener. This means
that any flag value change or field size change would be problematic,
the former failing to properly grab certain options, the latter possibly
causing permanent failures during this operation.
Since these two previous commits:
MINOR: reload: determine the foreing binding status from the socket
BUG/MINOR: reload: detect the OS's v6only status before choosing an old socket
we don't need this anymore as the values are determined from the file
descriptor itself.
Let's just turn the previous 32 bits to vestigal space, send them as
zeroes and ignore them on receipt. The only possible side effect is if
someone would want to roll back from a 2.3 to 2.2 or earlier, such options
might be ignored during this reload. But other forthcoming changes might
make this fail as well anyway so that's not a reason for keeping this
behavior.
Let's not look at the listener options passed by the original process
and determine from the socket itself whether it is configured for
transparent mode or not. This is cleaner and safer, and doesn't rely
on flag values that could possibly change between versions.
The v4v6 and v6only options are passed as data during the socket transfer
between processes so that the new process can decide whether it wants to
reuse a socket or not. But this actually misses one point: if no such option
is set and the OS defaults are changed between the reloads, then the socket
will still be inherited and will never be rebound using the new options.
This can be seen by starting the following config:
global
stats socket /tmp/haproxy.sock level admin expose-fd listeners
frontend testme
bind :::1234
timeout client 2000ms
Having a look at the OS settins, v6only is disabled:
$ cat /proc/sys/net/ipv6/bindv6only
0
A first check shows it's indeed bound to v4 and v6:
$ ss -an -6|grep 1234
tcp LISTEN 0 2035 *:1234 *:*
Reloading the process doesn't change anything (which is expected). Now let's set
bindv6only:
$ echo 1 | sudo tee /proc/sys/net/ipv6/bindv6only
1
$ cat /proc/sys/net/ipv6/bindv6only
1
Reloading gives the same state:
$ ss -an -6|grep 1234
tcp LISTEN 0 2035 *:1234 *:*
However a restart properly shows a correct bind:
$ ss -an -6|grep 1234
tcp LISTEN 0 2035 [::]:1234 [::]:*
This one doesn't change once bindv6only is reset, for the same reason.
This patch attacks this problem differently. Instead of passing the two
options at once for each listening fd, it ignores the options and reads
the socket's current state for the IPV6_V6ONLY flag and sets it only.
Then before looking for a compatible FD, it checks the OS's defaults
before deciding which of the v4v6 and v6only needs to be kept on the
listener. And the selection is only made on this.
First, it addresses this issue. Second, it also ensures that if such
options are changed between reloads to identical states, the socket
can still be inherited. For example adding v4v6 when bindv6only is not
set will allow the socket to still be usable. Third, it avoids an
undesired dependency on the LI_O_* bit values between processes across
a reload (for these ones at least).
It might make sense to backport this to some recent stable versions, but
quite frankly the likelyhood that anyone will ever notice it is extremely
faint.
Free the snapshots on deinit() when they were initialized in a proxy
upon an error.
This was introduced by c55015e ("MEDIUM: snapshots: dynamically allocate
the snapshots").
Should be backported as far as 1.9.
Given the following example configuration:
backend foo
mode http
use-server %[str(x)] if { always_true }
server x example.com:80
Running a configuration check with valgrind reports:
==19376== 170 (40 direct, 130 indirect) bytes in 1 blocks are definitely lost in loss record 281 of 347
==19376== at 0x4C2FB55: calloc (in /usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so)
==19376== by 0x5091AC: add_sample_to_logformat_list (log.c:511)
==19376== by 0x50A5A6: parse_logformat_string (log.c:671)
==19376== by 0x4957F2: check_config_validity (cfgparse.c:2588)
==19376== by 0x54442D: init (haproxy.c:2129)
==19376== by 0x421E42: main (haproxy.c:3169)
After this patch is applied the leak is gone as expected.
This is a very minor leak that can only be observed if deinit() is called,
shortly before the OS will free all memory of the process anyway. No
backport needed.
Given the following example configuration:
backend foo
mode http
use-server x if { always_true }
server x example.com:80
Running a configuration check with valgrind reports:
==18650== 14 bytes in 1 blocks are definitely lost in loss record 3 of 345
==18650== at 0x4C2DB8F: malloc (in /usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so)
==18650== by 0x649E489: strdup (strdup.c:42)
==18650== by 0x4A5438: cfg_parse_listen (cfgparse-listen.c:1548)
==18650== by 0x494C59: readcfgfile (cfgparse.c:2049)
==18650== by 0x5450B5: init (haproxy.c:2029)
==18650== by 0x421E42: main (haproxy.c:3168)
After this patch is applied the leak is gone as expected.
This is a very minor leak that can only be observed if deinit() is called,
shortly before the OS will free all memory of the process anyway. No
backport needed.
Given the following example configuration:
frontend foo
mode http
bind *:8080
unique-id-header x
Running a configuration check with valgrind reports:
==17621== 2 bytes in 1 blocks are definitely lost in loss record 1 of 341
==17621== at 0x4C2DB8F: malloc (in /usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so)
==17621== by 0x649E489: strdup (strdup.c:42)
==17621== by 0x4A87F1: cfg_parse_listen (cfgparse-listen.c:2747)
==17621== by 0x494C59: readcfgfile (cfgparse.c:2049)
==17621== by 0x545095: init (haproxy.c:2029)
==17621== by 0x421E42: main (haproxy.c:3167)
After this patch is applied the leak is gone as expected.
This is a very minor leak that can only be observed if deinit() is called,
shortly before the OS will free all memory of the process anyway. No
backport needed.
Given the following example configuration:
resolvers test
nameserver test 127.0.0.1:53
listen foo
bind *:8080
server foo example.com resolvers test
Running a configuration check within valgrind reports:
==21995== 5 bytes in 1 blocks are definitely lost in loss record 1 of 30
==21995== at 0x4C2DB8F: malloc (in /usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so)
==21995== by 0x5726489: strdup (strdup.c:42)
==21995== by 0x4B2CFB: parse_server (server.c:2163)
==21995== by 0x4680C1: cfg_parse_listen (cfgparse-listen.c:534)
==21995== by 0x459E33: readcfgfile (cfgparse.c:2167)
==21995== by 0x50778D: init (haproxy.c:2021)
==21995== by 0x418262: main (haproxy.c:3133)
==21995==
==21995== 12 bytes in 1 blocks are definitely lost in loss record 3 of 30
==21995== at 0x4C2DB8F: malloc (in /usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so)
==21995== by 0x5726489: strdup (strdup.c:42)
==21995== by 0x4AC666: srv_prepare_for_resolution (server.c:1606)
==21995== by 0x4B2EBD: parse_server (server.c:2081)
==21995== by 0x4680C1: cfg_parse_listen (cfgparse-listen.c:534)
==21995== by 0x459E33: readcfgfile (cfgparse.c:2167)
==21995== by 0x50778D: init (haproxy.c:2021)
==21995== by 0x418262: main (haproxy.c:3133)
with one more leak unrelated to `struct server`. After applying this
patch the leak is gone as expected.
This is a very minor leak that can only be observed if deinit() is called,
shortly before the OS will free all memory of the process anyway. No
backport needed.
Given the following example configuration:
frontend foo
mode http
bind *:8080
unique-id-format x
Running a configuration check with valgrind reports:
==30712== 42 (40 direct, 2 indirect) bytes in 1 blocks are definitely lost in loss record 18 of 39
==30712== at 0x4C2FB55: calloc (in /usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so)
==30712== by 0x4ED7E9: add_to_logformat_list (log.c:462)
==30712== by 0x4EEE28: parse_logformat_string (log.c:720)
==30712== by 0x47B09A: check_config_validity (cfgparse.c:3046)
==30712== by 0x52881D: init (haproxy.c:2121)
==30712== by 0x41F382: main (haproxy.c:3126)
After this patch is applied the leak is gone as expected.
This is a very minor leak that can only be observed if deinit() is called,
shortly before the OS will free all memory of the process anyway. No
backport needed.
As reported in issue #724, openbsd fails to build in haproxy.c
due to a faulty comma in the middle of a warning message. This code
is only compiled when RLIMIT_AS is not defined, which seems to be
rare these days.
This may be backported to older versions as the problem was likely
introduced when strict limits were added.
Enables ('on') or disables ('off') sharing of idle connection pools between
threads for a same server. The default is to share them between threads in
order to minimize the number of persistent connections to a server, and to
optimize the connection reuse rate. But to help with debugging or when
suspecting a bug in HAProxy around connection reuse, it can be convenient to
forcefully disable this idle pool sharing between multiple threads, and force
this option to "off". The default is on.
This could have been nice to have during the idle connections debugging,
but it's not too late to add it!
Commit d645574 ("MINOR: soft-stop: let the first stopper only signal
other threads") introduced a minor mistake which is that when a stopping
thread signals all other threads, it also signals itself. When
single-threaded, the process constantly wakes up while waiting for
last connections to exit. Let's reintroduce the lost mask to avoid
this.
No backport is needed, this is 2.2-dev only.
Move the ckch_deinit() and crtlist_deinit() call to ssl_sock.c,
also unlink the SNI from the ckch_inst because they are free'd before in
ssl_sock_free_all_ctx().
Add some functions to deinit the whole crtlist and ckch architecture.
It will free all crtlist, crtlist_entry, ckch_store, ckch_inst and their
associated SNI, ssl_conf and SSL_CTX.
The SSL_CTX in the default_ctx and initial_ctx still needs to be free'd
separately.
Initial default settings for maxconn/maxsock/maxpipes were rearranged
in commit a409f30d0 ("MINOR: init: move the maxsock calculation code
to compute_ideal_maxsock()") but as a side effect, the calculated
maxpipes value was not stored anymore into global.maxpipes. This
resulted in splicing being disabled unless there is an explicit
maxpipes setting in the global section.
This patch just stores the calculated ideal value as planned in the
computation and as was done before the patch above.
This is strictly 2.2, no backport is needed.
Nowadays signals cause tasks to be woken up. The historic code still
processes signals after tasks, which forces a second round in the loop
before they can effectively be processed. Let's move the signal queue
handling between wake_expired_tasks() and process_runnable_tasks() where
it makes much more sense.
localpeer <name>
Sets the local instance's peer name. It will be ignored if the "-L"
command line argument is specified or if used after "peers" section
definitions. In such cases, a warning message will be emitted during
the configuration parsing.
This option will also set the HAPROXY_LOCALPEER environment variable.
See also "-L" in the management guide and "peers" section in the
configuration manual.
For an unknown reason in commit bb1b63c079 I placed the compiler version
output in haproxy.c instead of version.c. Better have it in version.c which
is more suitable to this sort of things.
We cannot simply `release_sample_expr(rule->arg.vars.expr)` for a
`struct act_rule`, because `rule->arg` is a union that might not
contain valid `vars`. This leads to a crash on a configuration using
`http-request redirect` and possibly others:
frontend http
mode http
bind 127.0.0.1:80
http-request redirect scheme https
Instead a `struct act_rule` has a `release_ptr` that must be used
to properly free any additional storage allocated.
This patch fixes a regression in commit ff78fcdd7f.
It must be backported to whereever that patch is backported.
It has be verified that the configuration above no longer crashes.
It has also been verified that the configuration in ff78fcdd7f
does not leak.
Commit 0a3b43d9c ("MINOR: haproxy: Make use of deinit_and_exit() for
clean exits") introduced this build warning:
src/haproxy.c: In function 'main':
src/haproxy.c:3775:1: warning: control reaches end of non-void function [-Wreturn-type]
}
^
This is because the new deinit_and_exit() is not marked as "noreturn"
so depending on the optimizations, the noreturn attribute of exit() will
either leak through it and silence the warning or not and confuse the
compiler. Let's just add the attribute to fix this.
No backport is needed, this is purely 2.2.
Given the following example configuration:
frontend foo
bind *:8080
mode http
http-request set-var(txn.foo) str(bar)
Running a configuration check within valgrind reports:
==23665== Memcheck, a memory error detector
==23665== Copyright (C) 2002-2015, and GNU GPL'd, by Julian Seward et al.
==23665== Using Valgrind-3.11.0 and LibVEX; rerun with -h for copyright info
==23665== Command: ./haproxy -c -f ./crasher.cfg
==23665==
[WARNING] 165/002941 (23665) : config : missing timeouts for frontend 'foo'.
| While not properly invalid, you will certainly encounter various problems
| with such a configuration. To fix this, please ensure that all following
| timeouts are set to a non-zero value: 'client', 'connect', 'server'.
Warnings were found.
Configuration file is valid
==23665==
==23665== HEAP SUMMARY:
==23665== in use at exit: 314,008 bytes in 87 blocks
==23665== total heap usage: 160 allocs, 73 frees, 1,448,074 bytes allocated
==23665==
==23665== 132 (48 direct, 84 indirect) bytes in 1 blocks are definitely lost in loss record 15 of 28
==23665== at 0x4C2FB55: calloc (in /usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so)
==23665== by 0x4A2612: sample_parse_expr (sample.c:876)
==23665== by 0x54DF84: parse_store (vars.c:766)
==23665== by 0x528BDF: parse_http_req_cond (http_rules.c:95)
==23665== by 0x469F36: cfg_parse_listen (cfgparse-listen.c:1339)
==23665== by 0x459E33: readcfgfile (cfgparse.c:2167)
==23665== by 0x5074FD: init (haproxy.c:2021)
==23665== by 0x418262: main (haproxy.c:3126)
==23665==
==23665== LEAK SUMMARY:
==23665== definitely lost: 48 bytes in 1 blocks
==23665== indirectly lost: 84 bytes in 2 blocks
==23665== possibly lost: 0 bytes in 0 blocks
==23665== still reachable: 313,876 bytes in 84 blocks
==23665== suppressed: 0 bytes in 0 blocks
==23665== Reachable blocks (those to which a pointer was found) are not shown.
==23665== To see them, rerun with: --leak-check=full --show-leak-kinds=all
==23665==
==23665== For counts of detected and suppressed errors, rerun with: -v
==23665== ERROR SUMMARY: 1 errors from 1 contexts (suppressed: 0 from 0)
After this patch is applied the leak is gone as expected.
This is a very minor leak that can only be observed if deinit() is called,
shortly before the OS will free all memory of the process anyway. No
backport needed.
Particularly cleanly deinit() after a configuration check to clean up the
output of valgrind which reports "possible losses" without a deinit() and
does not with a deinit(), converting actual losses into proper hard losses
which makes the whole stuff easier to analyze.
As an example, given an example configuration of the following:
frontend foo
bind *:8080
mode http
Running `haproxy -c -f cfg` within valgrind will report 4 possible losses:
$ valgrind --leak-check=full ./haproxy -c -f ./example.cfg
==21219== Memcheck, a memory error detector
==21219== Copyright (C) 2002-2015, and GNU GPL'd, by Julian Seward et al.
==21219== Using Valgrind-3.11.0 and LibVEX; rerun with -h for copyright info
==21219== Command: ./haproxy -c -f ./example.cfg
==21219==
[WARNING] 165/001100 (21219) : config : missing timeouts for frontend 'foo'.
| While not properly invalid, you will certainly encounter various problems
| with such a configuration. To fix this, please ensure that all following
| timeouts are set to a non-zero value: 'client', 'connect', 'server'.
Warnings were found.
Configuration file is valid
==21219==
==21219== HEAP SUMMARY:
==21219== in use at exit: 1,436,631 bytes in 130 blocks
==21219== total heap usage: 153 allocs, 23 frees, 1,447,758 bytes allocated
==21219==
==21219== 7 bytes in 1 blocks are possibly lost in loss record 5 of 54
==21219== at 0x4C2DB8F: malloc (in /usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so)
==21219== by 0x5726489: strdup (strdup.c:42)
==21219== by 0x468FD9: bind_conf_alloc (listener.h:158)
==21219== by 0x468FD9: cfg_parse_listen (cfgparse-listen.c:557)
==21219== by 0x459DF3: readcfgfile (cfgparse.c:2167)
==21219== by 0x5056CD: init (haproxy.c:2021)
==21219== by 0x418232: main (haproxy.c:3121)
==21219==
==21219== 14 bytes in 1 blocks are possibly lost in loss record 9 of 54
==21219== at 0x4C2DB8F: malloc (in /usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so)
==21219== by 0x5726489: strdup (strdup.c:42)
==21219== by 0x468F9B: bind_conf_alloc (listener.h:154)
==21219== by 0x468F9B: cfg_parse_listen (cfgparse-listen.c:557)
==21219== by 0x459DF3: readcfgfile (cfgparse.c:2167)
==21219== by 0x5056CD: init (haproxy.c:2021)
==21219== by 0x418232: main (haproxy.c:3121)
==21219==
==21219== 128 bytes in 1 blocks are possibly lost in loss record 35 of 54
==21219== at 0x4C2FB55: calloc (in /usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so)
==21219== by 0x468F90: bind_conf_alloc (listener.h:152)
==21219== by 0x468F90: cfg_parse_listen (cfgparse-listen.c:557)
==21219== by 0x459DF3: readcfgfile (cfgparse.c:2167)
==21219== by 0x5056CD: init (haproxy.c:2021)
==21219== by 0x418232: main (haproxy.c:3121)
==21219==
==21219== 608 bytes in 1 blocks are possibly lost in loss record 46 of 54
==21219== at 0x4C2FB55: calloc (in /usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so)
==21219== by 0x4B953A: create_listeners (listener.c:576)
==21219== by 0x4578F6: str2listener (cfgparse.c:192)
==21219== by 0x469039: cfg_parse_listen (cfgparse-listen.c:568)
==21219== by 0x459DF3: readcfgfile (cfgparse.c:2167)
==21219== by 0x5056CD: init (haproxy.c:2021)
==21219== by 0x418232: main (haproxy.c:3121)
==21219==
==21219== LEAK SUMMARY:
==21219== definitely lost: 0 bytes in 0 blocks
==21219== indirectly lost: 0 bytes in 0 blocks
==21219== possibly lost: 757 bytes in 4 blocks
==21219== still reachable: 1,435,874 bytes in 126 blocks
==21219== suppressed: 0 bytes in 0 blocks
==21219== Reachable blocks (those to which a pointer was found) are not shown.
==21219== To see them, rerun with: --leak-check=full --show-leak-kinds=all
==21219==
==21219== For counts of detected and suppressed errors, rerun with: -v
==21219== ERROR SUMMARY: 4 errors from 4 contexts (suppressed: 0 from 0)
Re-running the same command with the patch applied will not report any
losses any more:
$ valgrind --leak-check=full ./haproxy -c -f ./example.cfg
==22124== Memcheck, a memory error detector
==22124== Copyright (C) 2002-2015, and GNU GPL'd, by Julian Seward et al.
==22124== Using Valgrind-3.11.0 and LibVEX; rerun with -h for copyright info
==22124== Command: ./haproxy -c -f ./example.cfg
==22124==
[WARNING] 165/001503 (22124) : config : missing timeouts for frontend 'foo'.
| While not properly invalid, you will certainly encounter various problems
| with such a configuration. To fix this, please ensure that all following
| timeouts are set to a non-zero value: 'client', 'connect', 'server'.
Warnings were found.
Configuration file is valid
==22124==
==22124== HEAP SUMMARY:
==22124== in use at exit: 313,864 bytes in 82 blocks
==22124== total heap usage: 153 allocs, 71 frees, 1,447,758 bytes allocated
==22124==
==22124== LEAK SUMMARY:
==22124== definitely lost: 0 bytes in 0 blocks
==22124== indirectly lost: 0 bytes in 0 blocks
==22124== possibly lost: 0 bytes in 0 blocks
==22124== still reachable: 313,864 bytes in 82 blocks
==22124== suppressed: 0 bytes in 0 blocks
==22124== Reachable blocks (those to which a pointer was found) are not shown.
==22124== To see them, rerun with: --leak-check=full --show-leak-kinds=all
==22124==
==22124== For counts of detected and suppressed errors, rerun with: -v
==22124== ERROR SUMMARY: 0 errors from 0 contexts (suppressed: 0 from 0)
It might be worth investigating what exactly HAProxy does to lose pointers
to the start of those 4 memory areas and then to be able to still free them
during deinit(). If HAProxy is able to free them, they ideally should be
"still reachable" and not "possibly lost".
This patch fixes all the leftovers from the include cleanup campaign. There
were not that many (~400 entries in ~150 files) but it was definitely worth
doing it as it revealed a few duplicates.
There's no point splitting the file in two since only cfgparse uses the
types defined there. A few call places were updated and cleaned up. All
of them were in C files which register keywords.
There is nothing left in common/ now so this directory must not be used
anymore.
This one was not easy because it was embarking many includes with it,
which other files would automatically find. At least global.h, arg.h
and tools.h were identified. 93 total locations were identified, 8
additional includes had to be added.
In the rare files where it was possible to finalize the sorting of
includes by adjusting only one or two extra lines, it was done. But
all files would need to be rechecked and cleaned up now.
It was the last set of files in types/ and proto/ and these directories
must not be reused anymore.
extern struct dict server_name_dict was moved from the type file to the
main file. A handful of inlined functions were moved at the bottom of
the file. Call places were updated to use server-t.h when relevant, or
to simply drop the entry when not needed.
The files remained mostly unchanged since they were OK. However, half of
the users didn't need to include them, and about as many actually needed
to have it and used to find functions like srv_currently_usable() through
a long chain that broke when moving the file.
This one is particularly difficult to split because it provides all the
functions used to manipulate a proxy state and to retrieve names or IDs
for error reporting, and as such, it was included in 73 files (down to
68 after cleanup). It would deserve a small cleanup though the cut points
are not obvious at the moment given the number of structs involved in
the struct proxy itself.
The current state of the logging is a real mess. The main problem is
that almost all files include log.h just in order to have access to
the alert/warning functions like ha_alert() etc, and don't care about
logs. But log.h also deals with real logging as well as log-format and
depends on stream.h and various other things. As such it forces a few
heavy files like stream.h to be loaded early and to hide missing
dependencies depending where it's loaded. Among the missing ones is
syslog.h which was often automatically included resulting in no less
than 3 users missing it.
Among 76 users, only 5 could be removed, and probably 70 don't need the
full set of dependencies.
A good approach would consist in splitting that file in 3 parts:
- one for error output ("errors" ?).
- one for log_format processing
- and one for actual logging.
It was moved without any change, however many callers didn't need it at
all. This was a consequence of the split of proto_http.c into several
parts that resulted in many locations to still reference it.
Almost no change except moving the cli_kw struct definition after the
defines. Almost all users had both types&proto included, which is not
surprizing since this code is old and it used to be the norm a decade
ago. These places were cleaned.
Initially it looked like this could have been placed into auth.h or
stats.h but it's not the case as it's what makes the link between them
and the HTTP layer. However the file needed to be split in two. Quite
a number of call places were dropped because these were mostly leftovers
from the early days where the stats and cli were packed together.
The files were moved almost as-is, just dropping arg-t and auth-t from
acl-t but keeping arg-t in acl.h. It was useful to revisit the call places
since a handful of files used to continue to include acl.h while they did
not need it at all. Struct stream was only made a forward declaration
since not otherwise needed.
The cfg_peers external declaration was moved to the main file instead
of the type one. A few types were still missing from the proto, causing
warnings in the functions prototypes (proxy, stick_table).
The type file is becoming a mess, half of it is for the proxy protocol,
another good part describes conn_streams and mux ops, it would deserve
being split again. At least it was reordered so that elements are easier
to find, with the PP-stuff left at the end. The MAX_SEND_FD macro was moved
to compat.h as it's said to be the value for Linux.
The TASK_IS_TASKLET() macro was moved to the proto file instead of the
type one. The proto part was a bit reordered to remove a number of ugly
forward declaration of static inline functions. About a tens of C and H
files had their dependency dropped since they were not using anything
from task.h.
global.h was one of the messiest files, it has accumulated tons of
implicit dependencies and declares many globals that make almost all
other file include it. It managed to silence a dependency loop between
server.h and proxy.h by being well placed to pre-define the required
structs, forcing struct proxy and struct server to be forward-declared
in a significant number of files.
It was split in to, one which is the global struct definition and the
few macros and flags, and the rest containing the functions prototypes.
The UNIX_MAX_PATH definition was moved to compat.h.
This one is particularly tricky to move because everyone uses it
and it depends on a lot of other types. For example it cannot include
arg-t.h and must absolutely only rely on forward declarations to avoid
dependency loops between vars -> sample_data -> arg. In order to address
this one, it would be nice to split the sample_data part out of sample.h.
It was moved as-is, except for extern declaration of pattern_reference.
A few C files used to include it but didn't need it anymore after having
been split apart so this was cleaned.
One function prototype makes reference to struct mworker_proc which was
not defined there but in global.h instead. This definition, along with
the PROC_O_* fields were moved to mworker-t.h instead.
The STATS_DEFAULT_REALM and STATS_DEFAULT_URI were moved to defaults.h.
It was required to include types/pattern.h and types/sample.h since they
are mentioned in function prototypes.
It would be wise to merge this with uri_auth.h later.
A few includes were missing in each file. A definition of
struct polled_mask was moved to fd-t.h. The MAX_POLLERS macro was
moved to defaults.h
Stdio used to be silently inherited from whatever path but it's needed
for list_pollers() which takes a FILE* and which can thus not be
forward-declared.
And also rename standard.c to tools.c. The original split between
tools.h and standard.h dates from version 1.3-dev and was mostly an
accident. This patch moves the files back to what they were expected
to be, and takes care of not changing anything else. However this
time tools.h was split between functions and types, because it contains
a small number of commonly used macros and structures (e.g. name_desc)
which in turn cause the massive list of includes of tools.h to conflict
with the callers.
They remain the ugliest files of the whole project and definitely need
to be cleaned and split apart. A few types are defined there only for
functions provided there, and some parts are even OS-specific and should
move somewhere else, such as the symbol resolution code.
The protocol.h files are pretty low in the dependency and (sadly) used
by some files from common/. Almost nothing was changed except lifting a
few comments.
Regex are essentially included for myregex_t but it turns out that
several of the C files didn't include it directly, relying on the
one included by their own .h. This has been cleanly addressed so
that only the type is included by H files which need it, and adding
the missing includes for the other ones.
The type was moved out as it's used by standard.h for netns_entry.
Instead of just being a forward declaration when not used, it's an
empty struct, which makes gdb happier (the resulting stripped executable
is the same).
The pretty confusing "buffer.h" was in fact not the place to look for
the definition of "struct buffer" but the one responsible for dynamic
buffer allocation. As such it defines the struct buffer_wait and the
few functions to allocate a buffer or wait for one.
This patch moves it renaming it to dynbuf.h. The type definition was
moved to its own file since it's included in a number of other structs.
Doing this cleanup revealed that a significant number of files used to
rely on this one to inherit struct buffer through it but didn't need
anything from this file at all.
This moves types/activity.h to haproxy/activity-t.h and
proto/activity.h to haproxy/activity.h.
The macros defining the bit field values for the profiling variable
were moved to the type file to be more future-proof.
Now the file is ready to be stored into its final destination. A few
minor reorderings were performed to keep the file properly organized,
making the various sections more visible (cache & lockless).
In addition and to stay consistent, memory.c was renamed to pool.c.
