Skip to content

Conversation

RealFatCat
Copy link
Contributor

… via sock_diag.

Probably, needs some restructurisation.

@randomizedcoder
Copy link

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

@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.

@RealFatCat RealFatCat force-pushed the feat/sock-diag-tcp-info branch from e2c06bf to faaf133 Compare June 3, 2020 19:28
@aboch
Copy link
Collaborator

aboch commented Jun 3, 2020

Is it feasible to add a UT for this ?

@RealFatCat RealFatCat force-pushed the feat/sock-diag-tcp-info branch from faaf133 to 5b160c9 Compare June 3, 2020 19:55
@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
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