Skip to content

Add CTLabel support in the GetValue method for MatchField#88

Merged
antoninbas merged 1 commit into
antrea-io:mainfrom
antoninbas:add-support-for-ctlabel-when-getting-match-field-value
Aug 19, 2025
Merged

Add CTLabel support in the GetValue method for MatchField#88
antoninbas merged 1 commit into
antrea-io:mainfrom
antoninbas:add-support-for-ctlabel-when-getting-match-field-value

Conversation

@antoninbas

Copy link
Copy Markdown

No description provided.

@hongliangl hongliangl left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

Comment thread ofctrl/ofMatchFields.go Outdated
if !m.HasMask {
return value
}
// TODO: we match the logic for the ByteArrayField case, but it's not clear why we

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is because the type of v.Data is byte[], while m.Mask type is util.Message, and we need to convert to byte[] to construct type DataWithMask.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

m.Value and m.Mask are the same. The type is util.Message but that's an interface. The concrete type is *CTLabel for both, so we could just use m.Mask.(*CTLabel).Data[:] for the mask (which would not make a copy). If you think no copy is needed, I can replace MarshalBinary with that.

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sounds good to me without additional copy.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I removed the unnecessary copies, please take another look

wenyingd
wenyingd previously approved these changes Aug 15, 2025

@wenyingd wenyingd left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

Also remove unnecessary copies when processing masks for other match
field types.

Signed-off-by: Antonin Bas <antonin.bas@broadcom.com>
@antoninbas antoninbas force-pushed the add-support-for-ctlabel-when-getting-match-field-value branch from 68b9f64 to 8cd8643 Compare August 18, 2025 17:21
@antoninbas antoninbas requested a review from wenyingd August 18, 2025 17:27

@wenyingd wenyingd left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@antoninbas antoninbas merged commit b63f481 into antrea-io:main Aug 19, 2025
4 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants