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

Fix handling of trailing spaces in named.conf (bsc#976643) #54

Merged
merged 2 commits into from
May 4, 2016

Conversation

cwh42
Copy link
Contributor

@cwh42 cwh42 commented May 4, 2016

I tested that fix manually in a SP1 VM using the named.conf attached to the bug: It did not merge those two lines as it did before. The file looks sane afterwards.

@@ -22,7 +22,7 @@
"comments" : [ "^[ \t]*#.*$", "^[ \t]*$" ],
"params" : [
$[
"match" : [ "([^ \t]+)[ \t]([^ \t]+(.*[^ \t]+)+)[ \t]*;$", "%s %s;" ],
"match" : [ "([^ \t]+)[ \t]([^ \t]+(.*[^ \t]+)+)[ \t]*;", "%s %s;" ],
Copy link
Member

Choose a reason for hiding this comment

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

what about using /spaces/*$ at the end?

Copy link
Contributor Author

@cwh42 cwh42 May 4, 2016

Choose a reason for hiding this comment

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

My first try was

[ \t]*;[ \t]*$

which also did the job. But then I found that one I used the even easier solution which the same outcome.

Honestly I am not sure if named.conf would even allow to have all the config in just one line without line breaks, but since our parser was not able to handle this even before this is at least no regression.

It is a L3 and so the priority should be a quick solution. The even better solution might be rewriting this using libaugeas – especially due to this comment in the code (by @jsrain):
// TODO: INI agent isn't optimal, in this file multiple occurences of one key
// in one section may be possible, section on one line is handled as a
// single value, and many many other problems are to be expected....
// should be replaced in future by some better agent
But this might better be a separate PBI.

Copy link
Contributor Author

@cwh42 cwh42 May 4, 2016

Choose a reason for hiding this comment

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

I tried a few scenarios and the solution that even would cope with comments after the semicolon would be:

[ \t];.*$

Both of the above versions fail on that and also the current implementation.
So with this one I would even fix a not-yet-found bug - although the parser needs improvement anyway.

@mvidner
Copy link
Member

mvidner commented May 4, 2016

The ini-agent parser is not very suitable for the config file. But given the bug, I think this limited fix is a good one.
LGTM.

@cwh42 cwh42 merged commit df1d197 into yast:master May 4, 2016
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.

None yet

3 participants