Skip to content
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

Store plugin's by name rather than path #23386

Draft
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

MewtR
Copy link
Contributor

@MewtR MewtR commented Sep 25, 2024

  • Mark this if you consider it ready to merge
  • I've added tests (optional)
  • I wrote some lines in the book (optional)

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.

@trufae
Copy link
Collaborator

trufae commented Sep 25, 2024

This change breaks the abi. Can u use the R2_USE_NEW_ABI ?

@MewtR
Copy link
Contributor Author

MewtR commented Sep 25, 2024

Alright, wasn't aware of that define, I'll look into it in the coming days.

@trufae
Copy link
Collaborator

trufae commented Sep 26, 2024

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.

- Make parse/bp plugins use RPluginMeta
- Print plugin path with Lcj
- Use plugin name as opposed to the filename to detect if a plugin has
  already been loaded.
@MewtR
Copy link
Contributor Author

MewtR commented Sep 27, 2024

You're right about the getname callback, I removed it.

I'll try to write some tests for this at some point.
Also, yeah we can wait until abi-breaking season I don't mind.

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?

@trufae
Copy link
Collaborator

trufae commented Sep 27, 2024

Theres inly afen. An external parse plugin. Dont know any about bp

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.

2 participants