From 63f62c3d9c2e6e5fd2b5185b0cb109f5bfdb801f Mon Sep 17 00:00:00 2001 From: Graham Clark Date: Sun, 10 Jul 2022 23:16:44 -0400 Subject: [PATCH 1/2] Fix bug meaning that default config was not reloaded When I refactored the config code to support profiles, I broke this functionality - the correct call restores the ability of termshark to respond to config file edits that happen outside of the running application. Note that to fully support this, I should add a watcher for the profile config too (coming soon). --- cmd/termshark/termshark.go | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/cmd/termshark/termshark.go b/cmd/termshark/termshark.go index 5b46776..47dddd7 100644 --- a/cmd/termshark/termshark.go +++ b/cmd/termshark/termshark.go @@ -20,9 +20,9 @@ import ( "github.com/blang/semver" "github.com/gcla/gowid" "github.com/gcla/termshark/v2" - "github.com/gcla/termshark/v2/pkg/cli" "github.com/gcla/termshark/v2/configs/profiles" "github.com/gcla/termshark/v2/pkg/capinfo" + "github.com/gcla/termshark/v2/pkg/cli" "github.com/gcla/termshark/v2/pkg/confwatcher" "github.com/gcla/termshark/v2/pkg/convs" "github.com/gcla/termshark/v2/pkg/fields" @@ -41,7 +41,6 @@ import ( "github.com/mattn/go-isatty" "github.com/shibukawa/configdir" log "github.com/sirupsen/logrus" - "github.com/spf13/viper" "net/http" _ "net/http" @@ -1357,7 +1356,7 @@ Loop: case <-watcher.ConfigChanged(): // Re-read so changes that can take effect immediately do so - if err := viper.ReadInConfig(); err != nil { + if err := profiles.ReadDefaultConfig(dirs[0].Path); err != nil { log.Warnf("Unexpected error re-reading toml config: %v", err) } ui.UpdateRecentMenu(app) From e8de021732fa62b3979b8000ec56c4336d6f3cf0 Mon Sep 17 00:00:00 2001 From: Graham Clark Date: Sun, 10 Jul 2022 23:19:38 -0400 Subject: [PATCH 2/2] Fix bug that resulted in sporadic failed stream reassembly The failure coincided with this error in the log: msg="Stream parser reported error: read |0: file already closed" The StdoutPipe docs explain the problem: https://pkg.go.dev/os/exec?utm_source=godoc#Cmd.StdoutPipe "Wait will close the pipe after seeing the command exit, so most callers need not close the pipe themselves. It is thus incorrect to call Wait before all reads from the pipe have completed. For the same reason, it is incorrect to call Run when using StdoutPipe. See the example for idiomatic usage." My bug was that I Wait()ed for the stream process to end in a goroutine while at the same time, I handed off the process's stdout to a parser to build the stream data structures. Depending on timing, I might call Wait() before the stream parser has finished reading the output - the problem described above. --- CHANGELOG.md | 1 + pkg/streams/loader.go | 6 +++--- 2 files changed, 4 insertions(+), 3 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 58a153c..5e31897 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -12,6 +12,7 @@ ### Changed +- Fixed a bug that resulted in stream reassembly sporadically failing and displaying a blank screen. - Termshark will now, by default, suppress errors from tshark. You can change this via the minibuffer `set suppress-tshark-errors` command. - Added a summary of standard error to the error dialogs displayed when a tshark process fails to run diff --git a/pkg/streams/loader.go b/pkg/streams/loader.go index 23209fc..c0f7789 100644 --- a/pkg/streams/loader.go +++ b/pkg/streams/loader.go @@ -205,9 +205,9 @@ func (c *Loader) loadStreamReassemblyAsync(pcapf string, proto string, idx int, log.Infof("Started stream reassembly command %v with pid %d", c.streamCmd, c.streamCmd.Pid()) - termshark.TrackedGo(func() { + defer func() { termChan <- c.streamCmd.Wait() - }, Goroutinewg) + }() pid = c.streamCmd.Pid() procChan <- pid @@ -219,7 +219,7 @@ func (c *Loader) loadStreamReassemblyAsync(pcapf string, proto string, idx int, func() { _, err := ParseReader("", streamOut, ops...) if err != nil { - log.Infof("Stream parser reported error: %v", err) + log.Warnf("Stream parser reported error: %v", err) } }()