plugins: mariabackup python-mysqlclient ignore invalid utf8#2546
Merged
BareosBot merged 5 commits intobareos:masterfrom Feb 25, 2026
Merged
plugins: mariabackup python-mysqlclient ignore invalid utf8#2546BareosBot merged 5 commits intobareos:masterfrom
BareosBot merged 5 commits intobareos:masterfrom
Conversation
Contributor
|
Caution Review failedThe pull request is closed. ℹ️ Recent review infoConfiguration used: Organization UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (3)
📝 WalkthroughWalkthroughBoth MariaDB and Percona XtraBackup FD plugins now connect with Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
Tip Try Coding Plans. Let us write the prompt for your AI agent so you can ship faster (with fewer bugs). Comment |
Contributor
|
Note Docstrings generation - SUCCESS |
coderabbitai bot
added a commit
that referenced
this pull request
Feb 24, 2026
Docstrings generation was requested by @bruno-at-bareos. * #2546 (comment) The following files were modified: * `core/src/plugins/filed/python/mariabackup/bareos-fd-mariabackup.py`
bruno-at-bareos
requested changes
Feb 24, 2026
Contributor
There was a problem hiding this comment.
Hi, Thanks a lot for the proposal.
To be able to validate the PR, I would like to see two small changes. See change proposal in code.
and also our pr-tools check complain about the length of the commit title.
ea0af9d24 bareos-fd-mariabackup: ignore invalid utf8 when retrieving innodb status using MySQLdb: headline too long (86 > 60)
see developer documentation for explanation.
Could you rebase and push force (we only have one commit here)
plugins: mariabackup python-mysqlclient ignore invalid utf8
core/src/plugins/filed/python/mariabackup/bareos-fd-mariabackup.py
Outdated
Show resolved
Hide resolved
This patch fixes situations where retrieving the innodb status using MySQLdb results in non-utf8-decodeable data.
ea0af9d to
4e0678e
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
core/src/plugins/filed/python/mariabackup/bareos-fd-mariabackup.py (1)
313-323:⚠️ Potential issue | 🟡 MinorFix approach is correct; add a
Noneguard before.decode()
use_unicode=Falseensures MySQLdb returns rawbytesfor the Status column, and.decode(errors="ignore")then safely strips non-UTF-8 sequences — this correctly mirrors theget_lsn_by_command()path already in place (line 195).One small gap: if
result[0][2]isNULLin the database (unlikely but technically possible),.decode(errors="ignore")raises anAttributeError. The existing empty-result guard on line 317 doesn't cover this case. Consider a minimal defensive check:🛡️ Proposed guard
- innodb_status = result[0][2].decode(errors="ignore") + raw = result[0][2] + if raw is None: + JobMessage( + M_FATAL, + "SHOW ENGINE INNODB STATUS returned NULL status field", + ) + return bRC_Error + innodb_status = raw.decode(errors="ignore")🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@core/src/plugins/filed/python/mariabackup/bareos-fd-mariabackup.py` around lines 313 - 323, The code fetches the INNODB STATUS into innodb_status by decoding result[0][2] but doesn't guard against NULL (None) in result[0][2], which would raise AttributeError; update the block around the MySQLdb cursor/SHOW ENGINE INNODB STATUS fetch to check if result[0][2] is None before calling .decode (mirror the defensive pattern used in get_lsn_by_command()), and if it is None either treat it as an empty bytes value to decode or emit the same JobMessage and return bRC_Error so the code never calls .decode on None.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@core/src/plugins/filed/python/mariabackup/bareos-fd-mariabackup.py`:
- Line 323: The code calls result[0][2].decode(...) without guarding for SQL
NULLs; if result[0][2] is None this raises AttributeError. Update the assignment
for innodb_status to handle None (e.g., treat None as empty bytes/string) before
calling .decode(errors="ignore") so decoding only happens on a bytes-like value;
locate the innodb_status assignment and change it to check result[0][2] (or use
a short-circuit like using a fallback) and then decode.
---
Outside diff comments:
In `@core/src/plugins/filed/python/mariabackup/bareos-fd-mariabackup.py`:
- Around line 313-323: The code fetches the INNODB STATUS into innodb_status by
decoding result[0][2] but doesn't guard against NULL (None) in result[0][2],
which would raise AttributeError; update the block around the MySQLdb
cursor/SHOW ENGINE INNODB STATUS fetch to check if result[0][2] is None before
calling .decode (mirror the defensive pattern used in get_lsn_by_command()), and
if it is None either treat it as an empty bytes value to decode or emit the same
JobMessage and return bRC_Error so the code never calls .decode on None.
core/src/plugins/filed/python/mariabackup/bareos-fd-mariabackup.py
Outdated
Show resolved
Hide resolved
Contributor
|
Note to myself: port the changes to the percona-xtrabackup plugin too. |
Contributor
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
core/src/plugins/filed/python/mariabackup/bareos-fd-mariabackup.py (1)
317-331:⚠️ Potential issue | 🟡 MinorConnection leak on both early-return paths inside the
tryblock.
conn.close()at Line 331 is only reached on the happy path. Both thelen(result) == 0guard (Line 317–322, pre-existing) and the newraw_status is Noneguard (Lines 324–329) callreturn bRC_Errorwithout closing the connection first. Since each backup job opens a fresh connection, this leaks a handle each time either guard fires.Wrap the connection lifetime in a
try/finally:🔧 Proposed fix using try/finally for guaranteed cleanup
try: - conn = MySQLdb.connect(**self.connect_options, use_unicode=False) - cursor = conn.cursor() - cursor.execute("SHOW ENGINE INNODB STATUS") - result = cursor.fetchall() - if len(result) == 0: - JobMessage( - M_FATAL, - "Could not fetch SHOW ENGINE INNODB STATUS, unprivileged user?", - ) - return bRC_Error - raw_status = result[0][2] - if raw_status is None: - JobMessage( - M_FATAL, - "SHOW ENGINE INNODB STATUS returned a NULL status field", - ) - return bRC_Error - innodb_status = raw_status.decode(errors="ignore") - conn.close() + conn = MySQLdb.connect(**self.connect_options, use_unicode=False) + try: + cursor = conn.cursor() + cursor.execute("SHOW ENGINE INNODB STATUS") + result = cursor.fetchall() + if len(result) == 0: + JobMessage( + M_FATAL, + "Could not fetch SHOW ENGINE INNODB STATUS, unprivileged user?", + ) + return bRC_Error + raw_status = result[0][2] + if raw_status is None: + JobMessage( + M_FATAL, + "SHOW ENGINE INNODB STATUS returned a NULL status field", + ) + return bRC_Error + innodb_status = raw_status.decode(errors="ignore") + finally: + conn.close() except Exception as e:🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@core/src/plugins/filed/python/mariabackup/bareos-fd-mariabackup.py` around lines 317 - 331, The code returns early on two error paths without closing the DB connection (conn), leaking handles; wrap the connection usage in a try/finally so conn.close() always runs (move the existing conn.close() into a finally block) and keep the current error returns that call JobMessage and return bRC_Error inside the try before the finally; reference the existing symbols conn, JobMessage, bRC_Error and the block that sets raw_status/innodb_status to locate where to add the try/finally.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@core/src/plugins/filed/python/mariabackup/bareos-fd-mariabackup.py`:
- Around line 317-331: The code returns early on two error paths without closing
the DB connection (conn), leaking handles; wrap the connection usage in a
try/finally so conn.close() always runs (move the existing conn.close() into a
finally block) and keep the current error returns that call JobMessage and
return bRC_Error inside the try before the finally; reference the existing
symbols conn, JobMessage, bRC_Error and the block that sets
raw_status/innodb_status to locate where to add the try/finally.
The len(result) == 0 check above covers an empty result set, but if the Status column returns NULL (making result[0][2] a Python None), .decode(errors="ignore") will raise an AttributeError that lands in the bare except Exception handler and emits a confusing M_FATAL message. The risk is very low in practice, but it's worth a one-liner guard given the stakes (a M_FATAL during backup): Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
This patch fixes situations where retrieving the innodb status using MySQLdb results in non-utf8-decodeable data, like we fixed in mariabackup plugin. Signed-off-by: Bruno Friedmann <bruno.friedmann@bareos.com>
6f6345a to
685fbe9
Compare
Contributor
|
Caution Failed to replace (edit) comment. This is likely due to insufficient permissions or the comment being deleted. Error details |
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
core/src/plugins/filed/python/percona-xtrabackup/bareos-fd-percona-xtrabackup.py (1)
224-241: LGTM — logic is correct; consider guardingconn.close()withfinallyThe chain
use_unicode=False→ bytes result →None-guard →.decode(errors="ignore")is correct and mirrors the mariabackup fix.bytes.decode()defaults to UTF-8, which is exactly the right encoding to strip invalid bytes fromSHOW ENGINE INNODB STATUSoutput.One pre-existing structural nit: if
cursor.execute()orfetchall()raises,conn.close()is skipped (the outerexceptre-raises asbRC_Errorwithout closing). The connection will eventually be GC'd, but an explicitfinallyis cleaner.♻️ Optional: guard conn.close() with try/finally
- try: - conn = MySQLdb.connect(**self.connect_options, use_unicode=False) - cursor = conn.cursor() - cursor.execute("SHOW ENGINE INNODB STATUS") - result = cursor.fetchall() - if len(result) == 0: - JobMessage( - M_FATAL, - "Could not fetch SHOW ENGINE INNODB STATUS, unprivileged user?", - ) - return bRC_Error - raw_status = result[0][2] - if raw_status is None: - JobMessage( - M_FATAL, - "SHOW ENGINE INNODB STATUS returned a NULL status field", - ) - return bRC_Error - info = raw_status.decode(errors="ignore") - conn.close() - except Exception as e: + conn = None + try: + conn = MySQLdb.connect(**self.connect_options, use_unicode=False) + cursor = conn.cursor() + cursor.execute("SHOW ENGINE INNODB STATUS") + result = cursor.fetchall() + if len(result) == 0: + JobMessage( + M_FATAL, + "Could not fetch SHOW ENGINE INNODB STATUS, unprivileged user?", + ) + return bRC_Error + raw_status = result[0][2] + if raw_status is None: + JobMessage( + M_FATAL, + "SHOW ENGINE INNODB STATUS returned a NULL status field", + ) + return bRC_Error + info = raw_status.decode(errors="ignore") + except Exception as e: JobMessage( M_FATAL, "Could not get LSN, Error: %s" % e, ) return bRC_Error + finally: + if conn is not None: + conn.close()🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@core/src/plugins/filed/python/percona-xtrabackup/bareos-fd-percona-xtrabackup.py` around lines 224 - 241, The DB connection is not closed if cursor.execute() or cursor.fetchall() raises; wrap the work that uses conn and cursor in a try/finally so conn.close() always runs. Specifically, after creating conn = MySQLdb.connect(**self.connect_options, use_unicode=False) and cursor = conn.cursor(), move the execute/fetchall/result handling into a try block and call conn.close() (and cursor.close() if desired) in the finally block; keep existing JobMessage and bRC_Error returns unchanged and ensure the finally runs before any returns.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In
`@core/src/plugins/filed/python/percona-xtrabackup/bareos-fd-percona-xtrabackup.py`:
- Around line 224-241: The DB connection is not closed if cursor.execute() or
cursor.fetchall() raises; wrap the work that uses conn and cursor in a
try/finally so conn.close() always runs. Specifically, after creating conn =
MySQLdb.connect(**self.connect_options, use_unicode=False) and cursor =
conn.cursor(), move the execute/fetchall/result handling into a try block and
call conn.close() (and cursor.close() if desired) in the finally block; keep
existing JobMessage and bRC_Error returns unchanged and ensure the finally runs
before any returns.
Contributor
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
core/src/plugins/filed/python/percona-xtrabackup/bareos-fd-percona-xtrabackup.py (1)
241-245:⚠️ Potential issue | 🟠 MajorGuard
last_lsnwhen no parseable LSN line is found.If Lines 243-245 never match,
last_lsnis unset and Line 280 can raiseUnboundLocalError.🐛 Proposed fix
- info = raw_status.decode(errors="ignore") + info = raw_status.decode(errors="ignore") conn.close() + last_lsn = None for line in info.split("\n"): if line.startswith("Log sequence number"): last_lsn = int(line.split(" ")[-1]) + if last_lsn is None: + JobMessage( + M_FATAL, + "Could not parse Log sequence number from INNODB status", + ) + return bRC_ErrorAlso applies to: 280-280
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@core/src/plugins/filed/python/percona-xtrabackup/bareos-fd-percona-xtrabackup.py` around lines 241 - 245, The loop parsing raw_status may never set last_lsn, causing an UnboundLocalError later when it’s used; initialize last_lsn (e.g., to None or 0) before the for loop that processes info (where raw_status.decode and conn.close() are called) and then check/guard its value before it’s consumed (the later usage around last_lsn at line ~280) — if last_lsn is None/invalid, raise a clear error or handle the missing LSN path accordingly in the surrounding function.
🧹 Nitpick comments (2)
core/src/plugins/filed/python/mariabackup/bareos-fd-mariabackup.py (1)
313-331: Close the MySQL connection on all exit paths.On Line 317 and Line 324, the function can return before Line 331, leaving the DB connection open.
♻️ Proposed fix
try: conn = MySQLdb.connect(**self.connect_options, use_unicode=False) - cursor = conn.cursor() - cursor.execute("SHOW ENGINE INNODB STATUS") - result = cursor.fetchall() - if len(result) == 0: - JobMessage( - M_FATAL, - "Could not fetch SHOW ENGINE INNODB STATUS, unprivileged user?", - ) - return bRC_Error - raw_status = result[0][2] - if raw_status is None: - JobMessage( - M_FATAL, - "SHOW ENGINE INNODB STATUS returned a NULL status field", - ) - return bRC_Error - innodb_status = raw_status.decode(errors="ignore") - conn.close() + try: + cursor = conn.cursor() + cursor.execute("SHOW ENGINE INNODB STATUS") + result = cursor.fetchall() + if len(result) == 0: + JobMessage( + M_FATAL, + "Could not fetch SHOW ENGINE INNODB STATUS, unprivileged user?", + ) + return bRC_Error + raw_status = result[0][2] + if raw_status is None: + JobMessage( + M_FATAL, + "SHOW ENGINE INNODB STATUS returned a NULL status field", + ) + return bRC_Error + innodb_status = raw_status.decode(errors="ignore") + finally: + conn.close() except Exception as e:🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@core/src/plugins/filed/python/mariabackup/bareos-fd-mariabackup.py` around lines 313 - 331, The MySQL connection opened via MySQLdb.connect (stored in conn) may be left open when early returns happen after checking result length or raw_status; ensure conn is always closed by wrapping the connect/cursor/execute block in a try/finally (or use a context manager) and call conn.close() in the finally block, or explicitly close conn before each return in the code around the SHOW ENGINE INNODB STATUS logic (references: MySQLdb.connect, conn, cursor, JobMessage, innodb_status).core/src/plugins/filed/python/percona-xtrabackup/bareos-fd-percona-xtrabackup.py (1)
224-242: Ensure DB connection is closed on error returns.Returns on Line 228 and Line 235/240 bypass
conn.close()on Line 242.♻️ Proposed fix
if hasMySQLdbModule: try: conn = MySQLdb.connect(**self.connect_options, use_unicode=False) - cursor = conn.cursor() - cursor.execute("SHOW ENGINE INNODB STATUS") - result = cursor.fetchall() - if len(result) == 0: - JobMessage( - M_FATAL, - "Could not fetch SHOW ENGINE INNODB STATUS, unprivileged user?", - ) - return bRC_Error - raw_status = result[0][2] - if raw_status is None: - JobMessage( - M_FATAL, - "SHOW ENGINE INNODB STATUS returned a NULL status field", - ) - return bRC_Error - info = raw_status.decode(errors="ignore") - conn.close() + try: + cursor = conn.cursor() + cursor.execute("SHOW ENGINE INNODB STATUS") + result = cursor.fetchall() + if len(result) == 0: + JobMessage( + M_FATAL, + "Could not fetch SHOW ENGINE INNODB STATUS, unprivileged user?", + ) + return bRC_Error + raw_status = result[0][2] + if raw_status is None: + JobMessage( + M_FATAL, + "SHOW ENGINE INNODB STATUS returned a NULL status field", + ) + return bRC_Error + info = raw_status.decode(errors="ignore") + finally: + conn.close() for line in info.split("\n"):🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@core/src/plugins/filed/python/percona-xtrabackup/bareos-fd-percona-xtrabackup.py` around lines 224 - 242, The code opens a DB connection via MySQLdb.connect into conn and may return bRC_Error on missing results or NULL raw_status without closing conn; wrap the connection/cursor usage in a try/finally (or context manager) around the block using conn/cursor so conn.close() always runs, or explicitly call conn.close() immediately before each early return in the section that queries "SHOW ENGINE INNODB STATUS" (references: MySQLdb.connect, conn, cursor, cursor.execute, result/raw_status, JobMessage, bRC_Error, info). Ensure conn.close() is executed even when JobMessage is invoked and bRC_Error is returned.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In
`@core/src/plugins/filed/python/percona-xtrabackup/bareos-fd-percona-xtrabackup.py`:
- Around line 241-245: The loop parsing raw_status may never set last_lsn,
causing an UnboundLocalError later when it’s used; initialize last_lsn (e.g., to
None or 0) before the for loop that processes info (where raw_status.decode and
conn.close() are called) and then check/guard its value before it’s consumed
(the later usage around last_lsn at line ~280) — if last_lsn is None/invalid,
raise a clear error or handle the missing LSN path accordingly in the
surrounding function.
---
Nitpick comments:
In `@core/src/plugins/filed/python/mariabackup/bareos-fd-mariabackup.py`:
- Around line 313-331: The MySQL connection opened via MySQLdb.connect (stored
in conn) may be left open when early returns happen after checking result length
or raw_status; ensure conn is always closed by wrapping the
connect/cursor/execute block in a try/finally (or use a context manager) and
call conn.close() in the finally block, or explicitly close conn before each
return in the code around the SHOW ENGINE INNODB STATUS logic (references:
MySQLdb.connect, conn, cursor, JobMessage, innodb_status).
In
`@core/src/plugins/filed/python/percona-xtrabackup/bareos-fd-percona-xtrabackup.py`:
- Around line 224-242: The code opens a DB connection via MySQLdb.connect into
conn and may return bRC_Error on missing results or NULL raw_status without
closing conn; wrap the connection/cursor usage in a try/finally (or context
manager) around the block using conn/cursor so conn.close() always runs, or
explicitly call conn.close() immediately before each early return in the section
that queries "SHOW ENGINE INNODB STATUS" (references: MySQLdb.connect, conn,
cursor, cursor.execute, result/raw_status, JobMessage, bRC_Error, info). Ensure
conn.close() is executed even when JobMessage is invoked and bRC_Error is
returned.
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
core/src/plugins/filed/python/mariabackup/bareos-fd-mariabackup.pycore/src/plugins/filed/python/percona-xtrabackup/bareos-fd-percona-xtrabackup.py
To avoid any connection leak on both early-return paths inside the try block, we enclose them in a try / finally block. Signed-off-by: Bruno Friedmann <bruno.friedmann@bareos.com>
7 tasks
bruno-at-bareos
approved these changes
Feb 25, 2026
Contributor
bruno-at-bareos
left a comment
There was a problem hiding this comment.
Thanks for the whole improvement.
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
3 participants
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.
Hi.
I found that
bareos-fd-mariabackupfailed to get the innodb status from time to time, asshow engine innodb statusreturned a not utf-8 conform output. To circumvent this, I madedecode()ignore non-decodeable chars (see #2527) and everything was fine.Now I installed the
python-mysqlclientlibrary whichbareos-fd-mariabackupmakes use of. And the backup fails again, this time atcursor.execute("SHOW ENGINE INNODB STATUS"). Reason enough to take a look at the database.First, the command outputs something like
The table in question comes from icingadb, see schema.sql.
So long story short, if there are events (like deadlocks) on tables with fields of type
binary,show engine innodb statusmay fail to decode, either using the MySQLdb library orcheck_output().This PR shows a possible workaround by deactivating unicode when connecting via MySQLdb and decoding it afterwards (again, using
errors='ignore').Comments welcome.
Thank you for contributing to the Bareos Project!
Please check
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)
Make sure you check/merge the PR using
devtools/pr-toolto have some simple automated checks run and a proper changelog record added.General
Source code quality
Tests