-
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
Optimize hosts read #454
Optimize hosts read #454
Changes from 19 commits
a76777f
671bc1c
183af41
69fb68e
2814e51
0fea43b
c9b506e
9cc559f
2891ea7
329459d
8f87a1d
1fc89e6
10a186a
ce70f39
447cc1b
8d9d6b8
b5fd998
daa9755
c460b5b
43ebe40
5d4bfaa
8eb9bfa
72281b1
9bdd9af
b508d83
e6b65e0
ed6a634
63ed52c
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,189 @@ | ||
require "yast" | ||
require "yast2/target_file" | ||
|
||
require "cfa/base_model" | ||
require "cfa/matcher" | ||
require "cfa/augeas_parser" | ||
|
||
module CFA | ||
# class representings /etc/hosts file model. It provides helper to manipulate | ||
# with file. It uses CFA framework and Augeas parser. | ||
# @see http://www.rubydoc.info/github/config-files-api/config_files_api/CFA/BaseModel | ||
# @see http://www.rubydoc.info/github/config-files-api/config_files_api/CFA/AugeasParser | ||
class Hosts < BaseModel | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. For the people that did not design CFA::BaseModel, a comment with the full URL of its API docs will be useful: http://www.rubydoc.info/github/config-files-api/config_files_api/CFA/BaseModel There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. updated |
||
PARSER = AugeasParser.new("hosts.lns") | ||
PATH = "/etc/hosts".freeze | ||
include Yast::Logger | ||
|
||
def initialize(file_handler: nil) | ||
super(PARSER, PATH, file_handler: file_handler) | ||
end | ||
|
||
# The old format used by {Yast::HostClass}. | ||
# @return [Hash{String => Array<String>}] keys are IPs, | ||
# values are lists of lines in /etc/hosts (not names!) | ||
# with whitespace separated hostnames, where the first one is canonical | ||
# and the rest are aliases | ||
# | ||
# For example, the file contents | ||
# | ||
# 1.2.3.4 www.example.org www | ||
# 1.2.3.7 log.example.org log | ||
# 1.2.3.7 sql.example.org sql | ||
# | ||
# is returned as | ||
# | ||
# { | ||
# "1.2.3.4" => "www.example.org www" | ||
# "1.2.3.7" => [ | ||
# "log.example.org log", | ||
# "sql.example.org sql" | ||
# ] | ||
# } | ||
def hosts | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. this and some other methods miss documentation. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. ok |
||
matcher = Matcher.new { |k, _v| k =~ /^\d*$/ } | ||
data.select(matcher).each_with_object({}) do |host, result| | ||
entry = host[:value] | ||
result[entry["ipaddr"]] ||= [] | ||
result[entry["ipaddr"]] << single_host_entry(entry) | ||
end | ||
end | ||
|
||
# Returns single entry from hosts for given ip or empty array if not found | ||
# @see #hosts | ||
# @return [Array<String>] | ||
def host(ip) | ||
hosts = data.select(ip_matcher(ip)) | ||
|
||
hosts.map do |host| | ||
single_host_entry(host[:value]) | ||
end | ||
end | ||
|
||
# deletes all occurences of given ip in host table | ||
# @return [void] | ||
def delete_host(ip) | ||
entries = data.select(ip_matcher(ip)) | ||
if entries.empty? | ||
log.info "no entry to delete for ip #{ip}" | ||
return | ||
end | ||
|
||
if entries.size > 1 | ||
log.info "delete host with ip '#{ip}' removes more then one entry" | ||
end | ||
|
||
entries.each do |e| | ||
log.info "deleting record #{e.inspect}" | ||
data.delete(e[:key]) | ||
end | ||
end | ||
|
||
# Replaces or adds a new host entry. | ||
# If more than one entry with the given ip exists | ||
# then it replaces the last instance. | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Does it make sense to replace only the last instance? I think the correct way is to replace all instances. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. you are right, Initially I wrongly understand code. But still there is a question. Does it make sense at all? so after this change from three entries:
you get
which looks bogus There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It is bogus, you should get
Ah, now that i think of it, which of hostA hostB hostC is the canonical name anyway? We need to test that. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @mvidner do you see it intuitive? I really do not expect such behavior. Currently it is used only in one place Also according to googling it looks like all of them are canonical and all of them will be resolved. So if you use hostA.suse.cz or hostB.suse.cz or hostC.suse.cz it should point you to same ip. And reverse it is first occurence, according to debian discussion - http://lists.debian.org/debian-devel/2004/06/msg00468.html so host for ip will get hostA.suse.cz There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. in fact I expect that such method always have oldhn and always have newhn and as result it just replace old hn with new hn...and maybe their aliases? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I have a suspicion that with the refactoring of Host.Update we have broken something. I will do some manual tests and report back. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. OK, I have tested changing
|
||
# @param [String] ip | ||
# @param [String] canonical | ||
# @param [Array<String>] aliases | ||
# @return [void] | ||
def set_host(ip, canonical, aliases = []) | ||
entries = data.select(ip_matcher(ip)) | ||
if entries.empty? | ||
add_host(ip, canonical, aliases) | ||
return | ||
end | ||
|
||
if entries.size > 1 | ||
log.info "more then one entry with ip '#{ip}'. Replacing last one." | ||
end | ||
|
||
entry = entries.last[:value] | ||
entry["ipaddr"] = ip | ||
entry["canonical"] = canonical | ||
# clear previous aliases | ||
entry.delete("alias") | ||
entry.delete("alias[]") | ||
aliases_col = entry.collection("alias") | ||
aliases.each do |a| | ||
aliases_col.add(a) | ||
end | ||
end | ||
|
||
# Adds new entry, even if it exists | ||
# @param [String] ip | ||
# @param [String] canonical | ||
# @param [Array<String>] aliases | ||
# @return [void] | ||
def add_host(ip, canonical, aliases = []) | ||
log.info "adding new entry for ip #{ip}" | ||
entry_line = AugeasTree.new | ||
entry_line["ipaddr"] = ip | ||
entry_line["canonical"] = canonical | ||
aliases_col = entry_line.collection("alias") | ||
aliases.each do |a| | ||
aliases_col.add(a) | ||
end | ||
data.add(unique_id, entry_line) | ||
end | ||
|
||
# Removes hostname from all entries in hosts table. | ||
# If it is the only hostname for a given ip, the ip is removed | ||
# If it is canonical name, then the first alias becomes the canonical hostname | ||
# @param [String] hostname | ||
# @return [void] | ||
def remove_hostname(hostname) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Rename this to |
||
entries = data.select(hostname_matcher(hostname)) | ||
entries.each do |pair| | ||
entry = pair[:value] | ||
if entry["canonical"] == hostname | ||
aliases = aliases_for(entry) | ||
if aliases.empty? | ||
delete_host(entry["ipaddr"]) | ||
else | ||
set_host(entry["ipaddr"], aliases.first, aliases[1..-1]) | ||
end | ||
else | ||
reduced_aliases = aliases_for(entry) | ||
reduced_aliases.delete(hostname) | ||
set_host(entry["ipaddr"], entry["canonical"], reduced_aliases) | ||
end | ||
end | ||
end | ||
|
||
private | ||
|
||
# returns matcher for cfa to find entries with given ip | ||
def ip_matcher(ip) | ||
Matcher.new { |_k, v| v["ipaddr"] == ip } | ||
end | ||
|
||
# returns matcher for cfa to find entries with given hostname | ||
def hostname_matcher(hostname) | ||
Matcher.new do |_k, v| | ||
v["canonical"] == hostname || aliases_for(v).include?(hostname) | ||
end | ||
end | ||
|
||
# returns aliases as array even if there is only one | ||
def aliases_for(entry) | ||
entry["alias[]"] ? entry.collection("alias").map { |a| a } : [entry["alias"]].compact | ||
end | ||
|
||
# generate old format string with first canonical and then aliases | ||
# all separated by space | ||
def single_host_entry(entry) | ||
result = [entry["canonical"]] | ||
result.concat(aliases_for(entry)) | ||
result.join(" ") | ||
end | ||
|
||
# helper to generate unique id for cfa entry | ||
def unique_id | ||
id = 1 | ||
loop do | ||
return id.to_s unless data[id.to_s] | ||
id += 1 | ||
end | ||
end | ||
end | ||
end |
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 didn't check that in details yet so it is more less note for me. Here you change semantics (list of ips -> single ip)
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.
yes, I change API because it is used only as single parameter. I think it do not make much sense to change hostname for more ips, it means you have one hostname for more ips which looks bogus for me.