>> but isn't it possible that your proposed patch deletes a newly-created file
>> even in the absence of power failure issues??
Accidental deletion of wrong files is not my intention, but please do not trust
my word, You have the source code in front of You.
>> On the other hand, if the file system state can be created through an
>> admin accident, too, and is easy to detect/work around, that's what we
>> should do -- because the unterminated loop isn't specific to a file
>> system bug.
Ack. Again there are many solutions to a problem. I am not the maintainer here
so I just indicate a solution to that partitcular problem without refactoring.
The proposed unlink() is intended to match the predominant programming style
found in that module.
I think we agree that the situation can be produced by admin accident or
malevolence so we can skip the focus on file system and power loss issues.
Also this unterminated loop if triggered leads to excessive CPU usage and is
able to block applications in case /dev/log is a unix stream.
As we saw in the previous discussion the error case is not very likely to
occur under normal operation. Also the special case with hard links I described
initially that leads to the rename() operation not working as expected without
returning an error code might require more effort to be detected than necessary.
On the other hand I do not think that detailed detection of the cause is
necessary here. The current log rotation loop is rather generic and ignorant
on file existence and return values:
/* rename: f.8 -> f.9; f.7 -> f.8; ... */
while (1) {
sprintf(newFile, "%s.%d", G.logFilePath, i);
if (i == 0) break;
sprintf(oldFile, "%s.%d", G.logFilePath, --i);
/* ignore errors - file might be missing */
rename(oldFile, newFile);
}
with the final step of the rotation as
/* newFile == "f.0" now */
rename(G.logFilePath, newFile);
after that point G.logFilePath is expected to be gone, so I think it is ok to
'make sure' it is
/* POSIX requires that rename does nothing and reports
* success if oldpath and newpath are existing hard
* links referring to the same file. */
unlink(G.logFilePath);.
otherwise
goto reopen;
...
if (G.logFileSize && G.isRegular && G.curFileSize > G.logFileSize) {
if (G.logFileRotate) { /* always 0..99 */
would trigger again and again due to the open construct as reused also during
the normal daemon startup
G.logFD = open(G.logFilePath, O_WRONLY | O_CREAT
| O_NOCTTY | O_APPEND | O_NONBLOCK,
0666);
which could otherwise have been extended by truncation.
Regards,
Christian
_______________________________________________
busybox mailing list
[email protected]
http://lists.busybox.net/mailman/listinfo/busybox
