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

Fix conditional order for setting net device param #360

Merged
merged 1 commit into from
Jun 22, 2021
Merged

Fix conditional order for setting net device param #360

merged 1 commit into from
Jun 22, 2021

Conversation

yanirq
Copy link
Contributor

@yanirq yanirq commented Jun 20, 2021 •
edited

The conditional for setting a net device parameter has
a faulty order where it can potentially try to first cast a string to int
in a wrong manner and only then check if the value is a specific integer value.

current order:

  1. check if a net device has a combined channel containing 0
  2. check if a net device has a combined channel containing 'n/a'

fixed order:
robust check to see if device parameter is either 0 or 'n/a'

Signed-off-by: Yanir Quinn yquinn@redhat.com

@yanirq
Copy link
Contributor Author

yanirq commented Jun 20, 2021

/cc @yarda

CZerta
CZerta approved these changes Jun 21, 2021
View reviewed changes
Copy link
Contributor

@CZerta CZerta left a comment

Choose a reason for hiding this comment

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

Seems ok, definitely reasonable

jmencak added a commit to jmencak/cluster-node-tuning-operator that referenced this pull request Jun 21, 2021
@jmencak
Fix conditional order for setting net device param.
97a3194 
The conditional for setting a net device parameter in TuneD has a wrong
order where it can potentially try to first cast a string to int in a
wrong manner and only then check if the value is a specific integer
value.

Upstream TuneD fix: redhat-performance/tuned#360

Resolves rhbz#1974277.
@jmencak jmencak mentioned this pull request Jun 21, 2021
Merged
yarda
yarda requested changes Jun 21, 2021
View reviewed changes
Copy link
Contributor

@yarda yarda left a comment

Choose a reason for hiding this comment

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

Although unlikely it could still fail with traceback if there is different string returned than 'n/a'. Could you rewrite it to something more robust and more simple like e.g.:

if context == "channels" and str(dev_params[next(iter(d))]) in ["n/a", "0"]:

@yarda
Copy link
Contributor

yarda commented Jun 22, 2021

@yanirq, could you please merge the commits to one and force push?

@yanirq
Fix conditional for setting net device param
f7e7269 
The conditional for setting a net device parameter has
a faulty order where it can potentially try to first cast a string to int
in a wrong manner and only then check if the value is a specific integer value.

current order:
1. check if a net device has a combined channel containing 0
2. check if a net device has a combined channel containing 'n/a'

fixed order:
robust check to see if device parameter is either 0 or 'n/a'

Signed-off-by: Yanir Quinn <yquinn@redhat.com>
@yanirq yanirq requested a review from yarda June 22, 2021 07:56
yarda
yarda approved these changes Jun 22, 2021
View reviewed changes
Copy link
Contributor

@yarda yarda 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.

@yarda yarda merged commit 5fbc59c into redhat-performance:master Jun 22, 2021
jmencak added a commit to jmencak/cluster-node-tuning-operator that referenced this pull request Jun 22, 2021
@jmencak
Fix conditional order for setting net device param.
637c5a4 
The conditional for setting a net device parameter in TuneD has a wrong
order where it can potentially try to first cast a string to int in a
wrong manner and only then check if the value is a specific integer
value.

Upstream TuneD fix: redhat-performance/tuned#360

Resolves rhbz#1974277.
openshift-cherrypick-robot pushed a commit to openshift-cherrypick-robot/cluster-node-tuning-operator that referenced this pull request Jun 22, 2021
@jmencak
Fix conditional order for setting net device param.
6394858 
The conditional for setting a net device parameter in TuneD has a wrong
order where it can potentially try to first cast a string to int in a
wrong manner and only then check if the value is a specific integer
value.

Upstream TuneD fix: redhat-performance/tuned#360

Resolves rhbz#1974277.
openshift-cherrypick-robot pushed a commit to openshift-cherrypick-robot/cluster-node-tuning-operator that referenced this pull request Jun 24, 2021
@jmencak
Fix conditional order for setting net device param.
55c89a4 
The conditional for setting a net device parameter in TuneD has a wrong
order where it can potentially try to first cast a string to int in a
wrong manner and only then check if the value is a specific integer
value.

Upstream TuneD fix: redhat-performance/tuned#360

Resolves rhbz#1974277.
IlyaTyomkin pushed a commit to IlyaTyomkin/cluster-node-tuning-operator that referenced this pull request Jun 5, 2023
@jmencak @IlyaTyomkin
Fix conditional order for setting net device param.
b2b01a2 
The conditional for setting a net device parameter in TuneD has a wrong
order where it can potentially try to first cast a string to int in a
wrong manner and only then check if the value is a specific integer
value.

Upstream TuneD fix: redhat-performance/tuned#360

Resolves rhbz#1974277.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@yarda yarda yarda approved these changes

@CZerta CZerta CZerta approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@yanirq @yarda @CZerta