Skip to content

Conversation

@axelkar
Copy link
Contributor

@axelkar axelkar commented Nov 21, 2025

This makes it possible for library users to match on errors.

Errors are also more descriptive now:

Error: I/O error while reading

Caused by:
   0: libusb error
   1: Overflow

Compared to what it was before:

Error: other error

For some reasoning, see https://kazlauskas.me/entries/errors

This makes it possible for library users to match on errors.

Errors are also more descriptive now:

    Error: I/O error while reading
    
    Caused by:
       0: libusb error
       1: Overflow

Compared to what it was before:

    Error: other error

For some reasoning, see https://kazlauskas.me/entries/errors
@quic-kdybcio
Copy link
Contributor

JFYI I noticed this PR, I'll try to take a look later this week

@quic-kdybcio
Copy link
Contributor

I like the general idea of this.

What I'm still deliberating is the level of granularity which the library should expect the consumer to care about, i.e. should firehose_read() really be exported and/or encouraged, or should the library provide a set of complete "issue command, process i/o, verify result" functions. anyhow is very convenient whenever it does the job, but understandably it's not always enough as you demonstrated.

What's your usecase? Are you building something OSS that we could take a look at in parallel?

I utilized the library in a(n unfortnuately internal) project and my usecase was rather simple and not very error prone (the device's state was more or less predictable) so this didn't hit me before. I know @quic-bjorande is cooking something too - maybe he can comment on any roadblocks he hit current approach too

@axelkar
Copy link
Contributor Author

axelkar commented Dec 12, 2025

I think the granularity is required for possible users to check for the level of the failure in their specific use case. They could retry depending on the error (i.e. buggy firehose loader vs USB disconnected) and give even more descriptive errors to end users. As noted in the linked blog article, thiserror also forces the context to be added, so error messages are more descriptive.

i.e. should firehose_read() really be exported and/or encouraged

The program can't know which state a device is in, any may even be restarted/run while a Firehose loader is running, so I'm all for having Sahara and Firehose functionality separate. About that specific function, I'm not sure.

What's your usecase? Are you building something OSS that we could take a look at in parallel?

Just testing currently. The ABL of a device seems to brick the boot partition with some fastboot commands, and I've just built an error-safe way to reflash the boot partition.

@axelkar
Copy link
Contributor Author

axelkar commented Dec 12, 2025

The blog post does also mention this, which may or may not be of concern to you:

When ap­plied in a lib­rary, con­sider mark­ing the er­ror types as #[non_exhaustive] or keep er­ror data mostly private. Hav­ing a unique er­ror for each fail­ure mode ne­ces­sar­ily ex­poses (through the pub­lic API) the im­ple­ment­a­tion de­tails of the lib­rary which in turn may make evol­u­tion of the lib­rary dif­fi­cult.

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