Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

support nsh encap and decap #25

Closed
wants to merge 0 commits into from
Closed

Conversation

zhenglinan
Copy link

support nsh encap and decap
@tnqn

Signed-off-by: zhenglinan [email protected]

@tnqn tnqn requested a review from wenyingd August 15, 2022 14:50
@tnqn
Copy link
Member

tnqn commented Aug 15, 2022

@zhenglinan thanks for the PR.
@wenyingd could you take a look?

Copy link

@wenyingd wenyingd left a comment

Choose a reason for hiding this comment

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

Two general comments except for the code change:

  1. Please update file VERSION whenever you propose a new change, change the min part (the version string uses this format: max.min.patch) if it is a new feature, e.g. this change is a new feature support. And change the patch part if it is bug fix.
  2. Please format your code before updating your patch on github.

var b []byte
n := 0

b, err = a.NXActionHeader.MarshalBinary()

Choose a reason for hiding this comment

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

Add error check here, becore consuming b?

Copy link
Author

Choose a reason for hiding this comment

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

I have added error check here in new commit

}

func (a *NXActionDecapsulate) Len() (n uint16) {
return a.Length

Choose a reason for hiding this comment

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

It is better to return a constant or calcuate the length according to the properties instead of consuming some property in function Len(), to avoid unnecessary issue if the property is not set in advance.

Copy link
Author

Choose a reason for hiding this comment

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

done , I have replaced it to constant 16

binary.BigEndian.PutUint32(data[n:], a.HeaderType)
n += 4
return
}

Choose a reason for hiding this comment

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

Function UnmarshalBinary in type NXActionDecapsulate is missing?

Copy link
Author

@zhenglinan zhenglinan Aug 21, 2022

Choose a reason for hiding this comment

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

you are right ! Thanks

type NXActionEncapsulate struct {
*NXActionHeader
HeaderSize uint16
HeaderType uint32

Choose a reason for hiding this comment

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

Would you use an enum to list the valid values of HeaderType in NXActionEncapsulate?

Copy link
Author

Choose a reason for hiding this comment

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

done

n := 0

b, err = a.NXActionHeader.MarshalBinary()
copy(data[n:], b)

Choose a reason for hiding this comment

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

Check err before consuming b?

Copy link
Author

Choose a reason for hiding this comment

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

done

a.HeaderType = binary.BigEndian.Uint32(data[n:])
n += 4
var prop PropTLV
for n < int(a.Length) {

Choose a reason for hiding this comment

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

Use a.Len() to be consistent with line 2012?

Copy link
Author

Choose a reason for hiding this comment

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

done

@wenyingd
Copy link

I have another question: are the new actions only for nsh packets, or also for ethernet? If they are only suitable for nsh packets, it is better to change names more concrete, otherwise, it is confusing when calling "encap(ethernet)"

a.NXActionHeader = new(NXActionHeader)
err := a.NXActionHeader.UnmarshalBinary(data[n:])
if err != nil {
return errors.New("failed to unmarshal header")

Choose a reason for hiding this comment

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

Suggested change
return errors.New("failed to unmarshal header")
return fmt.Errorf("failed to unmarshal NXAction header: %v", err)

*NXActionHeader
HeaderSize uint16
HeaderType EncapType
Property []PropTLV

Choose a reason for hiding this comment

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

Suggested change
Property []PropTLV
Property []PropTLV
Suggested change
Property []PropTLV
Properties []PropTLV

func NewNXActionEncapsulate(headerSize uint16, headerType EncapType, property []PropTLV) *NXActionEncapsulate {
a := new(NXActionEncapsulate)
a.NXActionHeader = NewNxActionHeader(NXAST_RAW_ENCAP)
a.Length = 16

Choose a reason for hiding this comment

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

If property is not empty, a.Length should be a value larger than 16?

a := new(NXActionDecapsulate)
a.NXActionHeader = NewNxActionHeader(NXAST_RAW_DECAP)
a.Length = 16
a.pad = make([]byte, 2)

Choose a reason for hiding this comment

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

No need to explicitly make a byte slice for pad, since its value is never accessible?

Copy link
Author

Choose a reason for hiding this comment

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

refer to NewNXActionCTNAT

n += int(a.NXActionHeader.Len())

if len(data) < int(a.Len()) {
return errors.New("the []byte is too short to unmarshal a full NXActionConjunction message")

Choose a reason for hiding this comment

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

Suggested change
return errors.New("the []byte is too short to unmarshal a full NXActionConjunction message")
return errors.New("the []byte is too short to unmarshal a full NXActionDecapsulate message")

}
n += int(a.NXActionHeader.Len())
if len(data) < int(a.Len()) {
return errors.New("the []byte is too short to unmarshal a full NXActionConjunction message")

Choose a reason for hiding this comment

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

Suggested change
return errors.New("the []byte is too short to unmarshal a full NXActionConjunction message")
return errors.New("the []byte is too short to unmarshal a full NXActionEncapsulate message")

@wenyingd
Copy link

wenyingd commented Aug 22, 2022

Please pay attention on the DCO and test/golint errors.

Update: Please add unit test on your new code, e.g., you can test the message Marshal and Unmarshal in the case, so that we could avoid runtime issues.

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