Skip to content

fix: use fgets return value as loop condition in exec()#1211

Open
xq9mend wants to merge 1 commit into
sonic-net:masterfrom
xq9mend:fix/exec-feof-loop-condition
Open

fix: use fgets return value as loop condition in exec()#1211
xq9mend wants to merge 1 commit into
sonic-net:masterfrom
xq9mend:fix/exec-feof-loop-condition

Conversation

@xq9mend

@xq9mend xq9mend commented Jun 19, 2026

Copy link
Copy Markdown

What

Fix incorrect loop condition in exec() in common/exec.cpp.

Why

Using !feof(pipe) as a loop condition has two problems:

  1. feof() only becomes true after a read has already hit EOF, so the loop always executes one extra iteration where fgets() returns NULL.
  2. feof() does not detect I/O errors (ferror). On a read error, fgets() returns NULL but feof() remains false, causing an infinite loop.

How

Drive the loop directly on the fgets() return value. This handles both EOF and I/O errors correctly with a single condition, and eliminates the redundant inner if guard.

// Before
while (!feof(pipe))
{
    if (fgets(buffer.data(), buffsz, pipe) != NULL)
    {
        stdout += buffer.data();
    }
}

// After
while (fgets(buffer.data(), buffsz, pipe) != NULL)
{
    stdout += buffer.data();
}

@mssonicbld

Copy link
Copy Markdown
Collaborator

/azp run

@azure-pipelines

Copy link
Copy Markdown
Azure Pipelines successfully started running 1 pipeline(s).

@xq9mend
xq9mend force-pushed the fix/exec-feof-loop-condition branch from de7afa8 to 3f5f5d3 Compare June 20, 2026 07:03
@xq9mend

xq9mend commented Jun 20, 2026

Copy link
Copy Markdown
Author

/azpw run Azure.sonic-swss-common

@mssonicbld

Copy link
Copy Markdown
Collaborator

⚠️ Notice: /azpw run only runs failed jobs now. If you want to trigger a whole pipline run, please rebase your branch or close and reopen the PR.
💡 Tip: You can also use /azpw retry to retry failed jobs directly.

Retrying failed(or canceled) jobs...

@mssonicbld

Copy link
Copy Markdown
Collaborator

/azp run

@mssonicbld

Copy link
Copy Markdown
Collaborator

No Azure DevOps builds found for #1211.

@azure-pipelines

Copy link
Copy Markdown
Azure Pipelines successfully started running 1 pipeline(s).

@mssonicbld

Copy link
Copy Markdown
Collaborator

Hi, there are workflow run(s) waiting for approval, you may be first-time contributor. I will notify maintainers to help approve once PR is approved. Thanks!

---Powered by SONiC BuildBot

@xq9mend

xq9mend commented Jun 20, 2026

Copy link
Copy Markdown
Author

/azpw run Azure.sonic-swss-common

@mssonicbld

Copy link
Copy Markdown
Collaborator

⚠️ Notice: /azpw run only runs failed jobs now. If you want to trigger a whole pipline run, please rebase your branch or close and reopen the PR.
💡 Tip: You can also use /azpw retry to retry failed jobs directly.

Retrying failed(or canceled) jobs...

@mssonicbld

Copy link
Copy Markdown
Collaborator

Retrying failed(or canceled) stages in build 1144007:

✅Stage Test:

  • Job vstest: retried.

@xq9mend

xq9mend commented Jun 21, 2026

Copy link
Copy Markdown
Author

/azpw run Azure.sonic-swss-common

@mssonicbld

Copy link
Copy Markdown
Collaborator

⚠️ Notice: /azpw run only runs failed jobs now. If you want to trigger a whole pipline run, please rebase your branch or close and reopen the PR.
💡 Tip: You can also use /azpw retry to retry failed jobs directly.

Retrying failed(or canceled) jobs...

@mssonicbld

Copy link
Copy Markdown
Collaborator

Retrying failed(or canceled) stages in build 1144007:

✅Stage Test:

  • Job vstest: retried.

Using !feof() as a loop condition is a classic bug: feof() only becomes
true after a read has already hit EOF, causing one extra iteration where
fgets() returns NULL. It also does not handle I/O errors (ferror), which
would cause an infinite loop.

Fix: drive the loop directly on the fgets() return value, which handles
both EOF and error correctly in a single condition.

Signed-off-by: xq9mend <xq9mend@users.noreply.github.com>
@xq9mend
xq9mend force-pushed the fix/exec-feof-loop-condition branch from 3f5f5d3 to 2aac3f5 Compare June 21, 2026 13:41
@mssonicbld

Copy link
Copy Markdown
Collaborator

/azp run

@azure-pipelines

Copy link
Copy Markdown
Azure Pipelines successfully started running 1 pipeline(s).

@xq9mend

xq9mend commented Jun 21, 2026

Copy link
Copy Markdown
Author

The Test vstest failure is a systemic CI issue unrelated to this PR. The p4rt test group in sonic-swss/tests/p4rt/ is failing across all sonic-swss-common PRs (exit code 123, zero tests passing) due to a mismatch between the latest sonic-swss master test code and the pre-built docker-sonic-vs image. This affects every open sonic-swss-common PR right now — see #1211, #1212, #1213, #1214 all showing the same failure.

The code change in this PR is correct and unrelated to p4rt.

@xq9mend

xq9mend commented Jul 4, 2026

Copy link
Copy Markdown
Author

/azp run Azure.sonic-swss-common

@azure-pipelines

Copy link
Copy Markdown
Commenter does not have sufficient privileges for PR 1211 in repo sonic-net/sonic-swss-common

@xq9mend

xq9mend commented Jul 21, 2026

Copy link
Copy Markdown
Author

/azp run Azure.sonic-swss-common

@azure-pipelines

Copy link
Copy Markdown
Commenter does not have sufficient privileges for PR 1211 in repo sonic-net/sonic-swss-common

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants