dedup backend: Fix leaking filedescriptor by sebsura · Pull Request #2548 · bareos/bareos

sebsura · 2026-02-19T07:16:17Z

Thank you for contributing to the Bareos Project!

Please check

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

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

Helpful documentation and best practices

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

Make sure you check/merge the PR using devtools/pr-tool to have some simple automated checks run and a proper changelog record added.

General

Is the PR title usable as CHANGELOG entry?
Purpose of the PR is understood
Commit descriptions are understandable and well formatted
Required backport PRs have been created
Correct milestone is set

Source code quality

Source code changes are understandable
Variable and function names are meaningful
Code comments are correct (logically and spelling)
Required documentation changes are present and part of the PR

coderabbitai · 2026-02-19T07:16:42Z

Caution

Review failed

The pull request is closed.

ℹ️ Recent review info

⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: 8d2e9a0d-c5b5-4958-a5f0-1e54afc962c5

📥 Commits

Reviewing files that changed from the base of the PR and between f1713de and b0704c6.

📒 Files selected for processing (4)

CHANGELOG.md
core/src/stored/backends/dedupable/util.h
core/src/stored/backends/dedupable/volume.cc
core/src/stored/backends/dedupable/volume.h

📝 Walkthrough

Walkthrough

Wrap raw directory file descriptors in raii_fd: constructors, move ops, fileno() and release() made noexcept/const, operator bool() made explicit and noexcept, and fd moved to private. volume stores dird as an raii_fd and uses dird.fileno() at call sites.

Changes

Cohort / File(s)	Summary
RAII wrapper `core/src/stored/backends/dedupable/util.h`	Made `raii_fd` constructor `explicit` and `noexcept`; move ctor and move assignment marked `noexcept`; `fileno()` → `int fileno() const noexcept`; `release()` marked `noexcept`; `operator bool()` → `explicit operator bool() const noexcept`; moved `fd` to `private`; updated copyright year.
Volume implementation & header `core/src/stored/backends/dedupable/volume.h`, `core/src/stored/backends/dedupable/volume.cc`	Replaced raw `int` dir-fd with `raii_fd dird`; `OpenRelative` now returns `raii_fd`; call sites use `dird.fileno()` and the explicit `operator bool()`; updated open/openat usages and error handling to work with wrapper; bumped copyright years.
Changelog `CHANGELOG.md`	Added unreleased changelog entry referencing the dedup backend FD leak fix (PR `#2548`).

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

Possibly related PRs

dedup backend: Fix leaking filedescriptor #2548 — Modifies the same dedupable backend to replace raw int file descriptors with raii_fd wrapper and adjust OpenRelative and call sites accordingly.

Suggested reviewers

pstorz

Poem

🐰 I tuck the fds in snug and tight,

Constructors promise no-throw night,
fileno peeks, explicit bool says "here",
Private burrow keeps descriptors near,
Hoppity code, neat and light.

🚥 Pre-merge checks | ✅ 2 | ❌ 1

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 10.00% which is insufficient. The required threshold is 80.00%.	Write docstrings for the functions missing them to satisfy the coverage threshold.

✅ Passed checks (2 passed)

Check name	Status	Explanation
Title check	✅ Passed	The title accurately describes the main objective of the PR - fixing a file descriptor leak in the dedup backend by introducing RAII wrappers to prevent resource leaks.
Description check	✅ Passed	The PR description follows the required template with all major sections present and checklists completed, indicating the author addressed the template requirements.

_{✏️ Tip: You can configure your own custom pre-merge checks in the settings.}

✨ Finishing Touches

🧪 Generate unit tests (beta)

Create PR with unit tests
Post copyable unit tests in a comment

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

coderabbitai

🧹 Nitpick comments (1)

core/src/stored/backends/dedupable/volume.cc (1)

180-187: Minor inconsistency: use !dird.ok() for consistency with other checks.

Line 182 uses dird.fileno() < 0 while all other validity checks in this file use the ok() method (e.g., lines 191, 275, 468, 480, etc.).

Suggested fix for consistency

   dird = open(path, dir_flags);
 
-  if (dird.fileno() < 0) {
+  if (!dird.ok()) {
     std::string errctx = "Cannot open '";
     errctx += path;
     errctx += "'";

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@core/src/stored/backends/dedupable/volume.cc` around lines 180 - 187, The
check after opening the directory uses dird.fileno() < 0 which is inconsistent
with other validity checks in this file; change the condition to use !dird.ok()
(i.e., if (!dird.ok())) when testing the result of open(path, dir_flags) so it
matches the style used elsewhere (see dird, open(path, dir_flags),
dird.fileno(), dird.ok()); keep the existing error construction and throw
behavior intact.

🤖 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/stored/backends/dedupable/volume.cc`:
- Around line 180-187: The check after opening the directory uses dird.fileno()
< 0 which is inconsistent with other validity checks in this file; change the
condition to use !dird.ok() (i.e., if (!dird.ok())) when testing the result of
open(path, dir_flags) so it matches the style used elsewhere (see dird,
open(path, dir_flags), dird.fileno(), dird.ok()); keep the existing error
construction and throw behavior intact.

pstorz

Looks good, please see my comment and let's sync about that.

core/src/stored/backends/dedupable/util.h

pstorz

Looks good! Only the backports are pending now.

This makes it clear that you closing the filedescriptor is already taken care of and reduces suprises.

sebsura linked an issue Feb 19, 2026 that may be closed by this pull request

Dedupable device on NFS appliance fails to recycle purged volume #2393

Closed

coderabbitai bot reviewed Feb 19, 2026

View reviewed changes

sebsura mentioned this pull request Feb 19, 2026

Dedupable device on NFS appliance fails to recycle purged volume #2393

Closed

sebsura force-pushed the dev/ssura/master/fix-fd-leak branch from aa4b444 to 730f04b Compare February 19, 2026 07:20

sebsura self-assigned this Feb 19, 2026

sebsura added requires backport to 23 requires backport to 24 requires backport to 25 labels Feb 19, 2026

This was referenced Feb 20, 2026

dedup backend: Fix leaking filedescriptor #2549

Merged

dedup backend: Fix leaking filedescriptor #2550

Merged

dedup backend: Fix leaking filedescriptor #2551

Merged

arogge requested a review from pstorz February 24, 2026 10:48

pstorz requested changes Mar 2, 2026

View reviewed changes

core/src/stored/backends/dedupable/util.h Outdated Show resolved Hide resolved

sebsura force-pushed the dev/ssura/master/fix-fd-leak branch from ece401b to f1713de Compare March 3, 2026 07:24

sebsura requested a review from pstorz March 3, 2026 07:24

pstorz changed the title ~~Fix leaking filedescriptor on dedup backend~~ dedup backend: Fix leaking filedescriptor Mar 4, 2026

pstorz modified the milestone: 25.0.3 Mar 4, 2026

pstorz approved these changes Mar 4, 2026

View reviewed changes

sebsura and others added 4 commits March 5, 2026 10:23

dedup: fix not closing directory file descriptor

4eb8112

util: add noexcept to raii_fd

c3d6ef0

dedupable: make raii_fd constructor explicit

a7b976d

This makes it clear that you closing the filedescriptor is already taken care of and reduces suprises.

Update CHANGELOG.md

b0704c6

BareosBot force-pushed the dev/ssura/master/fix-fd-leak branch from efbf05c to b0704c6 Compare March 5, 2026 10:23

BareosBot merged commit c638c80 into bareos:master Mar 5, 2026

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

dedup backend: Fix leaking filedescriptor#2548

dedup backend: Fix leaking filedescriptor#2548
BareosBot merged 4 commits intobareos:masterfrom
sebsura:dev/ssura/master/fix-fd-leak

sebsura commented Feb 19, 2026 •

edited

Loading

Uh oh!

coderabbitai bot commented Feb 19, 2026 •

edited

Loading

Review failed

Walkthrough

Changes

Estimated code review effort

Possibly related PRs

Suggested reviewers

Poem

❌ Failed checks (1 warning)

Uh oh!

coderabbitai bot left a comment

Uh oh!

pstorz left a comment

Uh oh!

Uh oh!

pstorz left a comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Conversation

sebsura commented Feb 19, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Thank you for contributing to the Bareos Project!

Please check

Helpful documentation and best practices

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

General

Source code quality

Uh oh!

coderabbitai bot commented Feb 19, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Review failed

Walkthrough

Changes

Estimated code review effort

Possibly related PRs

Suggested reviewers

Poem

❌ Failed checks (1 warning)

Uh oh!

coderabbitai bot left a comment

Choose a reason for hiding this comment

Uh oh!

pstorz left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

pstorz left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

sebsura commented Feb 19, 2026 •

edited

Loading

coderabbitai bot commented Feb 19, 2026 •

edited

Loading