GVCP support by tigercosmos · Pull Request #1357 · seladb/PcapPlusPlus

tigercosmos · 2024-04-12T09:31:26Z

Related to #1321

This PR adds a layer for the GigE Vision Control Protocol (GVCP). The layer supports parsing, creation and editing of GVCP messages on UDP port 3956.

Design

GvcpRequestLayer and GvcpAcknowledgeLayer represent all messages. Discovery and Force IP messages have their own layer classes.
A layer holds the GVCP header and the message fields that its class knows. parseNextLayer() puts the rest of the message in a PayloadLayer.
computeCalculateFields() sets the data size field to the size of the data after the header.
The header and body structs are protected or private members of the layer classes.
The request and acknowledge constructors throw std::invalid_argument for a value of the wrong kind. Command values are even, and acknowledge values are odd.
GvcpDiscoveryRequestLayer exposes the allow-broadcast-acknowledge flag.
operator<< prints commands and statuses by name. Values that the spec does not define map to Unknown.
UdpLayer falls back to PayloadLayer if the data is not valid GVCP.
The protocol type is 65, because GTPv2 uses 57.

Tests

The tests cover parsing, creation, editing and malformed data.

tigercosmos · 2024-08-12T08:53:44Z

@seladb The PR is ready. Please take a look.

codecov · 2024-08-12T09:17:23Z

Codecov Report

❌ Patch coverage is 92.33193% with 73 lines in your changes missing coverage. Please review.
✅ Project coverage is 82.82%. Comparing base (ad344a8) to head (b17ec4a).
⚠️ Report is 3 commits behind head on dev.

Files with missing lines	Patch %	Lines
Packet++/src/GvcpLayer.cpp	87.07%	72 Missing ⚠️
Packet++/header/GvcpLayer.h	98.71%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##              dev    #1357      +/-   ##
==========================================
+ Coverage   82.61%   82.82%   +0.21%     
==========================================
  Files         339      342       +3     
  Lines       61875    62972    +1097     
  Branches    13036    12947      -89     
==========================================
+ Hits        51115    52159    +1044     
- Misses       9877     9930      +53     
  Partials      883      883

Flag	Coverage Δ
23.11.6	`6.95% <0.15%> (-0.15%)`	⬇️
24.11.5	`6.93% <0.15%> (-0.14%)`	⬇️
25.11.1	`6.93% <0.15%> (-0.17%)`	⬇️
alpine320	`76.84% <85.46%> (+0.26%)`	⬆️
fedora42	`76.37% <84.43%> (+0.23%)`	⬆️
macos-15	`82.58% <94.46%> (+0.24%)`	⬆️
macos-26	`82.59% <94.46%> (+0.25%)`	⬆️
macos-26-intel	`82.51% <94.46%> (+0.24%)`	⬆️
mingw32	`71.16% <73.41%> (+0.09%)`	⬆️
mingw64	`70.84% <73.56%> (+0.32%)`	⬆️
npcap	`?`
rhel94	`76.18% <84.43%> (+0.26%)`	⬆️
ubuntu2204	`76.19% <84.40%> (+0.22%)`	⬆️
ubuntu2404	`76.47% <84.40%> (+0.21%)`	⬆️
ubuntu2604	`76.46% <84.37%> (+0.24%)`	⬆️
ubuntu2604-arm64	`76.33% <84.75%> (+0.25%)`	⬆️
ubuntu2604-icpx	`59.15% <66.32%> (+0.14%)`	⬆️
unittest	`82.82% <92.33%> (+0.21%)`	⬆️
windows-2022	`85.67% <92.13%> (+0.32%)`	⬆️
windows-2025	`85.37% <91.21%> (+0.28%)`	⬆️
winpcap	`85.72% <92.13%> (+0.43%)`	⬆️
xdp	`54.27% <84.40%> (+0.57%)`	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

tigercosmos · 2024-08-13T04:38:44Z

Don't know why the fuzz tests failed?

seladb

I just started reviewing this PR because I still need to learn this protocol. But here are a few general comments

seladb · 2024-08-14T06:41:24Z

+
+		GvcpCommand getCommand() const
+		{
+			return static_cast<GvcpCommand>(netToHost16(command));


I think we can move all implementations that use netToHost16 to GvcpLayer.cpp so we don't expose SystemUtils.h in the header file

Since it's one line function, I think it's clearer to be in the header. I forward declare the function, so we don't need to include the whole header file.

I don't think it's a good practice to put this in the header file, I vote we don't forward declare and move the implementation to the cpp file

seladb

@tigercosmos here is my suggestion of the structure we can use, which follows ideas implemented in other PcapPlusPlus layers. This code snippet is just the high level interface and doesn't include all the methods, implementation and lower level details, but it's mostly to demonstrate my suggestion.

Please let me know what you think and we can discuss it further.

struct gvcp_command_header
{
	uint8_t gcvpCommand = internal::kGvcpMagicNumber;  // always fixed
	uint8_t flag = 0;  // 0-3 bits are specified by each command, 4-6 bits are reserved, 7 bit is acknowledge
	uint16_t command = 0;
	uint16_t dataSize = 0;
	uint16_t requestId = 0;
};

struct gvcp_ack_header
{
	uint16_t status = 0;
	uint16_t command = 0;
	uint16_t dataSize = 0;
	uint16_t ackId = 0;
};

struct gvcp_discovery_message : public gvcp_command_header
{
};

struct gvcp_discovery_ack_message : public gvcp_ack_header
{
	uint16_t versionMajor = 0;
	uint16_t versionMinor = 0;
	uint32_t deviceMode = 0;
	uint16_t reserved = 0;
	uint8_t macAddress[6] = { 0 };
	uint32_t supportedIpConfigOptions = 0;
	uint32_t ipConfigCurrent = 0;
	uint8_t reserved2[12] = { 0 };
	uint32_t ipAddress = 0;
	uint8_t reserved3[12];
	uint32_t subnetMask = 0;
	uint8_t reserved4[12] = { 0 };
	uint32_t defaultGateway = 0;
	char manufacturerName[32] = { 0 };
	char modelName[32] = { 0 };
	char deviceVersion[32] = { 0 };
	char manufacturerSpecificInformation[48] = { 0 };
	char serialNumber[16] = { 0 };
	char userDefinedName[16] = { 0 };
};

class GvcpLayer : public Layer
{
public:
	GvcpLayer() = delete;

	static bool isGvcpPort(uint16_t port);
	static GvcpLayer* parseGvcpLayer(uint8_t* data, size_t dataLen, Layer* prevLayer, Packet* packet);

	// implement Layer's abstract methods
	void parseNextLayer() override;
	void computeCalculateFields() override;
	OsiModelLayer getOsiModelLayer() const override;
};

class GvcpRequestLayer : public GvcpLayer
{
public:
	GvcpRequestLayer() = delete;

	static bool isDataValid(const uint8_t* data, size_t dataLen);

	GvcpFlag getFlag() const;
	void setFlag(const GvcpFlag flag);

	GvcpCommand getCommand() const;

	uint16_t getLength() const;

	uint16_t getRequestId() const;
	void setRequestId(uint16_t requestId);
};

class GvcpAckLayer : public GvcpLayer
{
public:
	GvcpAckLayer() = delete;

	static bool isDataValid(const uint8_t* data, size_t dataLen);

	GvcpResponseStatus getStatus() const;
	void setStatus(GvcpResponseStatus status);

	GvcpCommand getCommand() const;

	uint16_t getLength() const;

	uint16_t getAckId() const;
	void setAckId(uint16_t ackId);
};

class GvcpDiscoveryRequestLayer : public GvcpRequestLayer
{
public:
	// constructors

	GvcpDiscoveryRequestLayer(uint8_t* data, size_t dataLen, Layer* prevLayer, Packet* packet);
	GvcpDiscoveryRequestLayer();

	// implement Layer's abstract methods

	std::string toString() const override;
	size_t getHeaderLen() const override;
};

class GvcpDiscoveryAckLayer : public GvcpAckLayer
{
	// constructors

	GvcpDiscoveryAckLayer(uint8_t* data, size_t dataLen, Layer* prevLayer, Packet* packet);
	GvcpDiscoveryAckLayer(/** a list of all params: uint16_t majorVersion, uint16_t minorVersion, uint32_t deviceMode, ...*/);

	// getters and setters

	std::pair<uint16_t, uint16_t> getVersion() const;

	pcpp::MacAddress getMacAddress() const;
	void setMacAddress(const pcpp::MacAddress macAddress);

	// ...

	// implement Layer's abstract methods

	std::string toString() const override;
	size_t getHeaderLen() const override;
};

class GvcpForceIpRequestLayer : public GvcpRequestLayer
{
	// ...
};

class GvcpForceIpAcktLayer : public GvcpAckLayer
{
	// ...
};

tigercosmos · 2024-08-18T11:36:56Z

@seladb Reading your sample code, basically I got your intention. So "Discovery" and "ForceIP" have their own layer, and for other commands, they will fall back to general GVCPRequestLayer and GvcpAcknowledgeLayer. I don't see a problem for now, and I will try to refactor the PR based on this and see if I encounter any issues.

seladb · 2024-08-18T23:11:57Z

@seladb Reading your sample code, basically I got your intention. So "Discovery" and "ForceIP" have their own layer, and for other commands, they will fall back to general GVCPRequestLayer and GvcpAcknowledgeLayer. I don't see a problem for now, and I will try to refactor the PR based on this and see if I encounter any issues.

Yes, other messages should fall under GvcpRequestLayer or GvcpAckLayer. In that way we can create specific layers for more messages in the future

tigercosmos · 2024-09-26T01:53:01Z

@seladb The code has been updated, could you take a look?

tigercosmos · 2024-10-02T04:38:56Z

@seladb ping.

seladb · 2024-10-02T07:40:25Z

@seladb ping.

Sorry for the delay, I'll review it soon

seladb · 2024-10-02T07:58:18Z

@tigercosmos I didn't review the entire PR because this comment wasn't addressed yet. Can you please address this and I'll review the rest of the PR?

tigercosmos · 2024-10-02T08:22:47Z

@tigercosmos I didn't review the entire PR because this comment wasn't addressed yet. Can you please address this and I'll review the rest of the PR?

@seladb What do you mean not changed? I have changed all layers. Or do you mean putting the header definition inside the layer? I don't do that because the layer will be too big.

seladb · 2024-10-09T07:58:59Z

+
+		GvcpCommand getCommand() const
+		{
+			return static_cast<GvcpCommand>(netToHost16(command));


I don't think it's a good practice to put this in the header file, I vote we don't forward declare and move the implementation to the cpp file

seladb · 2024-10-09T08:01:21Z

+		GvcpRequestHeader() = default;
+
+		GvcpRequestHeader(GvcpFlag flag, GvcpCommand command, uint16_t dataSize, uint16_t requestId)
+		    : flag(flag), command(hostToNet16(static_cast<uint16_t>(command))), dataSize(hostToNet16(dataSize)),
+		      requestId(hostToNet16(requestId))
+		{}


Why do we need c'tors for a struct? 🤔

What we usually do is declare all properties as public and then we don't need c'tors.
These structs are internal to the GVCP layers classes and shouldn't be exposed outside, so it doesn't matter if their properties are public.

If you want you can move these structs as protected inside of the GVCP layer classes

I added a constructor for it because the byte order will change, and it's better to handle it by the constructor. In addition, GvcpRequestHeader is public in purpose here. Users are free to get the raw header if they want, so there is also a public function getGvcpHeader() in the layers. I don't think we should stop users from getting the header.

I think I mentioned it in some comments, but I'm not sure.... In my opinion there is no need to expose both getGvcpHeader() and getters/setters for each property, that will create a somewhat confusing API.

In older protocols we used to only expose the "raw header" (like getGvcpHeader()), but in newer protocols we usually expose getters and setters which I think provides a cleaner API. We also got feedback from users that they prefer the getters/setters over the "raw header"

Sure, let me not expose it.

they are under internal now

seladb · 2024-10-09T08:26:18Z

+		 */
+		GvcpCommand getCommand() const
+		{
+			return static_cast<GvcpCommand>(netToHost16(getGvcpHeader()->command));


We can do:

return getGvcpHeader()->getCommand();

seladb · 2024-10-09T08:27:09Z

+		 * @param[in] data A pointer to the data including the header and the payload
+		 * @param[in] dataSize The size of the data in bytes
+		 */
+		GvcpAcknowledgeLayer(const uint8_t* data, size_t dataSize);


ditto: why do we need this c'tor if we have the c'tor below?

seladb · 2024-10-09T08:27:38Z

+		GvcpAckHeader* getGvcpHeader() const
+		{
+			return reinterpret_cast<GvcpAckHeader*>(m_Data);  // the header is at the beginning of the data
+		}


ditto: this method can be private/protected

seladb · 2024-10-09T08:28:32Z

+		 */
+		GvcpResponseStatus getStatus() const
+		{
+			return static_cast<GvcpResponseStatus>((netToHost16(getGvcpHeader()->status)));


This method should handle cases where getGvcpHeader()->status is an unexpected value and return GvcpResponseStatus::Unknown

seladb · 2024-10-09T08:29:00Z

+		 */
+		GvcpCommand getCommand() const
+		{
+			return static_cast<GvcpCommand>(netToHost16(getGvcpHeader()->command));


ditto: we can do:

return getGvcpHeader()->getCommand();

seladb · 2024-10-23T05:18:21Z

@tigercosmos a gentle reminder to address the PR comments ☝️

tigercosmos · 2024-10-23T05:51:35Z

@tigercosmos a gentle reminder to address the PR comments ☝️

Thanks. I will continue on this after October. Quite busy these days.

tigercosmos · 2024-11-26T08:31:58Z

Ops, I didn't notice this. Changed.

seladb · 2025-03-20T05:33:14Z

@tigercosmos this PR is open for a long time, do you plan to continue working on it or should we close it?

tigercosmos · 2025-03-20T15:29:06Z

Let's close it.

Rework of seladb#1357 on top of the current dev branch, addressing the open review comments: - Keep the wire structs plain (no constructors/methods) under pcpp::internal and move all byte-order handling to GvcpLayer.cpp. - Drop the raw-buffer copy constructors; layers are built either from a parsed packet or from typed arguments. - getHeaderLen() covers the whole GVCP message since GVCP is always the last layer; message data is exposed via getPayloadData(). - getCommand()/getStatus() map unexpected values to Unknown using a switch instead of a global unordered_set. - parseGvcpLayer() validates the data, passes prevLayer/packet to every layer it creates and UdpLayer falls back to PayloadLayer. - Use protocol type 65 (57 is taken by GTPv2). - Fix getVersion() byte order and the unbounded reads of the fixed-size string fields of the discovery acknowledge. - Add setters and typed constructors for Discovery and Force IP messages, and creation / edit / malformed-data tests. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>

Rework of seladb#1357 on top of the current dev branch, addressing the open review comments: - Keep the wire structs plain (no constructors/methods) under pcpp::internal and move all byte-order handling to GvcpLayer.cpp. - Drop the raw-buffer copy constructors; layers are built either from a parsed packet or from typed arguments. - getHeaderLen() covers the whole GVCP message since GVCP is always the last layer; message data is exposed via getPayloadData(). - getCommand()/getStatus() map unexpected values to Unknown using a switch instead of a global unordered_set. - parseGvcpLayer() validates the data, passes prevLayer/packet to every layer it creates and UdpLayer falls back to PayloadLayer. - Use protocol type 65 (57 is taken by GTPv2). - Fix getVersion() byte order and the unbounded reads of the fixed-size string fields of the discovery acknowledge. - Add setters and typed constructors for Discovery and Force IP messages, and creation / edit / malformed-data tests.

tigercosmos

@seladb @Dimi1010 I have reworked this PR and addressed all issues above. Please help review, thanks!

Dimi1010

LGTM code wise. Tho, I haven't gone in depth into the actual spec.

tigercosmos · 2026-09-27T06:49:01Z

@seladb All issues fixed. Please take a look, thanks

Add parsing, creation and editing of GVCP messages on UDP port 3956. - GvcpRequestLayer and GvcpAcknowledgeLayer represent all messages. Discovery and Force IP messages have their own layer classes. - A layer holds the GVCP header and the message fields that its class knows. parseNextLayer() puts the rest of the message in a PayloadLayer, and computeCalculateFields() updates the data size. - The header and body structs are protected or private members of the layer classes. - The request and acknowledge constructors reject a value of the wrong kind. Command values are even, and acknowledge values are odd. - GvcpDiscoveryRequestLayer exposes the allow-broadcast-acknowledge flag. - The command and status enums are nested in GvcpLayer. They print by name, and values that the spec does not define map to Unknown. - UdpLayer falls back to PayloadLayer if the data is not valid GVCP. - The protocol type is 65, because GTPv2 uses 57.

seladb

A few small comments, otherwise LGTM

seladb · 2026-09-30T06:31:02Z

+	struct GvcpVersion
+	{
+		/// The version major number
+		uint16_t major;
+		/// The version minor number
+		uint16_t minor;
+	};


Why did we keep it outside of the layer class? It can be a public struct inside GvcpDiscoveryAcknowledgeLayer

Done, it's GvcpDiscoveryAcknowledgeLayer::GvcpVersion now.

seladb · 2026-09-30T06:32:33Z

+
+		/// GVCP response status can be returned in an acknowledge message or a GVSP header.
+		/// See more in the spec "Table 19-1: List of Standard Status Codes"
+		enum class GvcpResponseStatus : uint16_t


This enum can move to GvcpAcknowledgeLayer

Done, it's GvcpAcknowledgeLayer::GvcpResponseStatus now.

Move GvcpResponseStatus into GvcpAcknowledgeLayer and GvcpVersion into GvcpDiscoveryAcknowledgeLayer, the layers that own these fields.

seladb · 2026-10-01T09:51:37Z

Before you merge, can you also implement serializeLayer()?
You can see several examples of how to implement it in this PR: #2277

If you don't have time feel free to merge this PR as is and I'll add it later.

Done, I added serializeLayer() to all the GVCP layers.

Implement serializeLayer() for the GVCP layers. Each layer writes its fields after the fields of its parent class: the command and data size, then the request or acknowledge header fields, then the Discovery or Force IP fields. Command and status values are written as a number and as a name.

tigercosmos requested a review from seladb as a code owner April 12, 2024 09:31

tigercosmos marked this pull request as draft April 12, 2024 09:31

egecetin added this to the Augest 2024 Release milestone Jun 5, 2024

tigercosmos marked this pull request as ready for review August 9, 2024 02:25

Dimi1010 reviewed Aug 9, 2024

View reviewed changes

Dimi1010 linked an issue Aug 9, 2024 that may be closed by this pull request

Introduce GigE Vision Control Protocol (GVCP) protocol #1321

Open

Dimi1010 mentioned this pull request Aug 12, 2024

Add an overload to Layer::Layer(...) that accepts a protocol parameter. #1530

Closed

seladb reviewed Aug 14, 2024

View reviewed changes

Dimi1010 mentioned this pull request Aug 14, 2024

Added a RAII PcapHandle wrapper class that manages a Pcap handle. #1532

Merged

seladb reviewed Aug 18, 2024

View reviewed changes

tigercosmos marked this pull request as draft August 23, 2024 09:08

tigercosmos removed this from the September 2024 Release milestone Sep 25, 2024

tigercosmos requested review from Dimi1010 and seladb September 26, 2024 01:52

tigercosmos marked this pull request as ready for review September 26, 2024 01:52

seladb reviewed Oct 2, 2024

View reviewed changes

tigercosmos commented Oct 2, 2024

View reviewed changes

Comment thread Packet++/header/GvcpLayer.h Outdated

seladb reviewed Oct 9, 2024

View reviewed changes

Dimi1010 reviewed Dec 6, 2024

View reviewed changes

tigercosmos closed this Mar 20, 2025

tigercosmos reopened this Sep 17, 2026

tigercosmos force-pushed the gvcp_0410 branch from 97a74a1 to e52bfdc Compare September 17, 2026 08:39

tigercosmos force-pushed the gvcp_0410 branch from e52bfdc to 0ee3f5d Compare September 17, 2026 08:42

tigercosmos self-assigned this Sep 17, 2026

tigercosmos added the enhancement label Sep 17, 2026

tigercosmos marked this pull request as ready for review September 17, 2026 10:24

tigercosmos commented Sep 17, 2026

View reviewed changes

tigercosmos requested review from Dimi1010 and seladb September 17, 2026 10:25

Dimi1010 approved these changes Sep 21, 2026

View reviewed changes

seladb reviewed Sep 25, 2026

View reviewed changes

tigercosmos force-pushed the gvcp_0410 branch 2 times, most recently from 3c87bd8 to 5027426 Compare September 27, 2026 04:11

seladb reviewed Sep 27, 2026

View reviewed changes

Comment thread Packet++/src/GvcpLayer.cpp

Comment thread Packet++/header/GvcpLayer.h Outdated

Comment thread Packet++/header/GvcpLayer.h Outdated

Comment thread Packet++/header/GvcpLayer.h Outdated

Comment thread Packet++/header/GvcpLayer.h

tigercosmos force-pushed the gvcp_0410 branch from 5027426 to 010ba10 Compare September 29, 2026 06:16

Merge branch 'dev' into gvcp_0410

7b9b2a9

tigercosmos requested a review from seladb September 30, 2026 04:34

seladb approved these changes Sep 30, 2026

View reviewed changes

Move GVCP response status and version into their layers

9cdc927

Move GvcpResponseStatus into GvcpAcknowledgeLayer and GvcpVersion into GvcpDiscoveryAcknowledgeLayer, the layers that own these fields.

seladb reviewed Oct 1, 2026

View reviewed changes

Conversation

tigercosmos commented Apr 12, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Design

Tests

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

tigercosmos commented Aug 12, 2024

Uh oh!

codecov Bot commented Aug 12, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

tigercosmos commented Aug 13, 2024

Uh oh!

seladb left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

seladb left a comment

Choose a reason for hiding this comment

Uh oh!

tigercosmos commented Aug 18, 2024

Uh oh!

seladb commented Aug 18, 2024

Uh oh!

tigercosmos commented Sep 26, 2024

Uh oh!

tigercosmos commented Oct 2, 2024

Uh oh!

seladb commented Oct 2, 2024

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

seladb commented Oct 2, 2024

Uh oh!

tigercosmos commented Oct 2, 2024

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

tigercosmos commented Apr 12, 2024 •

edited

Loading

codecov Bot commented Aug 12, 2024 •

edited

Loading