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
Sle 15 sp1 bsc 1132915 #797
Conversation
the CD has already been mounted
src/lib/transfer/file_from_url.rb
Outdated
) | ||
if Installation.boot == "cd" && !cdrom_device.empty? | ||
mtab = File.read("/proc/mounts") | ||
if mtab.include?(cdrom_device) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Wouldn't a regexp that at least guarantees that mtab.split().find
below finds something be better? include?
does only a substring match and that could be anything.
src/lib/transfer/file_from_url.rb
Outdated
) | ||
if Installation.boot == "cd" && !cdrom_device.empty? | ||
mtab = File.read("/proc/mounts") | ||
if mtab.include?(cdrom_device) | ||
Builtins.y2warning( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not part of the patch, but anyway: why a warning? This is imho a normal situation - so y2milestone
.
src/lib/transfer/file_from_url.rb
Outdated
Builtins.y2warning( | ||
if Installation.boot == "cd" && !cdrom_device.empty? | ||
mtab = File.read("/proc/mounts") | ||
if mtab.match(Regexp.new("^#{cdrom_device}")) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd add at least a space: Regexp.new("^#{cdrom_device}\\s")
. But why not simply
m = mtab.match(/^#{cdrom_device}\s+(\S+)/)
where m[1]
then is the mountpoint and get rid of this split and find business?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm
changelog? - I found it :-) |
Has also been tested by the reporter.