Skip to content

resume logs if prerotate script fails#502

Closed
banjiuqingshan wants to merge 1 commit intologrotate:masterfrom
banjiuqingshan:master
Closed

resume logs if prerotate script fails#502
banjiuqingshan wants to merge 1 commit intologrotate:masterfrom
banjiuqingshan:master

Conversation

@banjiuqingshan
Copy link

resume logs if prerotate script fails.#498

Copy link
Member

@kdudka kdudka left a comment

Choose a reason for hiding this comment

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

Before we dive into technical details ... @cgzones, could you please confirm that the new behavior is actually desired?

logrotate.c Outdated
int numRotated = 0;
struct logState **state;
struct logNames **rotNames;
struct resumeConfigs *resCfgs;
Copy link
Member

Choose a reason for hiding this comment

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

The resumeConfigs struct can be allocated on the stack so that we do not need to handle its (de)allocation explicitly.

int rotateCount = resCfgs->rotateCount;
int logStart = resCfgs->logStart;

if (asprintf(&oldName, "%s/%s.%d%s%s", rotNames->dirName,
Copy link
Member

Choose a reason for hiding this comment

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

The code that constructs oldname should be moved to a helper function so that it is maintained at a single place.

Copy link
Author

Choose a reason for hiding this comment

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

This place can indeed be refined. we can optimize it next time because it is also used elsewhere

@cgzones
Copy link
Member

cgzones commented Mar 28, 2023

I have no strong opinion on the behavior: the prerotate script failing is an error case, in which logrotate exists with a non 0 exit value and thus the system administrator should be notified (either via cron sending an email or the systemd timer entering the failing state). The man page currently only states

If the scripts exit with error, the remaining actions will not be executed for the affected log only.

and

The script is executed before the log file is rotated and only if the log will actually be rotated.

so the whether old logs are rotated on failure is not (for me at least) clearly stated.


Looking at the changed test cases isn't the change more about reverting the rotation of old logs than resuming?

@banjiuqingshan
Copy link
Author

I mean that logs that have been rotated will be renamed and overwritten, similar to test.log.3.gz, which will be renamed to test.log.4.gz.The commit will rename test.log.4gz back to test.log.3gz if the prerotate execution fails.Please consider whether this is necessary

cgzones added a commit to cgzones/logrotate that referenced this pull request Apr 4, 2023
Ensures old logs are preserved and not rotated out for logs with a
failing prerotate script.

Alternative to logrotate#502
@cgzones
Copy link
Member

cgzones commented Apr 4, 2023

This proposal does not resume/restore old logs of other files from the same directive when using sharedscripts.
Is this intened?
See #506 with the difference on test-shared-pre-B.log.

@banjiuqingshan
Copy link
Author

This proposal does not resume/restore old logs of other files from the same directive when using sharedscripts. Is this intened? See #506 with the difference on test-shared-pre-B.log.
This is a bug. I haven't had time to fix this recently. And being able to adjust the order of renaming would be better than resume logs.

cgzones added a commit to cgzones/logrotate that referenced this pull request Apr 28, 2023
Ensures old logs are preserved and not rotated out for logs with a
failing prerotate script.

Alternative to logrotate#502
cgzones added a commit that referenced this pull request May 5, 2023
Ensures old logs are preserved and not rotated out for logs with a
failing prerotate script.

Alternative to #502
@cgzones
Copy link
Member

cgzones commented May 5, 2023

Closing in favor of #506 (now merged).
Thanks for the patch nonetheless.

@cgzones cgzones closed this May 5, 2023
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.

3 participants