systemd: simplify parser and fix infinite loop #24825
Conversation
openshift-ci
bot
added
release-note
approved
giuseppe
force-pushed
the
simplify-systemd-parser
branch
from
6435f3a to
f88a2fe
Compare
giuseppe
force-pushed
the
simplify-systemd-parser
branch
2 times, most recently
from
d7b6d7c to
7b72ddb
Compare
There was a problem hiding this comment.
I think this solution is better not only by removing the extra loop, but also more easy to read than the code that kept looking at the entire file as a single string.
I did have some comments.
In addition, do you mind adding an e2e test that mimic the issue that started this discussion?
pkg/systemd/parser/unitfile.go
Outdated
| var line string | ||
| line, data = nextLine(data, 0) | ||
| for lineNr, line := range lines { | ||
| p.lineNr = lineNr + 1 |
There was a problem hiding this comment.
Since you're already changing the assignment to p.lineNr, I can see that it's only used for a print in parseLine. Maybe it's just simpler to pass the line as a parameter instead of maintaining it all the time
giuseppe
force-pushed
the
simplify-systemd-parser
branch
2 times, most recently
from
e01e43a to
bb61a1b
Compare
openshift-ci
bot
added
the
do-not-merge/work-in-progress
giuseppe
force-pushed
the
simplify-systemd-parser
branch
from
bb61a1b to
036e01b
Compare
openshift-ci
bot
removed
the
do-not-merge/work-in-progress
giuseppe
force-pushed
the
simplify-systemd-parser
branch
from
036e01b to
a26a2ff
Compare
|
@ygalblum thanks for the review. Addressed the comments and pushed a new version |
This commit simplifies the systemd parser logic, and it solves an infinite loop when using a continuation line. Closes: containers#24810 Signed-off-by: Giuseppe Scrivano <gscrivan@redhat.com>
giuseppe
force-pushed
the
simplify-systemd-parser
branch
from
a26a2ff to
64e94ef
Compare
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: giuseppe, ygalblum The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
There was a problem hiding this comment.
AFAIK none of the files are ever parsed until you explicitly list them in the quadlet_test.go file as test case so this is a NOP I think.
There was a problem hiding this comment.
You're right, sorry I missed it.
You need to add it to the test list in quadlet_test.go. It should go in the first long table (the ones expected to succeed without dependecies)
There was a problem hiding this comment.
follow-up PR here: #24841
|
/lgtm |
This commit simplifies the systemd parser logic, and it solves an infinite loop when using a continuation line.
Closes: #24810
Does this PR introduce a user-facing change?