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

openvpn plugin: Fix parsing empty fields #3425

Merged
merged 1 commit into from
Mar 19, 2020

Conversation

rpv-tomsk
Copy link
Contributor

@rpv-tomsk rpv-tomsk commented Mar 17, 2020

Closes: #3424

Changelog: openvpn plugin: Fix parsing empty fields

I have some another improvements too, but I'm not interested in publishing them due to lack of fast response from "trusted maintainers" and I'm not trusted enough to merge my own changes myself.

@rpv-tomsk rpv-tomsk requested a review from a team as a code owner March 17, 2020 12:30
@NikolayTsvetkov
Copy link
Contributor

Hi Pavel, Thank you for the contribution !
I'm sure the community and all the users would be thankful profiting from the further improvements you can provide to the plugin.
Merging your own PR has never been a good practice in any project, as code review doesn't only check the code quality, but in most cases leads to better ideas and improvements.
Looking forward seeing the new features here or in a separate PR.

Cheers !

@rpv-tomsk
Copy link
Contributor Author

rpv-tomsk commented Mar 17, 2020

as code review doesn't only check the code quality,

Code review in this project does nothing:
Examples:
#3381 - merged non-working implementation.
#1339 - (merged recently) - implementation effectively replaced by a const value
#2910 - (merged recently) - implemenation violates codestyle

... also I have another examples of review quality.

I do not believe that someone really checks the code. They simply push "approve"+"merge" buttons, and I able to do that myself, w/o third-party help.

Practice shows me - nobody interested in merging PRs, to which person has no relation.

@rpv-tomsk
Copy link
Contributor Author

... but in most cases leads to better ideas and improvements.

In most cases it leads to worthless waiting.

There was examples of 6+ months waiting. It takes so much time to review?
No, it simply just shows the attitude towards the developers, like the other endless problems of CI and clang-format infrastructure.

@faxm0dem
Copy link
Contributor

Hi Pavel, what's your rationale behind also changing the version detection?

@rpv-tomsk
Copy link
Contributor Author

rpv-tomsk commented Mar 17, 2020

Hi Pavel, what's your rationale behind also changing the version detection?

Versions 2 and 3 differs only by delimiter.
We need to detect delimiter to use it in parser.

@faxm0dem
Copy link
Contributor

according to manpage, strsep also supports a string as separator:

   that  is  delimited  by one of the bytes in the string delim.
   This token is terminated by overwriting the delimiter 

@rpv-tomsk
Copy link
Contributor Author

This implementation not uses strsep.

@faxm0dem
Copy link
Contributor

oh sorry, I misread.
oki doki, makes sense.
I'll test your patch ritght away, thanks

@faxm0dem
Copy link
Contributor

successfully tested on Centos7

@rpv-tomsk rpv-tomsk merged commit ca84bcb into collectd:master Mar 19, 2020
@rpv-tomsk rpv-tomsk deleted the openvpn branch March 19, 2020 11:25
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.

Plugin: openvpn
4 participants