glusterd: validate debug.{trace,error-gen,delay-gen} values - #4825
Closed
ThalesBarretto wants to merge 2 commits into
Closed
ThalesBarretto wants to merge 2 commits into
ThalesBarretto wants to merge 2 commits into
Conversation
get_server_xlator() walks server_graph_table[] to tell whether a value
names a brick-graph xlator, but the comparison is inverted: strcmp() is
0 on a match, so "if (strcmp(xlator, dbg_key)) return GF_XLATOR_SERVER"
returns GF_XLATOR_SERVER on the first *non*-matching entry, i.e. for
essentially any input. Its only caller is the guard in
debugxl_option_handler() that is meant to skip a debug.{trace,error-gen,
delay-gen} value which is not a valid xlator name; the inverted result
makes that guard dead, so an invalid target for one debug key is not
skipped when another debug key triggers the debug-xlator pass, and its
debug xlator is inserted above the wrong translator.
The function was introduced correct in 99f18fa (2012) as a series of
explicit strcmp(...) == 0 checks; the later table-driven rewrite dropped
the == 0. Restore it.
Updates: gluster#4824
Signed-off-by: Thales Antunes de Oliveira Barretto <thales.barretto.git@gmail.com>
These keys take the name of the translator to insert the debug xlator above (a brick-graph xlator such as "posix" or "locks", or "client"), or "off" to disable -- not a boolean. They had no validate_fn, so "volume set <vol> debug.delay-gen on" (or any non-xlator string) returned "volume set: success", stored the bogus value, and silently did nothing: the graph builder (check_and_add_debug_xl / debugxl_option_handler) only inserts the debug xlator when the value equals a graph xlator name, so a value that is not one is quietly ignored. Because the keys are NO_DOC, "volume set help" gives the operator no syntax hint either. Add a validate_fn that accepts a valid brick-graph xlator name (via the now-correct get_server_xlator()), "client", or "off", and rejects anything else with a message naming the expected form. Requires the get_server_xlator() fix in the previous commit; without it every value is treated as a valid server xlator and the validator would be a no-op. Fixes: gluster#4824 Signed-off-by: Thales Antunes de Oliveira Barretto <thales.barretto.git@gmail.com>
ThalesBarretto
marked this pull request as draft
September 22, 2026 10:02
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
glusterd: validate debug.{trace,error-gen,delay-gen} values
Problem
debug.trace,debug.error-genanddebug.delay-gentake the name of the translator to insert the debug xlatorabove (a brick-graph xlator such as
posix/locks, orclient), oroff— not a boolean. They have novalidate_fn, sogluster volume set VOL debug.delay-gen onreturnsvolume set: success, storeson, and doesnothing: the graph builder only inserts the debug xlator when the value equals a translator name, so any non-xlator
value is silently ignored. The keys are
NO_DOC, so the operator gets no hint fromvolume set helpeither.Behind the same path,
get_server_xlator()has an invertedstrcmp— it returnsGF_XLATOR_SERVERon the firstnon-matching entry, so it reports SERVER for almost any input and the guard in
debugxl_option_handler()that ismeant to skip an invalid debug target is dead. It was introduced correct in 99f18fa (2012) as explicit
strcmp(...) == 0checks; the later table-driven rewrite dropped the== 0.Fix
Two commits:
get_server_xlator()'s comparison to== 0, so it correctly reports whether a value names a brick-graphxlator. Its only caller is the debug-xlator guard.
validate_fnto the three debug keys that accepts a valid brick-graph xlator name (via the now-correctget_server_xlator()),client, oroff, and rejects anything else with a message naming the expected form.Commit 1 is a prerequisite: without it every value is treated as a valid server xlator and the validator would be a
no-op.
No behaviour change for the valid, documented usage (
debug.delay-gen posix,debug.error-gen client, …), which allin-tree tests use.
Test
tests/bugs/glusterd/debug-xlator-value-validation.t: assertsdebug.delay-gen on/bogusvalue,debug.trace enable,debug.error-gen 1are now rejected and not stored; thatdebug.delay-gen posix,debug.trace client,debug.error-gen lockssucceed and that delay-gen actually appears in the brick volfile; and thatoffis accepted.Fails on unpatched devel (the invalid sets succeed), passes with this change.
Fixes: #4824