-
Notifications
You must be signed in to change notification settings - Fork 35
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
Do not crash with internal error if /etc/hosts is corrupted #603
Conversation
Unit test would be more than nice to have... |
So the situation is that a config file contains garbage (which manifests as a parsing error down in CFA). It's not random bytes but YaST can't make sense of it. If we say "Reading /etc/hosts failed" the user will think that the file is not there or the permissions are wrong. We should clearly say that we do not understand the contents. |
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.
@jreidinger have you dealt with unparseable data yet? I would like to get your opinion to make this consistent.
src/modules/Lan.rb
Outdated
@@ -370,7 +370,10 @@ def Read(cache) | |||
# Progress step 6/9 | |||
ProgressNextStage(_("Reading hostname and DNS configuration...")) if @gui | |||
DNS.Read | |||
Host.Read | |||
if !Host.Read | |||
ret = @gui && Popup.ContinueCancel(_("Reading /etc/hosts failed.\nThe file content is not in expected format.\nIf you continue any existing configuration will be overwritten.")) |
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.
This line is too long
src/modules/Lan.rb
Outdated
@@ -370,7 +370,10 @@ def Read(cache) | |||
# Progress step 6/9 | |||
ProgressNextStage(_("Reading hostname and DNS configuration...")) if @gui | |||
DNS.Read | |||
Host.Read | |||
if !Host.Read |
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.
yast2 host
will fail because it does not check the return value or Host.Read, and there are other callers too.
src/modules/Host.rb
Outdated
# save hosts to check for changes later | ||
@hosts_init = CFA::Hosts.new | ||
@hosts_init.load | ||
rescue RuntimeError => error |
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.
Two problems with this:
RuntimeError
is too generic- cannot see where the parse error occurred
In detail:
-
How about having an exception class across YaST,
GarbageDataError
, meaning that YaST cannot understand the config files that it's supposed to edit. My point is that we should not let the user proceed and instead have a global rescue, similar to the (500) Internal Error, but labeled (400) "It's Your Fault!!", or "Lost In Translation", or "Cannot Process Such Data". -
The actual error is like
'Augeas parsing/serializing error: Iterated lens matched less than it should at /usr/share/augeas/lenses/dist/hosts.aug:23.12-.42:' (RuntimeError)
and a specific problem for Augeas is that the line location refers to the lens, or grammar, and not the actual unparseable data :( Line 23 in hosts.aug is
let lns = ( empty | comment | record ) *
We probably cannot solve that in this PR. See also https://github.com/hercules-team/augeas/wiki/Tracking-down-errors
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.
ideally we shoud log content of file and then we can easily reproduce it.
@mvidner in bootloader I am solving it with this: https://github.com/yast/yast-bootloader/blob/master/src/lib/bootloader/config_dialog.rb#L23 |
The PR for reporting error location in the parsed file was already merged for Taking that in account I would like to know which is the state of this PR. Do we plan to continue working on it?, is the patch OK for SP3 as it is (of course after solving the conflicts)? The PBI for this PR is under the waterline but has not received much attention since a while: |
5fc6792
to
32ccf01
Compare
backported updates from #621 |
32ccf01
to
40add27
Compare
bsc#1077435