Seagate / opensea-transport

Cross platform library containing common set of functions to issue standard commands to storage devices.
Other
21 stars 22 forks source link

quick: ata_cmds.c: remove redundant judgement on variable "feature" #13

Closed ThunderEX closed 2 years ago

ThunderEX commented 2 years ago

When I read these lines, I found the judgement in these if lines are already implemented in the bigger switch clause in line 546. So I think they are just redundent judgements, are they?

Signed-off-by: ThunderEX thunderex@gmail.com

vonericsen commented 2 years ago

Hi @ThunderEX, These are redundant, but necessary due to using the M_FALLTHROUGH at the bottom of the cases. This macro inserts either a comment /* Fallthrough */ or it will insert an attribute for GCC/Clang that specifies falling through the cases is intentional. This is due to using the -Wimplicit-fallthrough which is part of -Wextra. (I know this warning is in the makefile, but not sure if it is enabled in meson). It is checking for the value of feature to only print verbose output for the appropriate feature before setting up the data transfer flags. The first few are data-in, then write log is data-out, and the last few fall into the default case for non-data. The fallthroughs can be removed along with this additional check before the print, but the data transfer flags will need to be set in each case or those subcommands will fail. These are the flags that would need to be set in each case:

ataCommandOptions.commandDirection = /*data direction*/; ataCommandOptions.ataCommandLengthLocation = /* transfer length location (sector count or nondata) */; ataCommandOptions.ataTransferBlocks = /* transfer blocks (512B or no data)*/; ataCommandOptions.commadProtocol = /* protocol (PIO or non data)*/;

For example if this additional check of feature is removed as in your commit, in verbose mode for feature == ATA_SMART_READ_LOG, this would print out "Read Log - XXh" then it would also print "Read Thresholds" and then "Read Data". This would be confusing rather than only seeing "Read Log - XXh".

ThunderEX commented 2 years ago

Hi @vonericsen Thanks for explanation. I just didn't realize that fall through. I made a change to fix a small mis-alignment I could see. Could you please take a look?

vonericsen commented 2 years ago

This looks good. Thank you!