dohertj2/lmxopcua
1
0
Fork 0
Code Issues 32 Pull Requests Actions Packages Projects Releases Wiki Activity

fix(s7): validate span-read options at parse time; document the over-PDU degradation

Browse Source
This commit is contained in:
Joseph Doherty
2026-08-17 03:45:14 -04:00
parent f06cbe16d9
commit acb3772426
3 changed files with 135 additions and 10 deletions
Download Patch File Download Diff File Expand all files Collapse all files
src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.S7.Contracts/S7DriverOptions.cs
+12 -4
View File
@@ -88,10 +88,11 @@ public sealed class S7DriverOptions
/// </summary>
/// <remarks>
/// <para>
/// <b>Any value <c>&lt;= -1</c> disables span grouping entirely</b> — every tag is read
/// on its own, exactly as the driver behaved before grouping existed. That is the
/// escape hatch for a CPU that dislikes wide block reads, and the control arm the
/// parity tests read through.
/// <b><c>-1</c> disables span grouping entirely</b> — every tag is read on its own,
/// exactly as the driver behaved before grouping existed. That is the escape hatch for a
/// CPU that dislikes wide block reads, and the control arm the parity tests read through.
/// <c>-1</c> is the floor: <c>S7DriverFactoryExtensions.ParseOptions</c> rejects anything
/// below it, so a config cannot express "disabled" by accident with an arbitrary negative.
/// </para>
/// <para>
/// Grouping cannot make a read fail where the per-tag reads would have succeeded: a
@@ -121,6 +122,13 @@ public sealed class S7DriverOptions
/// one enormous request that is mostly padding. The 220-byte default sits just under the
/// usable payload of the common 240-byte S7-300/400 PDU, so a typical span is one request
/// on the wire.
/// <para>
/// There is deliberately <b>no upper bound</b>: the negotiated PDU size is opaque inside
/// S7.Net, so a value above it cannot be detected — it stays legal and simply degrades to
/// multi-request chunking inside S7.Net, costing extra wire round-trips per span rather
/// than failing. <c>S7DriverFactoryExtensions.ParseOptions</c> therefore validates only the
/// lower bound (<c>&gt;= 1</c>); a non-positive cap can never admit a single block read.
/// </para>
/// </remarks>
[Display(Name = "Span read max span (bytes)",
Description = "Largest single planned block read. Bounds wasted-gap amplification, not correctness.",
src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.S7/S7DriverFactoryExtensions.cs
+27 -6
View File
@@ -56,6 +56,26 @@ public static class S7DriverFactoryExtensions
throw new InvalidOperationException(
$"S7 driver config for '{driverInstanceId}' missing required Host");
// Block-read grouping knobs. There is no IValidateOptions machinery for S7, so this parse is
// the single choke point every deployed config passes through — validate here or a nonsensical
// value is accepted at deploy time and only misbehaves later, inside the read loop.
var spanReadMaxGapBytes = dto.SpanReadMaxGapBytes ?? S7DriverOptions.DefaultSpanReadMaxGapBytes;
var spanReadMaxSpanBytes = dto.SpanReadMaxSpanBytes ?? S7DriverOptions.DefaultSpanReadMaxSpanBytes;
if (spanReadMaxSpanBytes < 1)
throw new InvalidOperationException(
$"S7 driver config for '{driverInstanceId}' has SpanReadMaxSpanBytes {spanReadMaxSpanBytes}; " +
"must be >= 1 — it caps a single planned block read, so a non-positive cap admits none. " +
"There is no upper bound: a value above the negotiated PDU is legal and degrades to " +
"multi-request chunking inside S7.Net.");
// -1 is the documented escape hatch (span grouping disabled — S7Driver enables spans on >= 0)
// and 0 means "tolerate no gap", i.e. merge only strictly adjacent tags. Both stay legal.
if (spanReadMaxGapBytes < -1)
throw new InvalidOperationException(
$"S7 driver config for '{driverInstanceId}' has SpanReadMaxGapBytes {spanReadMaxGapBytes}; " +
"must be >= -1 — -1 disables span grouping and 0 merges only strictly adjacent tags.");
return new S7DriverOptions
{
Host = dto.Host!,
@@ -69,11 +89,11 @@ public static class S7DriverFactoryExtensions
// tags. The driver maps each RawTagEntry's TagConfig blob through S7TagDefinitionFactory
// at Initialize; the legacy pre-declared DTO tag path (Name/Address → BuildTag) is retired.
RawTags = dto.RawTags is { Count: > 0 } ? [.. dto.RawTags] : [],
// Block-read grouping knobs. Mapped explicitly like every other key on this DTO — the
// options object is never deserialised directly — so omitting them here would leave the
// documented "-1 disables grouping" escape hatch unreachable from a deployed config.
SpanReadMaxGapBytes = dto.SpanReadMaxGapBytes ?? S7DriverOptions.DefaultSpanReadMaxGapBytes,
SpanReadMaxSpanBytes = dto.SpanReadMaxSpanBytes ?? S7DriverOptions.DefaultSpanReadMaxSpanBytes,
// Mapped explicitly like every other key on this DTO — the options object is never
// deserialised directly — so omitting them here would leave the documented
// "-1 disables grouping" escape hatch unreachable from a deployed config.
SpanReadMaxGapBytes = spanReadMaxGapBytes,
SpanReadMaxSpanBytes = spanReadMaxSpanBytes,
Probe = new S7ProbeOptions
{
Enabled = dto.Probe?.Enabled ?? true,
@@ -128,7 +148,8 @@ public static class S7DriverFactoryExtensions
/// <summary>Gets the probe configuration.</summary>
public S7ProbeDto? Probe { get; init; }
/// <summary>Gets the largest hole the read planner reads through when merging tags into one
/// block request; any value below zero disables block-read grouping. Null keeps the default.</summary>
/// block request; <c>-1</c> disables block-read grouping and anything below it is rejected.
/// Null keeps the default.</summary>
public int? SpanReadMaxGapBytes { get; init; }
/// <summary>Gets the largest single planned block read in bytes. Null keeps the default.</summary>
public int? SpanReadMaxSpanBytes { get; init; }
tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.S7.Tests/S7SpanOptionValidationTests.cs
+96
View File
@@ -0,0 +1,96 @@
using Shouldly;
using Xunit;
namespace ZB.MOM.WW.OtOpcUa.Driver.S7.Tests;
/// <summary>
/// Config-time guard on the two span-read knobs. There is no <c>IValidateOptions</c> machinery
/// for S7 — <see cref="S7DriverFactoryExtensions.ParseOptions"/> is the single choke point every
/// deployed config passes through — so a nonsensical value used to be accepted at deploy time and
/// only misbehave later, in the read loop, on a driver that had already reported healthy.
/// </summary>
/// <remarks>
/// The legal-value cases matter as much as the rejections: <c>-1</c> on the gap is the documented
/// escape hatch that disables grouping entirely (<c>S7Driver</c> enables spans on <c>&gt;= 0</c>),
/// and <c>0</c> means "tolerate no gap at all", i.e. merge only strictly adjacent tags. A guard
/// written as <c>&lt; 0</c> would break both.
/// </remarks>
public sealed class S7SpanOptionValidationTests
{
/// <summary>A driver-config blob carrying the given span knob, host included (it is required).</summary>
private static string Config(string knob, int value) =>
"{\"host\":\"10.0.0.1\",\"" + knob + "\":" + value.ToString(System.Globalization.CultureInfo.InvariantCulture) + "}";
/// <summary>A span cap below one can never admit a single planned block read, so it is rejected.</summary>
[Theory]
[InlineData(0)]
[InlineData(-1)]
[InlineData(-220)]
public void A_span_cap_below_one_is_rejected(int maxSpanBytes)
{
var ex = Should.Throw<InvalidOperationException>(() => S7DriverFactoryExtensions.ParseOptions(
"s7-cfg", Config("spanReadMaxSpanBytes", maxSpanBytes)));
ex.Message.ShouldContain("SpanReadMaxSpanBytes");
ex.Message.ShouldContain("s7-cfg");
}
/// <summary>A gap budget below the grouping-disabled sentinel is rejected.</summary>
[Theory]
[InlineData(-2)]
[InlineData(-16)]
public void A_gap_budget_below_minus_one_is_rejected(int maxGapBytes)
{
var ex = Should.Throw<InvalidOperationException>(() => S7DriverFactoryExtensions.ParseOptions(
"s7-cfg", Config("spanReadMaxGapBytes", maxGapBytes)));
ex.Message.ShouldContain("SpanReadMaxGapBytes");
ex.Message.ShouldContain("s7-cfg");
}
/// <summary>
/// The two legal edge values survive: <c>-1</c> (grouping disabled) and <c>0</c> (no gap
/// tolerated). Both are documented behaviour, not degenerate input.
/// </summary>
[Theory]
[InlineData(-1)]
[InlineData(0)]
public void The_documented_gap_edge_values_are_accepted(int maxGapBytes)
{
var parsed = S7DriverFactoryExtensions.ParseOptions("s7-cfg", Config("spanReadMaxGapBytes", maxGapBytes));
parsed.SpanReadMaxGapBytes.ShouldBe(maxGapBytes);
}
/// <summary>A one-byte span cap is the smallest legal cap and must not trip the guard.</summary>
[Fact]
public void The_smallest_legal_span_cap_is_accepted()
{
var parsed = S7DriverFactoryExtensions.ParseOptions("s7-cfg", Config("spanReadMaxSpanBytes", 1));
parsed.SpanReadMaxSpanBytes.ShouldBe(1);
}
/// <summary>
/// An over-PDU span cap is legal — S7.Net transparently chunks an oversized <c>ReadBytes</c>
/// into several wire requests — so the guard must not invent an upper bound it cannot know
/// (the negotiated PDU size is opaque inside S7.Net).
/// </summary>
[Fact]
public void An_over_pdu_span_cap_is_accepted_because_S7Net_chunks_it()
{
var parsed = S7DriverFactoryExtensions.ParseOptions("s7-cfg", Config("spanReadMaxSpanBytes", 4096));
parsed.SpanReadMaxSpanBytes.ShouldBe(4096);
}
/// <summary>Omitting both knobs keeps the shipped defaults, which must themselves pass the guard.</summary>
[Fact]
public void The_shipped_defaults_pass_the_guard()
{
var parsed = S7DriverFactoryExtensions.ParseOptions("s7-cfg", "{\"host\":\"10.0.0.1\"}");
parsed.SpanReadMaxGapBytes.ShouldBe(S7DriverOptions.DefaultSpanReadMaxGapBytes);
parsed.SpanReadMaxSpanBytes.ShouldBe(S7DriverOptions.DefaultSpanReadMaxSpanBytes);
}
}