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

Add SocketDiagTCPInfo with some constants and structs to get tcp_info… #534

Merged
merged 1 commit into from
Sep 25, 2020

Conversation

RealFatCat
Copy link
Contributor

… via sock_diag.

Probably, needs some restructurisation.

@randomizedcoder
Copy link

G'day @RealFatCat , This is a really great PR.

tcp_linux.go Outdated Show resolved Hide resolved
tcp.go Outdated Show resolved Hide resolved
@RealFatCat
Copy link
Contributor Author

RealFatCat commented Apr 29, 2020

Yeah, found the problem that in different kernels we have different size of tcp_info struct.
Moreover, some fields can be added in the middle of structure.
For example, in 4.19 there is tcpi_delivery_rate_app_limited right after scales https://elixir.bootlin.com/linux/v4.19/source/include/uapi/linux/tcp.h#L184
But in v4.4 we don't have it: https://elixir.bootlin.com/linux/v4.4/source/include/uapi/linux/tcp.h#L159
I didn't count on such "inserts" in the kernel.

Any advises how to handle this?

UPD: Probably, need to determine version of current kernel and read that byte or not.

@khasanovbi
Copy link

Yeap, understand problem now.
I'm think, we could determine version or use size of b.
Maybe we could write some private structs for different versions (looks like blame should help https://github.com/torvalds/linux/blame/29d9f30d4ce6c7a38745a54a8cddface10013490/include/uapi/linux/tcp.h) and converters to our unified struct.
We can use struct embedding to don't duplicate same fields between different linux versions.

@RealFatCat
Copy link
Contributor Author

Probably, there is no issue with tcpi_delivery_rate_app_limited.
I'll update this PR after some tests.

@RealFatCat
Copy link
Contributor Author

This is probably not the most beautiful solution, but should work on every kernel.

socket_linux.go Show resolved Hide resolved
@aboch
Copy link
Collaborator

aboch commented Jun 3, 2020

Is it feasible to add a UT for this ?

tcp_linux.go Outdated Show resolved Hide resolved
@RealFatCat
Copy link
Contributor Author

Is it feasible to add a UT for this ?

I really don't know how to do it properly. It seems like we need to mock kernel answers somehow, to compare them with expected stats data. But how.

@vishvananda
Copy link
Owner

This looks good? How about a unit test to make sure that the request to the kernel for stats returns data without actually checking the values to see if they are valid? That would at least validate that the call works.

@RealFatCat
Copy link
Contributor Author

Will do.

… via sock_diag

Add simple test for SocketDiagTCPInfo
@vishvananda vishvananda merged commit e499154 into vishvananda:master Sep 25, 2020
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.

5 participants