-
Notifications
You must be signed in to change notification settings - Fork 481
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
Linux ip.Addr and ip.Link plugins #1079
base: develop
Are you sure you want to change the base?
Linux ip.Addr and ip.Link plugins #1079
Conversation
…xplicitily use the exception instance
Hey @ikelos I'm aware of this #1029 excellent contribution from @eve. We've already discussed this, and we agreed to collaborate, combining our efforts into one. |
@gcmoreira - looks really cool, I need to look over it properly. I certainly like pulling out the prefix into it's own column compared with #1029 - that makes it easier to work with programmatically later. +1 vote to renaming the plugins to linux.ip.addr etc to match the new linux commands. There are already a bunch of commands that were effectively renamed between vol2 and vol3 already. |
volatilityfoundation#1029 * IP address conversion via renderers.coversion.* * Use MAC address internal size instead of hardcoded. * Read NET_DEVICE_FLAGS from enumeration
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Generally looks good, just a couple of minor points to check please. 5:)
|
||
def _get_net_device_flag_value(self, name): | ||
"""Return the net_deivce flag value based on the flag name""" | ||
return self._get_flag_choices()[name] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd be happier if this were a get with a default value? Having said that, it's such a short function, perhaps it's not worth having at all (given it's private so can't be used by anyone else?)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Default value and error managed.
I prefer to keep it as a function, even though it's short and private. This way, it serves the purpose of documenting what it does and increases the code's readability. I would let the language to implement the bytecode inline if needed.
Anyway, if you still think it should be inline, let me know, and I will change it.
} | ||
|
||
# RFC 2863 operational status. Kernels >= 2.6.17. See IF_OPER_* in include/uapi/linux/if.h | ||
IF_OPER_STATES = ( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should these be Enum
s rather than just a list of strings? If may potentially lead to typos?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done. This is used by value, so I don't think at the moment being an Enum offers any usability advantage. Although I reckon it may look better to Pythonic eyes ;)
IFF_PROMISC = 0x100 | ||
# Only for kernels < 3.15 when the net_device_flags enum didn't exist | ||
# ref include/uapi/linux/if.h | ||
NET_DEVICE_FLAGS = { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This also looks like it could be a python Enum?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
hm here changing this to an Enum makes the code a bit complicated... have a look at how it's used in _get_flag_choices()
and _get_net_device_flag_value()
to support both kernels <3.15 and >=3.15.
Thanks @ikelos. I will have a look at this soon. I also plan to include the ip.link plugin from @eve-mem in this PR, that's why I marked it as a draft. In the meantime, could you have a look at the black formatter checks? I'm confused, as far as I understand it's failing on files which are not from this PR |
@gcmoreira - it looks nice to me. I'll run it against my collection of samples when I can. Can I help adding in |
It is, or was, github updated to black 24.1.0, so I went over the while
codebase, but it doesn't seem to get that the merge *target* updated, so
returning the task still results in the same output. 5:S. Basically if you
add a new commit, it should do a proper refresh and either pass or show you
how the new black failed it. Hard to tell if there's still something that
needs doing, but just a trivial commit will help and I figured since it was
in draft it would get added to... 5:)
…On Mon, 29 Jan 2024, 00:55 gcmoreira, ***@***.***> wrote:
Thanks @ikelos <https://github.com/ikelos>. I will have a look at this
soon. I also plan to include the ip.link plugin from @eve-mem
<https://github.com/eve-mem> in this PR, that's why I marked it as a
draft.
In the meantime, could you have a look at the black formatter checks? I'm
confused, as far as I understand it's failing on files which are not from
this PR
—
Reply to this email directly, view it on GitHub
<#1079 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AALIZVIJNUJJARGODCOUILTYQ3XOXAVCNFSM6AAAAABBTT6DKKVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMYTSMJTG44TEMRTG4>
.
You are receiving this because you were mentioned.Message ID:
***@***.***>
|
Yep, that did the trick, thanks |
@eve-mem sure, go ahead with that ;) |
…tform and, based on it, utilizes either the posixpath or ntpath modules
- On top of the @eve-mem, I've added the queue length field to mimic the ip link command. - Furthermore, I've included some functions to export the network device flags exactly as they are presented to userland
@ikelos This is now ready for review. Added testcases for both plugins |
Looks like there are some merge conflicts with this now. |
This PR adds the
linux.ip.Addr
andlinux.ip.Link
plugins.linux.ip.Addr:
Example output:
linux.ip.Link (by @eve-mem )
Both plugins were tested with the following linux kernel versions:
See the full test case suite output:
vol3_linux_ip_addr_output.txt
vol3_linux_ip_link_output.txt