Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

agent: Fix is_signal_handled failing parsing str to u64 #4252

Merged
merged 1 commit into from May 17, 2022
Merged

agent: Fix is_signal_handled failing parsing str to u64 #4252

merged 1 commit into from May 17, 2022

Conversation

Champ-Goblem
Copy link
Contributor

@Champ-Goblem Champ-Goblem commented May 13, 2022 •
edited

In the is_signal_handled function, when parsing the hex string returned
from /proc/<pid>/status the space/tab character after the colon
is not removed.

This patch trims the result of SigCgt so that
all whitespace characters are removed. It also extends the existing
test cases to check for this scenario.

Fixes: #4250
Signed-off-by: Champ-Goblem cameron@northflank.com

@katacontainersbot katacontainersbot added the size/tiny Smallest and simplest task label May 13, 2022
@fidencio
Copy link
Member

/test

@egernst
Copy link
Member

egernst commented May 13, 2022 •
edited

Good find @Champ-Goblem -- is it feasible to add some unit tests to exercise this? I realize the existing organization of the code may make this a bit painful. What do you think?

egernst
egernst reviewed May 13, 2022
View reviewed changes
src/agent/src/rpc.rs Outdated Show resolved Hide resolved
fidencio
fidencio reviewed May 13, 2022
View reviewed changes
src/agent/src/rpc.rs Outdated Show resolved Hide resolved
cmaf
cmaf reviewed May 14, 2022
View reviewed changes
src/agent/src/rpc.rs Outdated Show resolved Hide resolved
@katacontainersbot katacontainersbot added size/small Small and simple task and removed size/tiny Smallest and simplest task labels May 16, 2022
@Champ-Goblem
Copy link
Contributor Author

@egernst I see there already exists some tests for this function test_is_signal_handled, it doesnt look like it has any cases to cover a whitespace so I have extended this to include some

fidencio
fidencio approved these changes May 16, 2022
View reviewed changes
Copy link
Member

@fidencio fidencio left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm, thanks @Champ-Goblem!

@fidencio
Copy link
Member

/test

@fidencio
Copy link
Member

Hmm. CIs are failing to find the C version of virtiofsd and I think it would be easily solved with a rebase.

@fidencio fidencio force-pushed the patch/fix-is-signal-handled branch from 10f43e9 to a0e2d26 Compare May 16, 2022 18:32
@fidencio
Copy link
Member

Okay, I was able to rebase it myself.
Let me try again ...

/test

@Champ-Goblem @fidencio
agent: Fix is_signal_handled failing parsing str to u64
4b437d9 
In the is_signal_handled function, when parsing the hex string returned
from `/proc/<pid>/status` the space/tab character after the colon
is not removed.

This patch trims the result of SigCgt so that
all whitespace characters are removed. It also extends the existing
test cases to check for this scenario.

Fixes: kata-containers#4250
Signed-off-by: Champ-Goblem <cameron@northflank.com>
@fidencio fidencio force-pushed the patch/fix-is-signal-handled branch from a0e2d26 to 4b437d9 Compare May 16, 2022 18:34
@fidencio
Copy link
Member

/test

As I squashed both versions.

@fidencio
Copy link
Member

Ci is breaking due to kata-containers/tests#4791

kata-containers/tests#4792 is a possible fix.

@fidencio
Copy link
Member

/test

@fidencio
Copy link
Member

/retest-power due to the multistrap repo being unavaiable.

@fidencio
Copy link
Member 

time="2022-05-17T07:47:28-04:00" level=error msg="unable to delete test" error="write vsock host(2):1562314465->vm(256215773):1024: cannot allocate memory: unknown"
ctr: write vsock host(2):1562314465->vm(256215773):1024: cannot allocate memory: unknown
Ending hypervisor stability test
[hypervisor_stability_kill_test.sh:233] INFO: Wait until the containers gets removed
ctr: write vsock host(2):1562314465->vm(256215773):1024: cannot allocate memory: unknown
make: *** [Makefile:73: stability] Error 1

Power CI failure is unrelated.

@fidencio
Copy link
Member

/retest-power

@cmaf cmaf merged commit 04bd8f1 into kata-containers:main May 17, 2022
@norbjd norbjd mentioned this pull request Jun 3, 2022
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@egernst egernst egernst left review comments

@fidencio fidencio fidencio approved these changes

@cmaf cmaf cmaf approved these changes

Assignees
No one assigned
Labels
size/small Small and simple task
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Agent: is_signal_handled does not remove space before the hex string causing conversion to u64 to fail
5 participants
@Champ-Goblem @fidencio @egernst @cmaf @katacontainersbot