-
Notifications
You must be signed in to change notification settings - Fork 13.3k
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
Allow users to inherit and override CertStore #7827
Conversation
I've been thinking if it's not better to make a CertStoreBase with a virtual What are the thoughts of the community? I can modify my PR accordingly. |
That seems like a reasonable thing to do and would avoid dragging unneeded class variables into subclasses. But, I'm sure you are trying to get some other way to hold the certstore (i.e. a PROGMEM array), so if you can get that in it would really be nice. |
Perfect, I've implemented it. I've added a virtual destructor to CertStoreBase even tough we only borrow it, never own as CertStoreBase. And that CertStore should be long-lived and probably won't ever be destroyed. But Idk about c++ best practices surrounding virtual destructors. It seemed like the best choice. But I can remove it. |
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.
Looks good. Did you want to add in your other CertStore object?
I don't think it's the best move stabilizing another CertStore alternative, with its own python script. I can provide the alternative if it's deemed important since so many people depend on the hacks and save certs to PROGMEM. But I'm not really confortable doing it right now, but I can indeed provide it in a future PR. |
@paulocsanz: Thanks for adding this! It seems that this change was at least partly inspired by my hack, which indeed came about due to the lack of virtual methods. I have updated my code with the new inheritance mechanism in this commit: maakbaas/esp8266-iot-framework@8a752f8 |
Fixes: #7826
This seems the minimal change that is enough to avoid guard headers hacks and allow users to have a more customized CertStore.