You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@plc4x.apache.org by sr...@apache.org on 2026/09/07 09:15:48 UTC
(plc4x-extras) branch develop updated: fix: Document the analyzer's commands, and let a run be interrupted
This is an automated email from the ASF dual-hosted git repository.
sruehl pushed a commit to branch develop
in repository https://gitbox.apache.org/repos/asf/plc4x-extras.git
The following commit(s) were added to refs/heads/develop by this push:
new 8cf148c3 fix: Document the analyzer's commands, and let a run be interrupted
8cf148c3 is described below
commit 8cf148c3bb789338f88dda6eb2ca1d4331ff8fc1
Author: Sebastian Rühl <sr...@apache.org>
AuthorDate: Mon Sep 7 09:50:40 2026 +0200
fix: Document the analyzer's commands, and let a run be interrupted
Two defects on the non-interactive surface, which this work had barely
touched.
Six commands shipped a placeholder as their long help. Five said "TODO:
document me" and the root said "todo describe me", so
"plc4xpcapanalyzer --help" -- the first thing anyone reads -- answered
with a note to self. The ui command's placeholder went further and
described the bacnet command instead, and two Shorts read "using a
driver supplied driver". All six now document what the command does,
including the things worth knowing before running it: that extract
prints nothing below -vv, that the BACnet default filter drops a bare
Who-Is, and that a C-Bus capture only parses correctly against the
options the interface was configured with.
And a run could not be interrupted. Both the analysis and the extraction
check their context between packets and stop cleanly, keeping what they
have gathered -- but nothing ever gave them a cancellable one: the entry
points passed context.TODO and the root passes context.Background. The
abort was implemented and unreachable, so a capture of any size had
exactly one way to stop, which was for the process to be killed part way
through a report. The context now threads from the command, and
context.TODO is gone from both packages.
The handler is installed per command rather than on the root, and
deliberately: ui runs a terminal interface that reads ctrl+c as a key
press and asks before it exits, and cancelling its context from
underneath would take that decision away.
"Done" also used to be printed unconditionally, so once an interrupt
could reach the loop it would have claimed a complete report over a
partial one. The four commands now share one body that says which way
the run ended.
Three tests, each checked against a mutation of the code it covers. Two
of them needed a second attempt to be worth anything: the first mutation
missed because the test called a different entry point than the one I
changed, and the command-level test detected its mutation by hanging on
an unbounded wait for a context that would never be done, which is a
worse failure than none. The wait is bounded now.
---
plc4go/tools/README.md | 18 +-
plc4go/tools/plc4xpcapanalyzer/cmd/analyze.go | 31 ++--
plc4go/tools/plc4xpcapanalyzer/cmd/bacnet.go | 21 ++-
plc4go/tools/plc4xpcapanalyzer/cmd/cbus.go | 23 +--
plc4go/tools/plc4xpcapanalyzer/cmd/extract.go | 25 +--
plc4go/tools/plc4xpcapanalyzer/cmd/interrupt.go | 98 ++++++++++
.../tools/plc4xpcapanalyzer/cmd/interrupt_test.go | 197 +++++++++++++++++++++
plc4go/tools/plc4xpcapanalyzer/cmd/root.go | 19 +-
plc4go/tools/plc4xpcapanalyzer/cmd/ui.go | 17 +-
.../internal/analyzer/analyzer.go | 13 +-
.../analyzer/analyzer_characterisation_test.go | 69 +++++++-
.../internal/extractor/extractor.go | 16 +-
12 files changed, 486 insertions(+), 61 deletions(-)
diff --git a/plc4go/tools/README.md b/plc4go/tools/README.md
index b83c426b..e53d7d78 100644
--- a/plc4go/tools/README.md
+++ b/plc4go/tools/README.md
@@ -62,7 +62,23 @@ The analyzer handles `bacnetip` and `c-bus`. `bacnet` and `cbus` are accepted as
every layer resolves names through one registry so the command line, the analyzer, the
extractor and the interface cannot disagree about them.
-`extract` prints payloads only at verbosity 2 or above: pass `-vv`.
+`extract` prints payloads only at verbosity 2 or above: pass `-vv`. Without it the command walks
+the capture and prints nothing, which looks like a failure and is not one.
+
+`analyze bacnet <capture>` and `analyze c-bus <capture>` reach the *subcommands*, which carry
+their own filter and protocol-option flags. Reaching `analyze`'s own body takes a protocol name
+that is not also a subcommand -- `analyze bacnetip <capture>` -- and the two paths do the same
+work.
+
+`Ctrl+C` stops a run and reports `Aborted` rather than `Done`, keeping the counts gathered so
+far. The interrupt handler is installed per command rather than on the root, deliberately: `ui`
+runs a terminal interface that reads `Ctrl+C` as a key press and asks before it exits, and
+cancelling its context from underneath would take that decision away.
+
+Both loops have always checked their context between packets. What was missing was any way to
+reach the check: the entry points passed `context.TODO` and the root passes
+`context.Background`, so the abort was implemented and unreachable, and a capture of any size
+had exactly one way to stop -- for the process to be killed part way through a report.
## plc4xbrowser
diff --git a/plc4go/tools/plc4xpcapanalyzer/cmd/analyze.go b/plc4go/tools/plc4xpcapanalyzer/cmd/analyze.go
index beb3fb7e..06f1a261 100644
--- a/plc4go/tools/plc4xpcapanalyzer/cmd/analyze.go
+++ b/plc4go/tools/plc4xpcapanalyzer/cmd/analyze.go
@@ -20,7 +20,6 @@
package cmd
import (
- "fmt"
"math"
"os"
@@ -28,17 +27,31 @@ import (
"github.com/spf13/cobra"
"github.com/apache/plc4x-extras/plc4go/tools/plc4xpcapanalyzer/config"
- "github.com/apache/plc4x-extras/plc4go/tools/plc4xpcapanalyzer/internal/analyzer"
"github.com/apache/plc4x-extras/plc4go/tools/plc4xpcapanalyzer/internal/protocol"
)
// analyzeCmd represents the analyze command
var analyzeCmd = &cobra.Command{
Use: "analyze [protocolType] [pcapfile]",
- Short: "analyzes a pcap file using a driver supplied driver",
- Long: `Analyzes a pcap file using a driver
-TODO: document me
-`,
+ Short: "Parse, reserialize and compare every packet in a capture",
+ Long: `Runs a capture through a plc4x codec and reports what it cannot handle.
+
+Each packet's application payload is parsed, the message that comes back is re-serialized, and
+the bytes are compared against the original. Three things can go wrong, and they are counted
+separately because they mean different things:
+
+ parse the codec was handed a message and could not read it
+ serialize the codec read the message but could not write it back
+ compare it wrote it back as different bytes, so reader and writer disagree
+
+A packet the protocol itself says is not a whole message -- a split transmission, an empty
+packet, an echo -- is skipped rather than failed. A skip is not a defect and is not counted as
+one.
+
+The protocol is bacnetip or c-bus; bacnet and cbus are accepted as aliases. The bacnet and c-bus
+subcommands do the same job with the protocol fixed and their own filter flags available.
+
+An interrupt stops the run and keeps the counts gathered so far.`,
Args: func(cmd *cobra.Command, args []string) error {
if len(args) < 2 {
return errors.New("requires exactly two arguments")
@@ -55,11 +68,7 @@ TODO: document me
RunE: func(cmd *cobra.Command, args []string) error {
protocolType := args[0]
pcapFile := args[1]
- if err := analyzer.Analyze(pcapFile, protocolType); err != nil {
- return err
- }
- _, _ = fmt.Fprintln(cmd.OutOrStdout(), "Done")
- return nil
+ return analyse(cmd, pcapFile, protocolType)
},
}
diff --git a/plc4go/tools/plc4xpcapanalyzer/cmd/bacnet.go b/plc4go/tools/plc4xpcapanalyzer/cmd/bacnet.go
index 4a40ff01..81f72ed1 100644
--- a/plc4go/tools/plc4xpcapanalyzer/cmd/bacnet.go
+++ b/plc4go/tools/plc4xpcapanalyzer/cmd/bacnet.go
@@ -20,24 +20,27 @@
package cmd
import (
- "fmt"
"os"
"github.com/apache/plc4x/plc4go/spi/errors"
"github.com/spf13/cobra"
"github.com/apache/plc4x-extras/plc4go/tools/plc4xpcapanalyzer/config"
- "github.com/apache/plc4x-extras/plc4go/tools/plc4xpcapanalyzer/internal/analyzer"
"github.com/apache/plc4x-extras/plc4go/tools/plc4xpcapanalyzer/internal/protocol"
)
// bacnetCmd represents the bacnet command
var bacnetCmd = &cobra.Command{
Use: "bacnet [pcapfile]",
- Short: "analyzes a pcap file using a bacnet driver",
- Long: `Analyzes a pcap file using a bacnet driver
-TODO: document me
-`,
+ Short: "Analyse a capture as BACnet/IP",
+ Long: `Runs a capture through plc4x's BACnet/IP codec: the analyze command with the protocol
+fixed, and with the BACnet filter flag available.
+
+The default filter is "udp port 47808 and udp[4:2] > 29", which drops packets whose UDP length
+is 29 bytes or less. That excludes a bare Who-Is, which is 12 bytes: pass
+--default-bacnet-filter to widen it if the short messages are what you are looking for.
+
+An interrupt stops the run and keeps the counts gathered so far.`,
Args: func(cmd *cobra.Command, args []string) error {
if len(args) < 1 {
return errors.New("requires exactly one arguments")
@@ -50,11 +53,7 @@ TODO: document me
},
RunE: func(cmd *cobra.Command, args []string) error {
pcapFile := args[0]
- if err := analyzer.Analyze(pcapFile, protocol.BacnetIP.Name); err != nil {
- return err
- }
- _, _ = fmt.Fprintln(cmd.OutOrStdout(), "Done")
- return nil
+ return analyse(cmd, pcapFile, protocol.BacnetIP.Name)
},
}
diff --git a/plc4go/tools/plc4xpcapanalyzer/cmd/cbus.go b/plc4go/tools/plc4xpcapanalyzer/cmd/cbus.go
index ec174b6e..6fc523ff 100644
--- a/plc4go/tools/plc4xpcapanalyzer/cmd/cbus.go
+++ b/plc4go/tools/plc4xpcapanalyzer/cmd/cbus.go
@@ -20,24 +20,29 @@
package cmd
import (
- "fmt"
"os"
"github.com/apache/plc4x/plc4go/spi/errors"
"github.com/spf13/cobra"
"github.com/apache/plc4x-extras/plc4go/tools/plc4xpcapanalyzer/config"
- "github.com/apache/plc4x-extras/plc4go/tools/plc4xpcapanalyzer/internal/analyzer"
"github.com/apache/plc4x-extras/plc4go/tools/plc4xpcapanalyzer/internal/protocol"
)
// cbusCmd represents the cbus command
var cbusCmd = &cobra.Command{
Use: "c-bus [pcapfile]",
- Short: "analyzes a pcap file using a c-bus driver",
- Long: `Analyzes a pcap file using a c-bus driver
-TODO: document me
-`,
+ Short: "Analyse a capture as Clipsal C-Bus",
+ Long: `Runs a capture through plc4x's C-Bus codec: the analyze command with the protocol
+fixed, and with the C-Bus flags available.
+
+C-Bus is stateful, so how a message parses depends on the options the interface was configured
+with. The --cbus-* flags describe that configuration -- connect, smart, idmon, exstat, monitor,
+monall, pun, pcn and srchk -- and a capture taken from an interface in one mode will not parse
+correctly as another. --default-cbus-filter changes which packets are considered at all; it
+defaults to "tcp port 10001".
+
+An interrupt stops the run and keeps the counts gathered so far.`,
Args: func(cmd *cobra.Command, args []string) error {
if len(args) < 1 {
return errors.New("requires exactly one arguments")
@@ -50,11 +55,7 @@ TODO: document me
},
RunE: func(cmd *cobra.Command, args []string) error {
pcapFile := args[0]
- if err := analyzer.Analyze(pcapFile, protocol.CBus.Name); err != nil {
- return err
- }
- _, _ = fmt.Fprintln(cmd.OutOrStdout(), "Done")
- return nil
+ return analyse(cmd, pcapFile, protocol.CBus.Name)
},
}
diff --git a/plc4go/tools/plc4xpcapanalyzer/cmd/extract.go b/plc4go/tools/plc4xpcapanalyzer/cmd/extract.go
index b7ecc5b9..2b216f40 100644
--- a/plc4go/tools/plc4xpcapanalyzer/cmd/extract.go
+++ b/plc4go/tools/plc4xpcapanalyzer/cmd/extract.go
@@ -20,24 +20,31 @@
package cmd
import (
- "fmt"
"os"
"github.com/apache/plc4x/plc4go/spi/errors"
"github.com/spf13/cobra"
"github.com/apache/plc4x-extras/plc4go/tools/plc4xpcapanalyzer/config"
- "github.com/apache/plc4x-extras/plc4go/tools/plc4xpcapanalyzer/internal/extractor"
"github.com/apache/plc4x-extras/plc4go/tools/plc4xpcapanalyzer/internal/protocol"
)
// extractCmd represents the extract command
var extractCmd = &cobra.Command{
Use: "extract [protocolType] [pcapfile]",
- Short: "extract a pcap file using a driver supplied driver",
- Long: `Extract a pcap file using a driver
-TODO: document me
-`,
+ Short: "Dump the application payloads of a capture",
+ Long: `Writes out the application payload of each packet in a capture, without parsing it.
+
+This is the step before analyze: it shows what the codec is going to be handed, which is what
+you want when a capture is not being read the way you expect and the question is whether the
+payloads or the codec are at fault.
+
+The payloads are only printed at verbosity two or above, so pass -vv. Without it the command
+walks the capture and prints nothing, which looks like a failure and is not one.
+
+The protocol is bacnetip or c-bus; bacnet and cbus are accepted as aliases.
+
+An interrupt stops the run.`,
Args: func(cmd *cobra.Command, args []string) error {
if len(args) < 2 {
return errors.New("requires exactly two arguments")
@@ -54,11 +61,7 @@ TODO: document me
RunE: func(cmd *cobra.Command, args []string) error {
protocolType := args[0]
pcapFile := args[1]
- if err := extractor.Extract(pcapFile, protocolType); err != nil {
- return err
- }
- _, _ = fmt.Fprintln(cmd.OutOrStdout(), "Done")
- return nil
+ return extract(cmd, pcapFile, protocolType)
},
}
diff --git a/plc4go/tools/plc4xpcapanalyzer/cmd/interrupt.go b/plc4go/tools/plc4xpcapanalyzer/cmd/interrupt.go
new file mode 100644
index 00000000..54762669
--- /dev/null
+++ b/plc4go/tools/plc4xpcapanalyzer/cmd/interrupt.go
@@ -0,0 +1,98 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * https://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+package cmd
+
+import (
+ "context"
+ "fmt"
+ "os"
+ "os/signal"
+ "syscall"
+
+ "github.com/spf13/cobra"
+
+ "github.com/apache/plc4x-extras/plc4go/tools/plc4xpcapanalyzer/internal/analyzer"
+ "github.com/apache/plc4x-extras/plc4go/tools/plc4xpcapanalyzer/internal/extractor"
+)
+
+// The analysis and the extraction are reached through variables so that a test can see what
+// context the command hands them. That is the thing worth pinning: the loops themselves have
+// always checked their context between packets, but the entry points passed context.TODO, so
+// the check could never fire and the abort was implemented and unreachable. A test that only
+// reads the command's output cannot tell the difference, because the message it prints comes
+// from the same context the command cancelled.
+var (
+ runAnalysis = analyzer.Analyze
+ runExtractor = extractor.Extract
+)
+
+// interruptible returns a context cancelled by the first interrupt, and a function releasing
+// the handler.
+//
+// The analyzer and the extractor both check their context between packets and stop cleanly,
+// keeping whatever they have already gathered. Nothing ever gave them a cancellable one: the
+// entry points passed context.TODO and the root passes context.Background, so the abort was
+// implemented and could not be asked for. A capture of any size therefore had exactly one way
+// to stop, which was for the process to be killed part way through a report.
+//
+// Installed per command rather than on the root, and deliberately. The ui subcommand runs a
+// terminal interface that reads ctrl+c as a key press and asks before it exits; cancelling its
+// context from underneath would take that decision away from the user.
+func interruptible(cmd *cobra.Command) (context.Context, func()) {
+ return signal.NotifyContext(cmd.Context(), os.Interrupt, syscall.SIGTERM)
+}
+
+// analyse is the body every analysing command shares: install the interrupt handler, run the
+// analysis under the context it produces, and say how the run ended.
+func analyse(cmd *cobra.Command, pcapFile, protocolName string) error {
+ ctx, release := interruptible(cmd)
+ defer release()
+
+ if err := runAnalysis(ctx, pcapFile, protocolName); err != nil {
+ return err
+ }
+ reportOutcome(cmd, ctx)
+ return nil
+}
+
+// extract is the same for the extracting command.
+func extract(cmd *cobra.Command, pcapFile, protocolName string) error {
+ ctx, release := interruptible(cmd)
+ defer release()
+
+ if err := runExtractor(ctx, pcapFile, protocolName); err != nil {
+ return err
+ }
+ reportOutcome(cmd, ctx)
+ return nil
+}
+
+// reportOutcome says how a run ended, which is not always how it was asked to end.
+//
+// "Done" used to be printed unconditionally. Now that an interrupt actually reaches the loop,
+// printing it for a run the user stopped would claim a complete report where there is a partial
+// one -- the same class of untruth as a help entry for a key that does nothing.
+func reportOutcome(cmd *cobra.Command, ctx context.Context) {
+ if ctx.Err() != nil {
+ _, _ = fmt.Fprintln(cmd.OutOrStdout(), "Aborted")
+ return
+ }
+ _, _ = fmt.Fprintln(cmd.OutOrStdout(), "Done")
+}
diff --git a/plc4go/tools/plc4xpcapanalyzer/cmd/interrupt_test.go b/plc4go/tools/plc4xpcapanalyzer/cmd/interrupt_test.go
new file mode 100644
index 00000000..72b80872
--- /dev/null
+++ b/plc4go/tools/plc4xpcapanalyzer/cmd/interrupt_test.go
@@ -0,0 +1,197 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * https://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+package cmd
+
+import (
+ "bytes"
+ "context"
+ "strings"
+ "testing"
+ "time"
+
+ "github.com/spf13/cobra"
+ "github.com/stretchr/testify/assert"
+ "github.com/stretchr/testify/require"
+)
+
+// An interrupt has to reach the analysis loop, and the outcome has to say which way the run
+// ended. Both were broken in the same way: the loop checked a context nothing could cancel, and
+// the command printed "Done" whatever happened.
+
+// TestInterruptibleCancelsWithItsParent checks the returned context is derived from the
+// command's, so an already-cancelled command does not start work at all.
+func TestInterruptibleCancelsWithItsParent(t *testing.T) {
+ parent, cancel := context.WithCancel(context.Background())
+ command := &cobra.Command{}
+ command.SetContext(parent)
+
+ ctx, release := interruptible(command)
+ defer release()
+ require.NoError(t, ctx.Err(), "a live parent means a live context")
+
+ cancel()
+ <-ctx.Done()
+ assert.Error(t, ctx.Err(), "cancelling the parent has to cancel the run")
+}
+
+// TestReportOutcomeDistinguishesAnAbortFromCompletion is the honesty half. "Done" used to be
+// printed unconditionally, so once an interrupt could actually stop the loop it would have
+// claimed a complete report over a partial one.
+func TestReportOutcomeDistinguishesAnAbortFromCompletion(t *testing.T) {
+ for name, test := range map[string]struct {
+ ctx func() context.Context
+ want string
+ }{
+ "completed": {
+ ctx: context.Background,
+ want: "Done",
+ },
+ "interrupted": {
+ ctx: func() context.Context {
+ ctx, cancel := context.WithCancel(context.Background())
+ cancel()
+ return ctx
+ },
+ want: "Aborted",
+ },
+ } {
+ out := &bytes.Buffer{}
+ command := &cobra.Command{}
+ command.SetOut(out)
+
+ reportOutcome(command, test.ctx())
+
+ assert.Equal(t, test.want, strings.TrimSpace(out.String()), name)
+ }
+}
+
+// TestEveryCommandDocumentsItself is the same rule as the interfaces' help: a placeholder is
+// worse than nothing, because it looks like documentation. Six commands shipped with "TODO:
+// document me" as their long help, and the ui command's described the bacnet command instead.
+func TestEveryCommandDocumentsItself(t *testing.T) {
+ var walk func(command *cobra.Command)
+ seen := 0
+ walk = func(command *cobra.Command) {
+ if command.Name() != "help" && command.Name() != "completion" {
+ seen++
+ t.Run(command.CommandPath(), func(t *testing.T) {
+ assert.NotEmpty(t, command.Short, "needs a one-line description")
+ require.NotEmpty(t, command.Long, "needs a long description")
+ for _, placeholder := range []string{"TODO", "todo", "document me", "describe me"} {
+ assert.NotContains(t, command.Long, placeholder,
+ "a placeholder reads as documentation and is worse than none")
+ assert.NotContains(t, command.Short, placeholder)
+ }
+ assert.Greater(t, len(command.Long), len(command.Short),
+ "the long description should say more than the short one")
+ })
+ }
+ for _, child := range command.Commands() {
+ walk(child)
+ }
+ }
+ walk(rootCmd)
+ assert.Positive(t, seen, "the tree has to have commands in it")
+}
+
+// TestEveryRunCommandHandsDownTheCommandsContext is the guard for the original defect: the
+// loops have always checked their context between packets, but the entry points passed
+// context.TODO, so the check could never fire. A test reading only the command's output cannot
+// see that -- the message it prints comes from the same context the command cancelled -- so
+// these swap the seam and inspect the context while the run is still in progress. It has to be
+// inspected there: the interrupt handler is released when the command returns, which cancels
+// the context, so afterwards every context looks cancelled whatever it was derived from.
+func TestEveryRunCommandHandsDownTheCommandsContext(t *testing.T) {
+ capture := existingPcapPath(t)
+
+ for name, test := range map[string]struct {
+ args []string
+ seam func(func(context.Context)) func()
+ }{
+ // Four distinct commands, and distinct on purpose. "analyze bacnet" resolves to the
+ // bacnet subcommand rather than to analyze's own body, so reaching the generic path
+ // takes a protocol name that is not also a subcommand -- bacnetip. Running the same
+ // command twice would not work either: cobra hands a child the context from the
+ // previous Execute, so the second run would start already cancelled.
+ "analyze": {args: []string{"analyze", "bacnetip", capture}, seam: withAnalysisSeam},
+ "analyze bacnet": {args: []string{"analyze", "bacnet", capture}, seam: withAnalysisSeam},
+ "analyze c-bus": {args: []string{"analyze", "c-bus", capture}, seam: withAnalysisSeam},
+ "extract": {args: []string{"extract", "c-bus", capture}, seam: withExtractionSeam},
+ } {
+ t.Run(name, func(t *testing.T) {
+ parent, cancel := context.WithCancel(context.Background())
+ var live, derived, called bool
+
+ restore := test.seam(func(ctx context.Context) {
+ called = true
+ live = ctx.Err() == nil
+ // Cancelling the command's context has to cancel the one the run was given.
+ // Bounded: a context that is not derived from the command's never becomes
+ // done, and waiting on it unbounded would hang the suite rather than fail it.
+ cancel()
+ select {
+ case <-ctx.Done():
+ derived = true
+ case <-time.After(2 * time.Second):
+ derived = false
+ }
+ })
+ defer restore()
+
+ out := &bytes.Buffer{}
+ rootCmd.SetOut(out)
+ rootCmd.SetErr(out)
+ rootCmd.SetArgs(test.args)
+ t.Cleanup(func() {
+ rootCmd.SetOut(nil)
+ rootCmd.SetErr(nil)
+ rootCmd.SetArgs(nil)
+ })
+
+ require.NoError(t, rootCmd.ExecuteContext(parent))
+ require.True(t, called, "the command has to run the work at all")
+ assert.True(t, live, "the run has to start with a context that is not already done")
+ assert.True(t, derived,
+ "the context handed down has to be the command's, not a fresh background one")
+ assert.Contains(t, out.String(), "Aborted",
+ "and a run stopped part way has to say so rather than reporting Done")
+ })
+ }
+}
+
+// withAnalysisSeam replaces the analysis with an observer of the context it is handed.
+func withAnalysisSeam(observe func(context.Context)) func() {
+ saved := runAnalysis
+ runAnalysis = func(ctx context.Context, _, _ string) error {
+ observe(ctx)
+ return nil
+ }
+ return func() { runAnalysis = saved }
+}
+
+// withExtractionSeam does the same for the extraction.
+func withExtractionSeam(observe func(context.Context)) func() {
+ saved := runExtractor
+ runExtractor = func(ctx context.Context, _, _ string) error {
+ observe(ctx)
+ return nil
+ }
+ return func() { runExtractor = saved }
+}
diff --git a/plc4go/tools/plc4xpcapanalyzer/cmd/root.go b/plc4go/tools/plc4xpcapanalyzer/cmd/root.go
index 967e8014..1d9dc1b5 100644
--- a/plc4go/tools/plc4xpcapanalyzer/cmd/root.go
+++ b/plc4go/tools/plc4xpcapanalyzer/cmd/root.go
@@ -39,8 +39,23 @@ import (
// rootCmd represents the base command when called without any subcommands
var rootCmd = &cobra.Command{
Use: "plc4xpcapanalyzer",
- Short: "analyzes pcaps with plc4x",
- Long: `todo describe me`,
+ Short: "Analyse captured protocol traffic with plc4x's own codecs",
+ Long: `plc4xpcapanalyzer replays captured protocol traffic through plc4x's own codecs and
+reports what they cannot handle.
+
+For each packet it feeds the application payload into a real plc4x codec, re-serializes the
+message that comes back, and compares the bytes against the original. A mismatch means plc4x's
+reader and writer disagree about the same message, so the loop doubles as a regression harness
+for the codecs themselves.
+
+ plc4xpcapanalyzer analyze <protocol> <capture> parse, reserialize and compare
+ plc4xpcapanalyzer extract <protocol> <capture> dump the application payloads
+ plc4xpcapanalyzer ui [capture] the terminal interface
+
+The protocols are bacnetip and c-bus; bacnet and cbus are accepted as aliases. Reading a capture
+needs libpcap, because it goes through gopacket/pcap.
+
+With no capture to hand, "ui --demo" generates one and analyses it.`,
// Uncomment the following line if your bare application
// has an action associated with it:
// Run: func(cmd *cobra.Command, args []string) { },
diff --git a/plc4go/tools/plc4xpcapanalyzer/cmd/ui.go b/plc4go/tools/plc4xpcapanalyzer/cmd/ui.go
index fcd34b44..5bcd0820 100644
--- a/plc4go/tools/plc4xpcapanalyzer/cmd/ui.go
+++ b/plc4go/tools/plc4xpcapanalyzer/cmd/ui.go
@@ -32,10 +32,19 @@ import (
// uiCmd represents the ui command
var uiCmd = &cobra.Command{
Use: "ui [pcapfile]",
- Short: "Start the ui with optional pcapfile",
- Long: `Analyzes a pcap file using a bacnet driver
-TODO: document me
-`,
+ Short: "Start the terminal interface, optionally on a capture",
+ Long: `Opens the terminal interface, on the given capture if one is named.
+
+The interface does what the command line does and shows the result rather than printing it: the
+packet list with a verdict against each, and a detail pane with the raw bytes, the parsed tree
+and, where the round trip failed, a diff of the original bytes against the reserialized ones
+with the first differing offset marked.
+
+ --demo generate a small capture of real C-Bus traffic and analyse it immediately, so the
+ tool can be tried out and manually debugged with no capture and no device at hand
+
+Press ? for the keys. Ctrl+C stops a run in progress rather than the session; with nothing
+running it offers to quit.`,
Args: func(_ *cobra.Command, args []string) error {
if len(args) < 1 {
return nil
diff --git a/plc4go/tools/plc4xpcapanalyzer/internal/analyzer/analyzer.go b/plc4go/tools/plc4xpcapanalyzer/internal/analyzer/analyzer.go
index 40cec06b..6be247b0 100644
--- a/plc4go/tools/plc4xpcapanalyzer/internal/analyzer/analyzer.go
+++ b/plc4go/tools/plc4xpcapanalyzer/internal/analyzer/analyzer.go
@@ -81,12 +81,17 @@ func (o Options) reporter() progress.Reporter {
return progress.ForWriter(o.Stderr, config.RootConfigInstance.HideProgressBar, theme)
}
-func Analyze(pcapFile, protocolType string) error {
- return AnalyzeWithOutput(pcapFile, protocolType, os.Stdout, os.Stderr)
+// Analyze runs the parse, reserialize and compare loop over a capture.
+//
+// The context is honoured: the loop checks it between packets and stops cleanly, keeping the
+// counts it has already gathered. It used to be context.TODO here, which made the check inside
+// the loop unreachable -- the abort was implemented and could never be asked for.
+func Analyze(ctx context.Context, pcapFile, protocolType string) error {
+ return AnalyzeWithOutput(ctx, pcapFile, protocolType, os.Stdout, os.Stderr)
}
-func AnalyzeWithOutput(pcapFile, protocolType string, stdout, stderr io.Writer) error {
- return AnalyzeWithOutputAndCallback(context.TODO(), pcapFile, protocolType, stdout, stderr, nil)
+func AnalyzeWithOutput(ctx context.Context, pcapFile, protocolType string, stdout, stderr io.Writer) error {
+ return AnalyzeWithOutputAndCallback(ctx, pcapFile, protocolType, stdout, stderr, nil)
}
func AnalyzeWithOutputAndCallback(ctx context.Context, pcapFile, protocolType string, stdout, stderr io.Writer, messageCallback func(parsed spi.Message)) error {
diff --git a/plc4go/tools/plc4xpcapanalyzer/internal/analyzer/analyzer_characterisation_test.go b/plc4go/tools/plc4xpcapanalyzer/internal/analyzer/analyzer_characterisation_test.go
index 2616a3bf..090b973f 100644
--- a/plc4go/tools/plc4xpcapanalyzer/internal/analyzer/analyzer_characterisation_test.go
+++ b/plc4go/tools/plc4xpcapanalyzer/internal/analyzer/analyzer_characterisation_test.go
@@ -21,6 +21,7 @@ package analyzer_test
import (
"bytes"
+ "context"
"encoding/json"
"io"
"path/filepath"
@@ -122,7 +123,7 @@ func runAnalyzeRaw(t *testing.T, pcapFile, protocolName string, configure func()
log.Logger = zerolog.New(logs).Level(zerolog.InfoLevel)
zerolog.SetGlobalLevel(zerolog.InfoLevel)
- err := analyzer.AnalyzeWithOutput(pcapFile, protocolName, io.Discard, io.Discard)
+ err := analyzer.AnalyzeWithOutput(t.Context(), pcapFile, protocolName, io.Discard, io.Discard)
got, found := lastSummary(t, logs.String())
return got, found, err
@@ -239,3 +240,69 @@ func TestAnalyzePackageNumberLimitStopsEarly(t *testing.T) {
"the loop increments before testing the limit, so it stops one past it; pinned deliberately")
assert.Less(t, got.CurrentPackageNum, len(session), "the limit must actually cut the run short")
}
+
+// TestACancelledContextStopsTheAnalysis is the half of the abort that lives here. The check
+// between packets has always been in the loop; what was missing was any way to reach it, because
+// the entry points passed context.TODO. The evidence is the analyzer's own summary: a cancelled
+// run reports a capture of eight packets and none walked, where an unhonoured cancellation
+// would walk all eight.
+//
+// It runs in this package rather than through the command, because cobra.OnInitialize replaces
+// the global logger before a command's body runs and takes the summary with it.
+func TestACancelledContextStopsTheAnalysis(t *testing.T) {
+ capture := filepath.Join(t.TempDir(), "cbus.pcap")
+ require.NoError(t, pcapfixture.WriteCBus(capture, pcapfixture.CBusSession()))
+
+ ctx, cancel := context.WithCancel(context.Background())
+ cancel()
+
+ got, found, err := runAnalyzeWith(t, ctx, capture, "c-bus")
+
+ assert.NoError(t, err, "an interrupt is not a failure")
+ require.True(t, found, "the analyzer still reports what it managed to do")
+ assert.Positive(t, got.NumberOfPackage, "the capture has packets in it to walk")
+ assert.Zero(t, got.CurrentPackageNum,
+ "a run cancelled before it started must not walk the capture")
+}
+
+// TestALiveContextRunsToTheEnd is the control. Without it the test above would pass just as well
+// against an analyzer that had stopped working altogether.
+func TestALiveContextRunsToTheEnd(t *testing.T) {
+ capture := filepath.Join(t.TempDir(), "cbus.pcap")
+ require.NoError(t, pcapfixture.WriteCBus(capture, pcapfixture.CBusSession()))
+
+ got, found, err := runAnalyzeWith(t, context.Background(), capture, "c-bus")
+
+ require.NoError(t, err)
+ require.True(t, found)
+ assert.Equal(t, got.NumberOfPackage, got.CurrentPackageNum,
+ "a live context has to let the whole capture through")
+}
+
+// runAnalyzeWith is runAnalyzeRaw with the context chosen by the caller, which is the one thing
+// the other helpers cannot vary.
+func runAnalyzeWith(t *testing.T, ctx context.Context, pcapFile, protocolName string) (summary, bool, error) {
+ t.Helper()
+
+ savedRoot, savedCBus := config.RootConfigInstance, config.CBusConfigInstance
+ savedPcap, savedAnalyze := config.PcapConfigInstance, config.AnalyzeConfigInstance
+ savedLogger, savedLevel := log.Logger, zerolog.GlobalLevel()
+ t.Cleanup(func() {
+ config.RootConfigInstance, config.CBusConfigInstance = savedRoot, savedCBus
+ config.PcapConfigInstance, config.AnalyzeConfigInstance = savedPcap, savedAnalyze
+ log.Logger = savedLogger
+ zerolog.SetGlobalLevel(savedLevel)
+ })
+ config.RootConfigInstance.HideProgressBar = true
+ // No flags are parsed here, so the limit is its zero value and the loop would stop after
+ // the first packet -- which would make the control below indistinguishable from the abort.
+ config.PcapConfigInstance.PackageNumberLimit = ^uint(0)
+
+ logs := &syncBuffer{}
+ log.Logger = zerolog.New(logs).Level(zerolog.InfoLevel)
+ zerolog.SetGlobalLevel(zerolog.InfoLevel)
+
+ err := analyzer.AnalyzeWithOutput(ctx, pcapFile, protocolName, io.Discard, io.Discard)
+ got, found := lastSummary(t, logs.String())
+ return got, found, err
+}
diff --git a/plc4go/tools/plc4xpcapanalyzer/internal/extractor/extractor.go b/plc4go/tools/plc4xpcapanalyzer/internal/extractor/extractor.go
index 17315a01..c68562ad 100644
--- a/plc4go/tools/plc4xpcapanalyzer/internal/extractor/extractor.go
+++ b/plc4go/tools/plc4xpcapanalyzer/internal/extractor/extractor.go
@@ -80,11 +80,17 @@ func (o Options) reporter() progress.Reporter {
return progress.ForWriter(o.Stderr, config.RootConfigInstance.HideProgressBar, o.themeFor(o.Stderr))
}
-func Extract(pcapFile, protocolType string) error {
- // os.Stdout and os.Stderr, rather than the go-ansi wrappers that used to be here: those
- // wrappers hid the file descriptor, so nothing downstream could tell whether it was writing
- // to a terminal, which is how the bar came to be drawn over the log in the first place.
- return ExtractWithOutput(context.TODO(), pcapFile, protocolType, os.Stdout, os.Stderr)
+// Extract dumps the application payloads of a capture.
+//
+// The context is honoured: the loop checks it between packets and stops cleanly. It used to be
+// context.TODO here, which made the check inside the loop unreachable -- the abort was
+// implemented and could never be asked for.
+//
+// os.Stdout and os.Stderr, rather than the go-ansi wrappers that used to be here: those
+// wrappers hid the file descriptor, so nothing downstream could tell whether it was writing
+// to a terminal, which is how the bar came to be drawn over the log in the first place.
+func Extract(ctx context.Context, pcapFile, protocolType string) error {
+ return ExtractWithOutput(ctx, pcapFile, protocolType, os.Stdout, os.Stderr)
}
func ExtractWithOutput(ctx context.Context, pcapFile, protocolType string, stdout, stderr io.Writer) error {