PR: 270497
Details
Details
Diff Detail
Diff Detail
- Repository
- rG FreeBSD src repository
- Lint
Lint Skipped - Unit
Tests Skipped - Build Status
Buildable 69814 Build 66697: arc lint + arc unit
Event Timeline
Comment Actions
Michael, I took another look at your review, and it dawned on me that we don't need to know which timestamp format is in use. We only need patterns that match either format. Rather than trying to explain things in the other review or take it over, I thought I would first create a separate review for you to consider.
I haven't thoroughly tested this version because I don't have adequate logs.
What do you think?
| usr.sbin/periodic/etc/daily/460.status-mail-rejects | ||
|---|---|---|
| 41 | The space after %e was there on purpose. I read it somewhere either in a manpage or the RFC, therfore I retained it after I found this information. I need to ivenstigate the requirement if you want me to. | |
| 47 | We should consistently decide whether the T' should be part of the prefix or covered by *.`. It is inconsistent now. | |
| 71 | You have the caret now here and in the actual prefix. I personally left if here instead of the prefix to avoid duplication. Please decide where you want to have it. | |
| usr.sbin/periodic/etc/security/800.loginfail | ||
| 44 | Same here | |
| 67 | This is wrong and will fail for the newer RFC. Please check my review. | |
| usr.sbin/periodic/etc/security/900.tcpwrap | ||
| 44 | Same here | |
Comment Actions
I like your idea better than mine. No runtime fiddling, we know both formats and can filter for both.
Comment Actions
- Add a space after %e
- Ensure we match what we really want by putting the .* in the grep command and putting the patterns for the alternatives timestamps inside parentheses.
- We now neeed egrep in 900.tcpwrap.
Comment Actions
@michaelo, I believe I've addressed all your concerns, and I've performed some light testing with manually created logs. Are you able and willing to do any testing with your real logs for the three scripts?
Comment Actions
I'll do all of it tomorrow. Thank you very much for the time while others keep ignoring me! Truly appreciated.
Comment Actions
Thanks. At your convenience.
I think the feeling of being ignored is fairly common in the project. There is just more work to be done than time to do it all, and people often expend their energy on things that interest them.
Comment Actions
That is fine, but that should not block people of doing work if others don't want to participate.
| usr.sbin/periodic/etc/daily/460.status-mail-rejects | ||
|---|---|---|
| 39–47 | ||
| usr.sbin/periodic/etc/security/800.loginfail | ||
| 49 | For consistency reasons these variables should be in the same order as in 460.status-mail-rejects: yesterday_3164="$(date -v-1d '+%b %e ')"
yesterday_5424="$(date -v-1d -I)"
prefix_yesterday="(^${yesterday_3164}|^<[0-9]{1,3}>1 ${yesterday_5424}T)" | |
| 67 | Are you confident that you can drop ".*:" in the traditional format and a plain ".*" is safe? I am fine if you are. | |
| usr.sbin/periodic/etc/security/900.tcpwrap | ||
| 49 | Same here: For consistency reasons these variables should be in the same order as in 460.status-mail-rejects: yesterday_3164="$(date -v-1d '+%b %e ')"
yesterday_5424="$(date -v-1d -I)"
prefix_yesterday="(^${yesterday_3164}|^<[0-9]{1,3}>1 ${yesterday_5424}T)" | |
| usr.sbin/periodic/etc/security/800.loginfail | ||
|---|---|---|
| 49 | The current ordering of the variables makes the most sense to me. In 460.status-mail-rejects, we are building two patterns:
So, it makes sense to order the variables like this: # pattern for today today_3164=... today_5424=... prefix_today=... # pattern for yesterday yesterday_3164=... yesterday_5424=... prefix_yesterday=... ... sed -En -e "/$prefix_today/q" -e "/$prefix_yesterday/.... In the other two files, we are building a single pattern to match yesterday's logs in either format. So, it makes sense to order the variables like this: yesterday_3164=...
prefix_3164=...
yesterday_5424=...
prefix_5424=...
prefix="(${prefix_3164}|${prefix_5424})"In other words, the variables are introduced in the same order they are found in the final pattern. | |
| 67 | I'm confident, but you're right to point it out. Let's keep things as they were unless there is a compelling reason not to. I'll update the diff momentarily. | |