Fix #181

[gin-ahirsch]

This moves the allocation of the LogDataOperation slightly down, since we now only enqueue the operation conditionally.

[Lightjohn]

I am now running that branch as my normal build but it's crashing on verbose logging.
It happens systematically when I follow the output and enable the search for pattern (no regex, only multiple words). If you need more information I can run in debug mode or something like that.

[gin-ahirsch]

I can't reproduce this with neither "-d" nor "-dddd". If you could build in debug mode and post a stack-trace when glogg crashes that'd help.

[Lightjohn]

That's strange it's systematic for me:
The error in debug mode (-dddd):
*** Error in ./glogg: corrupted size vs. prev_size: 0x-

The heapdump from -d:
heap.log

The error from -dddd is not here every time.

I'm on Ubuntu 16.04 default qt5-dev libs.

[gin-ahirsch]

It seems seek()ing the closed file fails and it eventually crashes. I've changed the fix to open in a new QFile and only use it if opening is successful. Otherwise it keeps the previously opened file.

[gin-ahirsch]

@nickbnf Do I need to lock fileMutex_ to modify the attached_file_?

[Lightjohn]

It's no more crashing on my side.
I don't know what was the goal of that fix, but there is still a corner case (that I know is painful):
If the folder containing the logs is deleted and re created with the logs, this version does not see the new one.
I don't think it's in the scope of that fix but I wanted you to know that case.

[gin-ahirsch]

Yeah, that'd be considered a different file. I think the watcher works in terms of inodes, not filenames. It should behave the same as tail -f I think.

[Lightjohn]

Exactly, that was my guess, but these branch is already a big improvement for my daily workflow :) Thanks

…

On Thu, 21 Jun 2018 at 13:10, gin-ahirsch ***@***.***> wrote: Yeah, that'd be considered a different file. I think the watcher works in terms of inodes, not filenames. It should behave the same as tail -f I think. — You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub <#222 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AHN8GVjLS-Ade_Js0jIPCgYyMVUA_3bEks5t-401gaJpZM4Uni5x> .

[nickbnf]

@gin-ahirsch, yes we should lock fileMutex_ everytime we touch attached_file_ (it was mainly done to maintain the atomicity of each seek-read sequence). I don't really see what could go wrong if we do not here but it should.

I don't see why this change would fix @Lightjohn's issue #151. And yes I know it does but I just can't see why, any idea?

[gin-ahirsch]

@Lightjohn had issues with seek() being called on a closed file. Perhaps this sometimes just returned messed up contents instead of causing a crash? Compiling with address-sanitizer indicates problems in src/compressedlinestorage.cpp which disappears with this change.
Possibly the inotify implementation could also be improved so that we only get a signal when we can actually open the changed file? I mean there can still be "races" if we change and immediately after delete the file, the inotify-signal gets sent in-between those two actions and glogg reacts after the deletion has occurred. But this isn't even the case I tested, I just edited the file and saved the changes.

[nickbnf]

Thanks @gin-ahirsch, I've committed a similar fix in 9f85093 that uses a heuristic to re-open the file only when needed, it should fix @Lightjohn's problem too without causing extra processing in the common case of an appended file.