pkcs11 icon indicating copy to clipboard operation
pkcs11 copied to clipboard

Replace p11.Slot struct with interface

Open JeremyRand opened this issue 4 years ago • 4 comments

p11.Session is an interface, which is helpful for use cases where a downstream user needs to re-implement the API. Alas, p11.Slot is not an interface, which makes such use cases rather unpleasant. Would a PR be accepted that makes p11.Slot an unexported struct and adds a public p11.Slot interface to replace it?

JeremyRand avatar Nov 19 '21 17:11 JeremyRand

I'm not against that. It's p11 code so not the main repo, however we've never had a backwards incompatible change here... So other opinions might differ

On Fri, 19 Nov 2021, 18:22 JeremyRand, @.***> wrote:

p11.Session is an interface, which is helpful for use cases where a downstream user needs to re-implement the API. Alas, p11.Slot is not an interface, which makes such use cases rather unpleasant. Would a PR be accepted that makes p11.Slot an unexported struct and adds a public p11.Slot interface to replace it?

— You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub https://github.com/miekg/pkcs11/issues/144, or unsubscribe https://github.com/notifications/unsubscribe-auth/AACWIWZT32YGTLMBXTSLF53UM2BUZANCNFSM5IMUGGVA .

miekg avatar Nov 19 '21 17:11 miekg

What's your preferred way of gauging opinions? Just wait some time to see if anyone comments in this issue? If, hypothetically, it were desired to make some of the other structs interfaces as well (the others aren't as high priority for me, but might be a bit helpful in the future), would it be preferable to roll them all into one PR to minimize the number of breaking change events?

JeremyRand avatar Nov 19 '21 17:11 JeremyRand

Yeah. Just have to wait.

I'm in favor of doing all the changes at once, forcing folks to deal with compilation errors. We could do the go V2 stuff as well, but don't want to, because it's more involved

On Fri, 19 Nov 2021, 18:46 JeremyRand, @.***> wrote:

What's your preferred way of gauging opinions? Just wait some time to see if anyone comments in this issue? If, hypothetically, it were desired to make some of the other structs interfaces as well (the others aren't as high priority for me, but might be a bit helpful in the future), would it be preferable to roll them all into one PR to minimize the number of breaking change events?

— You are receiving this because you commented. Reply to this email directly, view it on GitHub https://github.com/miekg/pkcs11/issues/144#issuecomment-974278472, or unsubscribe https://github.com/notifications/unsubscribe-auth/AACWIW5BFC2TYYLV7XMOCOLUM2EORANCNFSM5IMUGGVA .

miekg avatar Nov 19 '21 19:11 miekg

Okay, so if we're going to roll all the breaking changes into one release, here are my suggestions for additional API changes:

  • Convert pkcs11.Ctx and p11.Object to interfaces as well; same motivation as p11.Slot.
  • Split out convenience functions such as p11.Object.Value and p11.Session.Login into separate interfaces, so that types that implement the underlying functions (p11.Object.Attribute and p11.Session.LoginAs) don't need to re-implement those convenience functions. So e.g. there could be a p11.Object interface which has both Attribute and Value functions, and a p11.ObjectCore interface that has Attribute but not Value, with a p11.ObjectFromCore function that returns an Object that uses the given ObjectCore.
  • I guess https://github.com/miekg/pkcs11/issues/26 can be rolled in too?
  • Once a PR is ready, ideally I'd like to run that PR in production for a few months before it's merged, so I can identify whether there are any other API changes that my use cases would benefit from; this reduces the risk that I'll ask for another breaking change later.

JeremyRand avatar Jan 04 '22 11:01 JeremyRand