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

bridge: add riscv64 build tags #2389

Merged
merged 1 commit into from
Jun 26, 2019
Merged

bridge: add riscv64 build tags #2389

merged 1 commit into from
Jun 26, 2019

Conversation

tonistiigi
Copy link
Member

Add riscv64 build tags to unbreak the build. If needed I can rename the files cause they are not arm specific (even before this pr).

Signed-off-by: Tonis Tiigi tonistiigi@gmail.com

Copy link
Collaborator

@selansen selansen left a comment

Choose a reason for hiding this comment

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

LGTM

tiborvass
tiborvass previously approved these changes Jun 5, 2019
@@ -1,4 +1,4 @@
// +build arm ppc64 ppc64le
// +build arm ppc64 ppc64le riscv64
Copy link
Contributor

Choose a reason for hiding this comment

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

filename is weird actually. Not sure what it should be.

Copy link
Contributor

Choose a reason for hiding this comment

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

@selansen @arkodg the only difference between these two files is ifrDataByte returning uint8 vs int8. I'm not familiar what ifrData means, but if you could help coming up with a better filename like netlink_deprecated_linux_???_uint8.go and netlink_deprecated_linux_???_int8.go it would be appreciated!

Copy link
Collaborator

Choose a reason for hiding this comment

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

It is getting invoked in only one place . Looking at the caller, we should use only int8 ( checked few different type variant )
IftData . - given interface's hardware address ( MAC ) value

you can use intf_addr or hwintf_addr

Copy link
Contributor

Choose a reason for hiding this comment

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

@selansen In https://github.com/docker/libnetwork/blob/20e2079/drivers/bridge/netlink_deprecated_linux.go#L100 IfruHwaddr is of type syscall.RawSockaddr whose Data field's type varies across architectures, so we do need a different ifrDataByte function.

I suggest we go with something like netlink_deprecated_linux_rawsockaddr_data_uint8.go and netlink_deprecated_linux_rawsockaddr_data_int8.go

Copy link
Collaborator

Choose a reason for hiding this comment

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

I did check only few architectures but the variable where we store is int8. I do understand there may be other arch which might use int8.
WRT name , rawsockeraddr ( the actual structure name) data is very generic one. This particular scenario, we use it only for hw_Interface_addr(MAC)
I am fine with generic name too.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes it may only be used for mac address, but the reason the IfruHwaddr function needs different implementations on different arches is because of syscall.RawSockaddr.Data type hence the suggested name.

@tonistiigi
Copy link
Member Author

@tiborvass @selansen renamed

@selansen
Copy link
Collaborator

selansen commented Jun 6, 2019

CI failure is known flaky test ( I have seen it few times in the past) . so LGTM

	networkdb_test.go:57: Number of nodes for node2 into the cluster does not match 5 != 4```

@carlosedp
Copy link

Any news on this guys?

@arkodg
Copy link
Contributor

arkodg commented Jun 21, 2019

@tonistiigi the flaky TestValidRemoteDriver is fixed, can you please rebase and retest

Signed-off-by: Tonis Tiigi <tonistiigi@gmail.com>
@tonistiigi
Copy link
Member Author

@arkodg green now

@arkodg
Copy link
Contributor

arkodg commented Jun 26, 2019

PTAL @selansen

@euanh euanh merged commit 424b422 into moby:master Jun 26, 2019
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.

6 participants