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

Add fill command to overwrite SD card #125

Closed
wants to merge 8 commits into from

Conversation

robinkrahl
Copy link
Collaborator

No description provided.

This patch adds the fill command that overwrites the SD card with random
data.  Similar to the reset command, we always require the user to enter
the admin PIN even if is cached.
This patch adds the is_tty field to the Context struct that indicates
whether stdout is a TTY.  This allows us to use TTY features like moving
the cursor in our output.
This patch uses the progressing crate to display a progress bar for the
fill command if the output is printed to a TTY.
@d-e-s-o
Copy link
Owner

d-e-s-o commented Sep 11, 2020

I'll have a look at that tomorrow.

@d-e-s-o d-e-s-o self-assigned this Sep 12, 2020
Copy link
Owner

@d-e-s-o d-e-s-o left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for working on this functionality. I have a few comments.

doc/nitrocli.1 Show resolved Hide resolved
src/commands.rs Show resolved Hide resolved
Cargo.toml Outdated Show resolved Hide resolved
src/main.rs Outdated Show resolved Hide resolved
src/commands.rs Outdated Show resolved Hide resolved
src/output.rs Outdated Show resolved Hide resolved
src/output.rs Outdated
}

pub fn draw(&self, ctx: &mut Context<'_>) -> anyhow::Result<()> {
use crossterm::{cursor, terminal};
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh...guess we need it for other stuff too :-|

Have you considered using termion? Would it fulfill our requirements?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes. termion works for ANSI terminals only. crossterm is cross-platform and also supports e. g. Windows terminals.

@robinkrahl
Copy link
Collaborator Author

I’ve pushed fixes for the issues you mentioned. Let me know if you want me to replace crossterm with termion or if you want to increase the delay.

@d-e-s-o
Copy link
Owner

d-e-s-o commented Sep 12, 2020

I’ve pushed fixes for the issues you mentioned. Let me know if you want me to replace crossterm with termion or if you want to increase the delay.

Yeah, I'd prefer we keep an eye on dependencies and add them responsibly and only as necessary. I am sure crossterm has good reasons for pulling in parking_lot, mio etc. but I don't think we actively need them in our small app. Not conceptually, anyway. Hence, I'd prefer we use some other, lower profile crate. termion is what I know a little and it fits the bill better. If that means that some rarely used feature will have a bit worse UX on a platform we don't actively target or test on, I think that's a reasonable compromise.

replace crossterm with termion
@robinkrahl
Copy link
Collaborator Author

Unfortunately, termion’s DetectCursorPos does not work for me:

$  ./target/debug/nitrocli fill
^[[63;1RFailed to query cursor position

Caused by:
    Cursor position detection timed out.

Could you please check if it is working for you? Here is the code with termion instead of crossterm.

@d-e-s-o
Copy link
Owner

d-e-s-o commented Sep 12, 2020

Hm. Seeing the same issue.

@d-e-s-o
Copy link
Owner

d-e-s-o commented Sep 12, 2020

Ugh, here is a good one too. This is the error that I get when running fill while another fill is still in progress.

Failed to retrieve serial number

Caused by:
    Library error: The supplied string is not in hexadecimal format

which makes absolutely no sense to the average user.

That's just utterly brain dead.

Here is the verbose log:

[Sat Sep 12 13:38:38 2020][DEBUG_L1]    Connection success: 1 (0001:000a:02)
[Sat Sep 12 13:38:38 2020][DEBUG_L1]    [0001:000a:02]Device successfully changed
[Sat Sep 12 13:38:38 2020][DEBUG_L1]    [0001:000a:02]=> GET_DEVICE_STATUS
[Sat Sep 12 13:38:38 2020][WARNING]     [0001:000a:02]Accepting response with CRC other than expected Command ID: 39 FILL_SD_CARD_WITH_RANDOM_CHARS  Reported by response and expected: 854714777!=2224801007
[Sat Sep 12 13:38:38 2020][DEBUG_L1]    [0001:000a:02]<= FILL_SD_CARD_WITH_RANDOM_CHARS 1 4
[Sat Sep 12 13:38:38 2020][DEBUG_L1]    [0001:000a:02]Throw: Long operation in progress exception
[Sat Sep 12 13:38:38 2020][DEBUG_L1]    [0001:000a:02]Disconnection: handle already freed: 1 (0001:000a:02)

If only we were capable of handing errors through properly...

@d-e-s-o
Copy link
Owner

d-e-s-o commented Sep 12, 2020

Hm. Seeing the same issue.

It works with:

--- src/output.rs
+++ src/output.rs
@@ -67,6 +67,7 @@ impl ProgressBar {
   pub fn draw(&self, ctx: &mut Context<'_>) -> anyhow::Result<()> {
     use anyhow::Context as _;
     use termion::cursor::DetectCursorPos as _;
+    use termion::raw::IntoRawMode;

     if !ctx.is_tty {
       return Ok(());
@@ -74,6 +75,8 @@ impl ProgressBar {

     let pos = ctx
       .stdout
+      .into_raw_mode()
+      .unwrap()
       .cursor_pos()
       .context("Failed to query cursor position")?;
     let progress_char = if self.toggle && !self.finished {

@robinkrahl
Copy link
Collaborator Author

Hmm, it seems to work if I activate raw mode, at least in a minimal example. (Unfortunately, termion’s API reference is rather poor.) But should we always activate raw mode in main.rs?

@d-e-s-o
Copy link
Owner

d-e-s-o commented Sep 12, 2020

Hmm, it seems to work if I activate raw mode, at least in a minimal example. (Unfortunately, termion’s API reference is rather poor.) But should we always activate raw mode in main.rs?

I think we can just do that only in draw(). I'd hope everything just works as expected when leaving the scope.

@robinkrahl
Copy link
Collaborator Author

I think we can just do that only in draw(). I'd hope everything just works as expected when leaving the scope.

I don’t think so – IntoRawMode takes ownership of the io::Write object.

@d-e-s-o
Copy link
Owner

d-e-s-o commented Sep 12, 2020

Yes, but we have a reference to a Writer and Write is directly implemented for that if I recall correctly. In any case, that's what I did in the patch and it is working (to the degree I can tell). I see the progress bar progressing and I also have a fully functioning terminal after the command finishes.

@robinkrahl
Copy link
Collaborator Author

Ugh, here is a good one too. This is the error that I get when running fill while another fill is still in progress.

This issue is caused by overlapping error codes in libnitrokey, see Nitrokey/libnitrokey#170. My PR with a fix is waiting to be merged: Nitrokey/libnitrokey#177

@robinkrahl
Copy link
Collaborator Author

Sorry, I missed your comment with the patch. I’ll add it to my implementation and open an issue with termion regarding the documentation.

replace crossterm with termion
@robinkrahl
Copy link
Collaborator Author

robinkrahl commented Sep 12, 2020

Updated.

Regarding killing nitrocli while the operation is still running (I wanted to discuss this separately, but as you’ve brought up the issue, we can discuss this here): I think we should provide some means to re-attach to the progress bar if we killed nitrocli.

There are four ways to deal with this:

  1. Once we have a proper error code for the LongOperationInProgressException, we could just show the progress bar every time we encounter this error. But this might be confusing if e. g. the nitrocli status command shows the progress bar for the fill background process.
  2. We could always check if an operation is running for the fill command. If yes, we just show the progress bar. Otherwise we execute the fill command. But with bad timing, this could trigger a new fill command if the user only wants to attach to a running operation.
  3. We could a new --attach option to the fill command that checks whether an operation is running. If yes, the progress bar is shown. Otherwise we show an error message.
  4. We could introduce a new subcommand that just shows the progress of the currently running background operation (or an error if there is no background operation).

The OperationStatus does not tell us what kind of background operation is running. But currently, the fill command is the only supported background operation AFAIK.

I like options 3 and 4 best. 4 would be future proof, but the problem is that I can’t think of a good name for this subcommand. attach? progress?

@robinkrahl
Copy link
Collaborator Author

By the way, this is the reason why IntoRawMode works with a reference:

impl<'_, W: Write + ?Sized> Write for &'_ mut W

@d-e-s-o
Copy link
Owner

d-e-s-o commented Sep 12, 2020

Updated.

Thank you! Merged it now.

I like options 3 and 4 the best. 4 would be future proof, but the problem is that I can’t think of a good name for this subcommand. attach? progress?

Intuitively I'd prefer keeping it in fill itself, at least for the time being. It doesn't appeal to me too much to have a high level command dedicated to it. So 3 would be my preference. However, I can see that in terms of future proofness a high level command would be better. I suppose either way is fine with me.

@d-e-s-o d-e-s-o closed this Sep 12, 2020
@robinkrahl robinkrahl deleted the fill-sd-card branch September 20, 2020 21:25
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