drgn.helpers.linux.net: add some room helpers #353
Conversation
Useful in debugging net issues that are often about skbs. Signed-off-by: Mina Almasry <[email protected]>
Signed-off-by: Mina Almasry <[email protected]>
There was a problem hiding this comment.
Thanks for these! A few comments here, and a couple of more generic requests in line with CONTRIBUTING.rst:
- Could you please add docstrings to these helpers?
- Could you please add test cases? The test I added for
skb_shinfo()in 470b2e0 might be good inspiration. At the very least we should call them from a test case without looking at the result to make sure they don't blow up, but bonus points if you can craft SKBs to meaningfully test these.
| def skb_is_nonlinear(skb: Object) -> Object: | ||
| return skb.data_len |
There was a problem hiding this comment.
I think it would make more sense for this one to return bool:
| def skb_is_nonlinear(skb: Object) -> Object: | |
| return skb.data_len | |
| def skb_is_nonlinear(skb: Object) -> bool: | |
| return bool(skb.data_len) |
|
|
||
|
|
||
| def skb_headroom(skb: Object) -> Object: | ||
| return skb.data - skb.head |
There was a problem hiding this comment.
In the kernel, skb_headroom() returns unsigned int, but this returns ptrdiff_t. If we want to be extra accurate, we should cast this here:
| return skb.data - skb.head | |
| return cast("unsigned int", skb.data - skb.head) |
| def skb_tailroom(skb: Object) -> IntegerLike: | ||
| if skb_is_nonlinear(skb): | ||
| return 0 | ||
| return skb.end - skb.tail |
There was a problem hiding this comment.
Similarly, this one is int in the kernel, so it could be:
| def skb_tailroom(skb: Object) -> IntegerLike: | |
| if skb_is_nonlinear(skb): | |
| return 0 | |
| return skb.end - skb.tail | |
| def skb_tailroom(skb: Object) -> Object: | |
| if skb_is_nonlinear(skb): | |
| return Object(prog, "int", 0) | |
| return cast("int", skb.end - skb.tail) |
There doesn't seem to be much logic in how the return types were picked in the kernel, but we might as well copy those choices for the rest, too.
| "shinfo(txflags=%u nr_frags=%u gso(size=%hu type=%u segs=%hu))\n" | ||
| "csum(0x%x ip_summed=%u complete_sw=%u valid=%u level=%u)\n" | ||
| "hash(0x%x sw=%u l4=%u) proto=0x%04x pkttype=%u iif=%d\n" | ||
| % ( |
There was a problem hiding this comment.
I'd prefer an f-string or at least str.format() over % formatting.
They were useful in my debugging of an issue. I thought I'd contribute them.