Add deprecation decorator and split Linux modules utilities

[Abyss-W4tcher]

Hi,

This PR adds a dedicated deprecation decorator to centralize the process. Sample:

WARNING  volatility3.framework.configuration.requirements: Method "volatility3.framework.symbols.linux.lookup_module_address" is deprecated, use "volatility3.framework.symbols.linux.utilities.modules.Modules.lookup_module_address" instead.

Happy to discuss on the verbosity too (WARNING, INFO, DEBUG ?).

A couple of modules related LinuxUtilities were deprecated and moved to a dedicated module. The "kernel" related utilities (even if it is a module) weren't moved out. The framework was updated to use the new APIs paths, so the deprecation messages will be output only for external plugins.

Note: Some plugins weren't bumped due to the lack of versioning, but there is already on-going work on this subject in #1547.

Versioning

A minor bump should be needed for the new decorator, but I'll wait for a review on the general idea first.

[ikelos]

Looks good, but a few comments. Where we put the generic deprecation header is still an open question. I don't think configuration.requirements is the right place for it. It's more of a framework-wide helper...

[ikelos]

Thanks, I prefer the improvements, but still some tweaks I think we could do. Lemme know what you think?

[ikelos]

Looking better, and definitely very thorough, but probably overkill. It doesn't need any of the requirements machinery, it just needs to semver check (which I think is a static/classmethod). You've done the hard part of finding the class from the method, so then just grab the version number and test it and throw an exception if they're wrong. We can put a standard exception handler to explain what they should do... 5:)

[Abyss-W4tcher]

I made the requested changes, hopefully it matches more closely what you had in mind ? I think the result is more explicit now:

A version mismatch was detected between two components:
volatility3.framework.symbols.linux.LinuxUtilities.lookup_module_address: Version (1, 2, 0) dependency on volatility3.framework.symbols.linux.utilities.modules.Modules (1, 0, 0) unmet.

        * A deprecated method was unable to proxy the call to its replacement
        * Please re-run with -vvv and file a bug with the output at https://github.com/volatilityfoundation/volatility3/issues

No further results will be produced

I'll count the required framework bumps at the end. :)

[ikelos]

Keeps getting better, just a couple more points and then I think we can get it across the line! 5:)

[ikelos]

qualname doesn't quite do what we want I think? We want the class that was called, not the class where the method was defined...

For example:

>>> class A:
...   version = (1,0,0)
...   def thing(self):
...     print(self.version)
... 
>>> class B(A):
...   version = (2,0,0)
... 
>>> ver2 = B()
>>> ver2.thing()
(2, 0, 0)
>>> ver2.thing.__qualname__
'A.thing'

I think we want to be using __self__:

>>> ver2.thing.__self__
<__main__.B object at 0x7f3049a33ec0>

That also saves us needing up lookup the class using __globals__.

[Abyss-W4tcher]

__self__ works on an stateful instance, however we are referencing linux_utilities_modules.Modules.lookup_module_address not linux_utilities_modules.Modules().lookup_module_address. I am not sure that instantiating the class is exactly what we want, as it might imply non-optional arguments sometimes.

That's a good point however, thanks for catching it ! Python internals are making it quite difficult to get B on this specific scenario.

I think using an extra class argument in deprecated_method will save us a lot of headaches, or sketchy/magical processing ?

[ikelos]

It still seems to work for class methods?

>>> class A:
...   version = (1,0,0)
...   def thing(self):
...     print(self.version)
...   @classmethod
...   def classthing(cls):
...     print(cls.version)
... 
>>> class B(A):
...   version = (2,0,0)
... 
>>> B.classthing
<bound method A.classthing of <class '__main__.B'>>
>>> B.classthing.__self__
<class '__main__.B'>

[Abyss-W4tcher]

Yes, nice catch as well. However it doesn't work with staticmethod (thanks Python ?), which we use a lot.

[ikelos]

I think that's a better reason to go back through versioned components and change them from staticmethod to classmethod? Also, this only affects new things so that we can deprecate old ones. We can just require new ones to be classmethods (will need documenting somewhere, and we should probably go back through versioned things and make the staticmethods into classmethods (in a different PR!)...

[Abyss-W4tcher]

Honestly, I am not sure that it is worth the hassle, and staticmethod has valid use cases. Sometimes, a class only purpose is basically to group functions under it without necessarily making them communicate or access its properties ?

We could simply do the following to resolve the issue explicitely:

    @Deprecation.deprecated_method(
        replacement=linux_utilities_modules.Modules.lookup_module_address,
        replacement_base_class=linux_utilities_modules.Modules,  # <-- added
        replacement_version=(1, 0, 0),
    )

This also prevents edge cases that we haven't anticipated when accessing objects internals ?

[ikelos]

The valid use case is a slight boost in efficiency because it doesn't need to lug its class around with it, but now we need the class to get to the version property and otherwise the two are essentially indistinguishable? I'm ok with staticmethods being used in certain places, but generally not in versioned classes (which essentially means all plugins). I remember being a little against going to staticmethod when someone first tried to put one in a year or two ago.

I think people will find it odd that they need to define a second variable that's already contained in the first in order to get deprecation warnings to work, but also, I'd like to be able to get to the version of any method defined on a versionable component...

[ikelos]

One of the first times I mentioned it... 5:S #115 (comment)

[Abyss-W4tcher]

Ok if this is a requirement for versioned methods it makes sense to apply this restriction. I will check this PR (and my other ones) for any staticmethod.

Spider-Sense kicked in 5 years ago for an issue we are encountering today 🕸️

[ikelos]

Ok, I think we're there. Thanks for your patience!

[Abyss-W4tcher]

@ikelos I was planning on bumping the framework version first ?

Thanks for your patience as well :)

[Abyss-W4tcher]

In the end, one minor bump would be needed I think.

[ikelos]

Happy for you to throw it in a separate PR if you want. I guess it is an additive change...