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

Address family is a u8, not u16 #198

Merged
merged 1 commit into from
Nov 24, 2023

Conversation

dswarbrick
Copy link
Contributor

As per definition of ndmsg in kernel source:

struct ndmsg {
	__u8		ndm_family;
	__u8		ndm_pad1;
	__u16		ndm_pad2;
	__s32		ndm_ifindex;
	__u16		ndm_state;
	__u8		ndm_flags;
	__u8		ndm_type;
};

As per definition of ndmsg in kernel source:

struct ndmsg {
	__u8		ndm_family;
	__u8		ndm_pad1;
	__u16		ndm_pad2;
	__s32		ndm_ifindex;
	__u16		ndm_state;
	__u8		ndm_flags;
	__u8		ndm_type;
};

Signed-off-by: Daniel Swarbrick <daniel.swarbrick@gmail.com>
@dswarbrick
Copy link
Contributor Author

dswarbrick commented Nov 23, 2023

This probably did not have any ill-effects in the past, but if there was junk in ndm_pad1, it would have erroneously been unmarshalled into the NeighMessage.Family uint16.

Incidentally, why not use the predefined NdMsg struct (and others) in the unix package?

@jsimonetti
Copy link
Owner

Thank you! Nice catch.

With regard to not using NdMsg, I believe at the time of writing the initial package (some 7 years ago), these structs where not yet extracted from the kernel and we had to do this in the package.
Though I am not certain.. it was a while ago :)

@jsimonetti jsimonetti merged commit eac8438 into jsimonetti:master Nov 24, 2023
5 checks passed
@dswarbrick dswarbrick deleted the ndmsg-fixups branch November 24, 2023 13:06
@dswarbrick
Copy link
Contributor Author

With regard to not using NdMsg, I believe at the time of writing the initial package (some 7 years ago), these structs where not yet extracted from the kernel and we had to do this in the package. Though I am not certain.. it was a while ago :)

If you don't mind, I would gladly submit a PR to refactor the code to use such structs from the unix package - in the interest of code cleanliness and DRY.

@jsimonetti
Copy link
Owner

If you don't mind, I would gladly submit a PR to refactor the code to use such structs from the unix package - in the interest of code cleanliness and DRY.

If this can be done in a backwards compatible fashion I have no problem with that.

the current design (using an internal unix package) has been a thorn in my eyes ever since it was introduced to accommodate developers using apple. I am not sure if this is still relevant since the introduction of go modules.

@dswarbrick
Copy link
Contributor Author

dswarbrick commented Nov 25, 2023

the current design (using an internal unix package) has been a thorn in my eyes ever since it was introduced to accommodate developers using apple. I am not sure if this is still relevant since the introduction of go modules.

Aha, now I see what you mean about your internal/unix package, providing the Linux constants on non-Linux GOOS. Correct me if I'm wrong, but doesn't this rtnetlink package specifically only support Linux? I'm not sure I understand what Darwin users could actually achieve by using this code, other than running the unit tests. It's not like the Darwin kernel is going to speak the same rtnetlink protocol as Linux does.

@jsimonetti
Copy link
Owner

The support is for sure linux only. The reasoning behind those internal constant for non-Linux GOOS was to allow macos developers to work with this package proper.
This might be an old pre-gomod thing. I am fairly certain current IDE's can work fine if you set GOOS=linux in your project.

i will have a look if this internal package is still required.

@florianl could you elaborate a bit if the internal package created in pr #63 is still required these days?

@florianl
Copy link
Contributor

Looking at packages that import github.com/jsimonetti/rtnetlink, like cilium/pwru or tailscale/tailscale, none of them sets GOOS=linux. As far as I know, it is not possible to set GOOS on a package/project level, other than doing so every time on the shell. So if internal/unix is dropped, then developers on OS other than linux, will face IDE issues - unless the IDE supports to set GOOS on a project/package level.

In addition, GOOS=darwin golang.org/x/sys/unix does not provide NdMsg.

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