Store plugin's by name rather than path #23386
Conversation
|
This change breaks the abi. Can u use the R2_USE_NEW_ABI ? |
|
Alright, wasn't aware of that define, I'll look into it in the coming days. |
|
As long as this is a rather large PR changing the abi of rparse i dont think it makes sense to use the new-abi define. so i think it's better to hold this PR until 5.9.9 is in dev. (we need to release 5.9.6 and 5.9.8 before reaching the abi breaking seasson). Apart from that i think the change makes sense and seems reasonable. I want to perform more changes in the rparse api, so i wanted to have some time before the 5.9.9 time arrives to do that. what do you think? as long as we are not gonna break abi, this pr shouldnt be affected by any change until the time arrives. also, do u think the getname callback is necessary? because if all the plugins use the rpluginmeta now, we can just assume this struct on all of them and not depend on helper functions to do that. |
MewtR
force-pushed
the
plugin_meta
branch
2 times, most recently
from
4eb2d14
to
283d4ab
Compare
|
You're right about the getname callback, I removed it. I'll try to write some tests for this at some point. I'm afraid this change might break some plugins that are outside the r2 repo though. Are you aware of any bp/parse plugins "external" to r2 or how I can find them? |
|
Theres inly afen. An external parse plugin. Dont know any about bp |
MewtR
force-pushed
the
plugin_meta
branch
3 times, most recently
from
9556cb3
to
e988777
Compare
libr/arch/p/avr/pseudo.c
Outdated
| .name = "avr.pseudo", | ||
| .desc = "AVR pseudo syntax", | ||
| .author = "pancake", | ||
| .license = "LGPL3", |
There was a problem hiding this comment.
Please use SPDX namings. See the related commits in git log | grep SPDX for more info
MewtR
force-pushed
the
plugin_meta
branch
2 times, most recently
from
01acca1
to
cd02b7f
Compare
|
Now it’s the abi breaking moment! Feel free to help improve the state or other apis too! Glad to see this pr updated now btw |
MewtR
force-pushed
the
plugin_meta
branch
6 times, most recently
from
0ba07a2
to
080c3a7
Compare
|
I think the PR is ready. I wanted to write some tests but I need a "dummy" plugin and I'm not sure where to put them. I was initially thinking of putting it in radare2-testbins but it might get complicated to support all platforms. Do you have any suggestions? |
|
Looks good to merge for me too. No need for tests imho. We have many other important refactorings to do rn and we probably want to mess a little more with the plugins architecture before we release. Would u like to split this up into three commits for bp/parse meta and another for the speicific changes to make this logic work? Otherwise in fine to merge it as 1 commit too |
|
Alright, I split it into three commits. That's what you meant right? 3 commits and not 3 PRs? I can also have 3 PRs if you like |
|
Maybe worth using "Fix #22445 - Store plugin's by name rather than path ##core" this way the commit will be listed in the release notes, and the ticket will be closed when merged |
|
one 1 with 3 commits is fine, what i care mainly is about using the ## for the relevant commits to appear in the release notes :) so just update the last commit and ill merge |
- Print plugin path with Lcj - Use plugin name as opposed to the filename to detect if a plugin has already been loaded - Refactor r_lib_close and allow L- (remove plugin) to work with the plugin's name in addition to file names
|
Ah yes ok, it's done! |
Description
So I was looking at #22445 and I figured it would be interesting to store plugins in the hash table by their names instead of their paths.
It makes it easier to detect duplicates and easier to get the path from which the plugin was loaded, but this is more of a proposition.
Code is a little awkward in some places, looking for feedback.