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.

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

bsc#1045098 and bsc#1045103: Suppress popup message from command-line and check alloc_mem parameter. #80

Merged
merged 4 commits into from
Jun 20, 2017

Conversation

gilsonsouza
Copy link
Contributor

No description provided.

@coveralls
Copy link

Coverage Status

Changes Unknown when pulling 52e3b9e on gilsonsouza:SLE-12-SP3 into ** on yast:SLE-12-SP3**.


- The alloc_mem parameter is verified to be in accordance with
documentation (bsc#1045098).
- Pop-up is supppressed from command line when the user enables or
Copy link
Contributor

Choose a reason for hiding this comment

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

TooManyPs exception :) supppressed -> suppressed.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

lol

@@ -742,6 +742,11 @@ def cmdKdumpStartup(options)
Kdump.add_crashkernel_param = true
alloc_mem_low, alloc_mem_high = options["alloc_mem"].split(',')
Kdump.allocated_memory = { low: alloc_mem_low, high: alloc_mem_high }
unless alloc_mem_low.scan(/\D/).empty? &&
Copy link
Contributor

Choose a reason for hiding this comment

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

The condition looks tricky, specially using scan for that. What about something like:

if alloc_mem_low !~ ALLOC_MEM_REGEXP || (alloc_mem_high && alloc_mem_high !~ ALLOC_MEM_REGEXP
  # report error
  return false
end

And you could define the regular expression before the method, explaining what it does (\A and \z are the preferred way of matching beginning and end of line in Ruby):

# Only numbers are allowed as allow_mem_high and allow_mem_low values
ALLOC_MEM_REGEXP = /\A\d+\z/

@coveralls
Copy link

Coverage Status

Changes Unknown when pulling 6f361e6 on gilsonsouza:SLE-12-SP3 into ** on yast:SLE-12-SP3**.

@coveralls
Copy link

Coverage Status

Changes Unknown when pulling 2a7a439 on gilsonsouza:SLE-12-SP3 into ** on yast:SLE-12-SP3**.

@gilsonsouza gilsonsouza merged commit 71f1d2a into yast:SLE-12-SP3 Jun 20, 2017
@@ -734,6 +734,8 @@ def cmdKdumpShow(options)
end


# Only numbers are allowed as allow_mem_high and allow_mem_low values
ALLOC_MEM_REGEXP = /\D/
Copy link
Member

Choose a reason for hiding this comment

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

@gilsonsouza this is not what @imobachgs has suggested. This will wrongly mark "R2D2" as valid input.

Copy link
Member

Choose a reason for hiding this comment

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

Oh, \D is a non-digit, but that leaves me even more confused

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I just tested:
gilson@linux-8x2s:~/yast-kdump> sudo yast2 kdump startup enable alloc_mem=R2D2
Invalid allocation memory parameter
Use 'yast2 kdump help' for a complete list of available commands.

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.

4 participants