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

switch clog to cat in status_tinc.php #691

Merged
merged 9 commits into from
Nov 6, 2019

Conversation

vktg
Copy link
Contributor

@vktg vktg commented Oct 23, 2019

Redmine Issue: https://redmine.pfsense.org/issues/9740
Ready for review

There is no /usr/local/sbin/clog in pfSense 2.5
using "cat" instead

Copy link
Contributor

@jim-p jim-p left a comment

Choose a reason for hiding this comment

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

This shouldn't use cat alone, as it would produce inconsistent behavior around log rotations.

Check how other changes were made, such as mailreports (See 43710af ) and use that style.

"tail -n 50"
I think there is no reason to "sed" all log file
there is can be too many connections in logs

exec("{$clog_path} /var/log/tinc.log | /usr/bin/sed -e 's/.*tinc\[.*\]: //'", $result);
if ($firmware['config_version'] >= 19.7) {
exec(system_log_get_cat() . ' ' . $logfile . "| /usr/bin/sed -e 's/.*tinc\[.*\]: //'", $result);
Copy link
Contributor

Choose a reason for hiding this comment

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

Rather than $logfile here, it should use sort_related_log_files() like the other example.

if ($firmware['config_version'] >= 19.7) {
exec(system_log_get_cat() . ' ' . $logfile . "| /usr/bin/sed -e 's/.*tinc\[.*\]: //'", $result);
} else {
exec("{$clog_path} /var/log/tinc.log | /usr/bin/sed -e 's/.*tinc\[.*\]: //'", $result);
Copy link
Contributor

Choose a reason for hiding this comment

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

Why use the full hardcoded path to the log here, rather than {$logfile} which is populated above?


exec("{$clog_path} /var/log/tinc.log | /usr/bin/sed -e 's/.*tinc\[.*\]: //'", $result);
if ($firmware['config_version'] >= 19.7) {
Copy link
Contributor

Choose a reason for hiding this comment

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

This isn't a proper version test. The config revision is not a reliable indicator of the pfSense version. It should be based on $g['product_version'] or similar to test the actual version directly.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

in case of $g['product_version'] we need to enumerate all supported versions
better to use kernel version number

Copy link
Contributor

Choose a reason for hiding this comment

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

No, it is not. That does not necessarily determine the pfSense version either. It must use product_version. If you need to compare, there are convenience functions like pfs_version_compare() that facilitate a proper comparison.

Comment on lines 69 to 74
if ($firmware['config_version'] >= 19.7) {
exec(system_log_get_cat() . ' ' . $logfile . "| /usr/bin/sed -e 's/.*tinc\[.*\]: //'", $result);
} else {
exec("{$clog_path} /var/log/tinc.log | /usr/bin/sed -e 's/.*tinc\[.*\]: //'", $result);
}

Copy link
Contributor

Choose a reason for hiding this comment

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

See comments above which also apply to this similar block.
(And since they are practically identical, I have to wonder if this could be simplified significantly, though that's likely out of scope for this particular PR)

in case of $g['product_version'] we need to enumerate all supported versions
better to use kernel version number
if (preg_match("/Connections:/", $line)) {
$begin = $i;
if ($usr == 'USR1') {
foreach ($result as $line) {
Copy link
Contributor

Choose a reason for hiding this comment

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

The USR1 and else block are still pretty similar. Looks like you could use the if ($usr == 'USR1') ... else test to set the beginning and ending match strings only, and then just have one foreach.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fixed pfs_version_compare() + foreach

@netgate-git-updates netgate-git-updates merged commit cbe226a into pfsense:devel Nov 6, 2019
netgate-git-updates pushed a commit that referenced this pull request Nov 27, 2021
Changelog:
	* Fixed: Version `Qt_5.15' not found (required by
	  /usr/bin/ksnip). (#712)
	* Fixed: CI packages show continuous suffix for tagged build.
	  (#710)
	* Fixed: kImageAnnotator not translated with deb package. (#359)
	* Fixed: Windows packages increased in size. (#713)
	* Fixed: The string 'Actions' is not available for translation.
	  (#729)
	* Fixed: HiDPI issue with multiple screen on Windows. (#668)
	* Fixed: Snipping Area not closing when pressing ESC. (#735)
	* Fixed: Sometimes "Snipping Area Rulers" not shown after
	  starting rectangular selection. (#684)
	* Fixed: Cursor not positioned correctly when snipping area
	  opens. (#736)
	* Fixed: Mouse cursor not captured when triggered via global
	  shortcut. (#737)
	* Fixed: Dual 4K screens get scrambled on X11. (#734)
	* Fixed: VCRUNTIME140_1.dll was not found. (#743)
	* Fixed: Screenshot area issue when monitor count changes on
	  Windows. (#722)
	* Fixed: Wayland does not support QWindow::requestActivate().
	  (#656)
	* Fixed: Wrong area is captured on a Wayland screen scaling.
	  (#691)
	* Changed: Enforce xdg-desktop-portal screenshots for Gnome >=
	  41. (#727)
	* Fixed kImageAnnotator: Crash while typing text on wayland.
	  (#256)
	* Changed kImageAnnotator: Show scrollbar when not all tools
	  visible. (#258)
netgate-git-updates pushed a commit that referenced this pull request Feb 18, 2024
Changes since 1.4.0:

v1.5.1

- fix: Properly close created files by @beatbrot in #702
- fix: Skip auth type prompt if already set by @ankitpokhrel in #701

- @beatbrot made their first contribution in #702

Full Changelog: ankitpokhrel/jira-cli@v1.5.0...v1.5.1

v1.5.0

This release brings the support for mTLS authentication along with some other features like setting affects version, updating the estimate, etc.

- feat: Enable issue edit to read body from stdin by @erpel in #619
- feat: Affects version by @damianoneill in #642
- feat: Add mtls authentication for client certificate auth by @markhatc- in
  #615
- feat: Add support for updating the estimate by @chapmanc in #669

- fix: Issue with no-input on create by @ankitpokhrel in #655
- fix: Jira init broken due to authtype value by @ankitpokhrel in #694
- fix: Bring bearer back by @ankitpokhrel in #696
- fix: Respect jira timezone by @ankitpokhrel in #697

- chore: Bump go & alpine versions by @pbnj in #691
- ci: Upgrade workflow + linter by @ankitpokhrel in #695
- dep: Upgrade all by @ankitpokhrel in #643

- @erpel made their first contribution in #619
- @damianoneill made their first contribution in #642
- @pbnj made their first contribution in #691
- @chapmanc made their first contribution in #669

Full Changelog:
ankitpokhrel/jira-cli@v1.4.0...v1.5.0
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants