Refactor connection handling for multiple devices #119
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This patch introduces two new functions, find_device and connect, to connect to a Nitrokey device. find_device queries the attached Nitrokey devices, applies the filters (currently only the --model option) and returns the first match. connect calls find_device and connects to the returned device. This refactoring allows us to add more device filters, for example a --serial-number option, without code duplication.
Previously, we just applied our filter (if any) to all attached Nitrokey devices and selected the first match when connection to a Nitrokey device. This may lead to unexpected behaviour if multiple devices are attached. This patch changes the find_device function to return an error if multiple matching devices are found.
This patch adds the --serial-number option that allows the user to filter the attached Nitrokey devices by serial number. As the Nitrokey Storage does not include its serial number in the USB device descriptor and as we don’t want to connect to it just to query the serial number, this option only works for Nitrokey Storage devices.
Use old error message
Fix status::not_found_raw test case
|
Thanks for the pull request! Looks good! I've made a few cosmetic changes and merged it. But can we come up with a way to test this functionality better? E.g., one way could be to have a test function that accepts any model (runs on pro & storage), lists the available devices to get their serial number, and then invokes |
|
Thanks! I’ll check whether I can implement the test cases you
described.
However, your changes introduced a problem with the with_storage_device
function. In my implementation, I always used Model::Storage when
calling connect. Your implementation uses the model from the context
configuration, which may be empty if the user did not set the --model
option. I’ll open a new PR with a fix.
|
|
Oh, sorry about that :-| Didn't want too many review round trips. I'll merge your suggestions later today, but probably just as |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This patch series refactors the connection handling into two new functions,
find_deviceandconnect, and improves the handling of multiple attached devices as discussed in issue #99: If mulitple attached devices match the filter set by the user, nitrocli now fails instead of just picking the first match. And the--serial-numberoption now allows the user to filter the devices by serial number. (We already have the --model option to filter by model.)This means that we always list all attached Nitrokey devices before connecting. Still the overhead is very small as the list can be read from the sys file system without any communication with the Nitrokey device itself.
Unfortunately, the Nitrokey Storage sends a wrong serial number in the USB device descriptor that is used for listing the devices, see Nitrokey/nitrokey-storage-firmware#88. Therefore the new
--serial-numberoption only works for Nitrokey Pro devices. Originally, I wanted to wait with this feature until this is fixed in the firmware, but I don’t think this will happen any time soon. (And we would still have to support devices with older firmware versions.)As a next step, we could add support for multiple devices to the
statuscommand.