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

Using different defaults about logging for vs/pure-ftpd (bnc#888287) #5

Closed
wants to merge 7 commits into from

Conversation

cwh42
Copy link
Contributor

@cwh42 cwh42 commented Jul 30, 2014

also added test

@@ -1,4 +1,10 @@
-------------------------------------------------------------------
Wed Jul 30 09:32:53 UTC 2014 - cwh@suse.com

- Using different defaults about logging for vs/pure-ftpd (bnc#888287)
Copy link
Member

Choose a reason for hiding this comment

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

vsftpd/pure-ftpd otherwise someone grepping for vsftpd wouldn't find it

@mvidner
Copy link
Member

mvidner commented Jul 30, 2014

  • the bug report explicitly talks about syslog_enable=NO. But this PR changes log_ftp_protocol. Why the difference? That should be explained both in the PR and in the bug.
  • we have test cases, good, but they all test another parameter, VerboseLogging, without explanation.

@mvidner
Copy link
Member

mvidner commented Jul 30, 2014

Looking briefly at the existing code I see a big mess (ValueUI has 1000 lines!) that configures two FTP servers, with or without xinetd. Are there comments in the code explaining the general design?

I can guess what @VS_SETTINGS and @PURE_SETTINGS mean. But what about @EDIT_SETTINGS? Does it simply copy one of the above? Does it translate one vocabulary to the other? The same for @DEFAULT_CONFIG.

@cwh42
Copy link
Contributor Author

cwh42 commented Jul 30, 2014

You are right that it is not obvious how it works. It needs definitively some love. After fixing the bug I created the test case to be used after a rewrite of least a part of that module in proper ruby. For now I just wanted to fix that bug - all the code beautification should happen in a different commit. I will try to explain my fix a bit more detailled.

@cwh42
Copy link
Contributor Author

cwh42 commented Jul 31, 2014

Explaining this fix:
To configure vsftpd's logging, two options need to be enabled: log_ftp_protocol and syslog_enable
Vsftp's default config file has commented out the default value 'YES' for log_ftp_protocol. So yast tries read it from /etc/vsftpd.conf and returnes nil. Since we did not offer a proper default value within yast, the UI showed the checkbox "Verbose Logging" as disabled.
When writing the config file it sets both values, log_ftp_protocol and syslog_enable to the same value according to the checkbox - which is, when not changed by the user: NO.

So the fix is to add in yast's internal list of default values for vsftpd "log_ftp_protocol = YES" which results in yast writing the right default settings.

@mvidner
Copy link
Member

mvidner commented Aug 1, 2014

Actually not. It turns out the correct setting is xferlog_enable.
As for the various *_SETTINGS attributes, I've documented that in #7.

@ancorgs
Copy link
Contributor

ancorgs commented Aug 1, 2014

Remember that information in pull requests is volatile (nobody read it once it's closed), so explanations about what the code does and why should be in the code itself.

@kobliha
Copy link
Member

kobliha commented Aug 20, 2014

I guess we should focus on finishing this PR.

"pasv_max_port" => "30100"
}

PURE_SETTINGS = {
Copy link
Member

Choose a reason for hiding this comment

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

Just nit picking: I'd rather prefer putting large mock data into separate files (fixtures). In this case it's not that big like in available_addons.yml, but still makes sense IMHO.

(Ideally define a helper method in spec_helper.rb and then use it in tests.)

Then you can easily share the data in more tests.

@cwh42
Copy link
Contributor Author

cwh42 commented Aug 20, 2014

Opened a new pull request, since this one is somehow screwed

@cwh42 cwh42 closed this Aug 20, 2014
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

5 participants