RfidResearchGroup / proxmark3

Iceman Fork - Proxmark3
http://www.icedev.se
GNU General Public License v3.0
3.63k stars 981 forks source link

Properly handle "hf mf dump" errors #2291

Closed gsingh93 closed 4 months ago

gsingh93 commented 4 months ago

I was confused why hf mf dump wasn't showing any errors but the dump file was all zeros:

[usb|script] pm3 --> hf mf dump
[+] time: 0 seconds

[+] saved 1024 bytes to binary file `/Users/gsgx/hf-mf-D6E6ACDF-dump-001.bin`
[+] saved to json file `/Users/gsgx/hf-mf-D6E6ACDF-dump-001.json`

For some reason PM3_SUCCESS was being returned from this error path, and the log message was using the DEBUG log level. After this PR, it's more clear an error occured:

[usb|script] pm3 --> hf mf dump
[-]  iso14443a card select failed
github-actions[bot] commented 4 months ago

You are welcome to add an entry to the CHANGELOG.md as well

iceman1001 commented 4 months ago

Its more of a client design choice. Not just one command change like you suggested.

A) Should all commands give a textual feed back if it fails ? or B) Should it not give out any output and only give output when successful.

I tend today to prefer B, so I am making the client act unified towards that way.

gsingh93 commented 4 months ago

@iceman1001 in that case, I've updated the PR to only change the return value, which will make the command produce no output.

I do prefer option A though. If you prefer option B, is there ever any reason to use "FAILED" or "ERR" log messages? I assume you have some criteria for determining when it's appropriate to use them?

iceman1001 commented 4 months ago

I am not saying that the pm3 client is a marvelous wonder of unified thought through design concepts.

gsingh93 commented 4 months ago

@iceman1001 no worries 😄 let me know if the current version of the PR looks good.