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

Introduce portable, endian-clean structures using shifts and masks instead of bitfields #66

Merged
merged 5 commits into from
May 13, 2022

Conversation

jmwilson
Copy link
Contributor

Bitfields are nonportable and often have buggy implementations. Shifts and masks, while more verbose, have better defined semantics. This is a preview of using them for the SCSI communication structures. The end of this change will be to get rid of the need for the #if STENC_BIG_ENDIAN checks and the run-time endian check in main (yuck) and ensure that the code is endian-clean at compile time.

I went back to the references of SCP-5 and SSP-5 to get the layout of the SCSI command structures and reimplemented them side-by-side with the old structures for comparison. Following NL.8 and NL.9, I went with a snake case naming convention for these structures. I also followed a style based on ARM's register naming convention:

struct foo {
  uint8_t flags1;
static constexpr auto flags1_field1_pos {0u};
static constexpr uint8_t flags1_field1_mask {1u << flags1_field1_pos};
static constexpr auto flags1_field2_pos {1u};
static constexpr uint8_t flags1_field1_mask {3u << flags1_field2_pos};
  ...
};

vs

struct foo {
  uint8_t field1 : 1;
  uint8_t field2 : 2;
  ...
};

Endian conversion is handled by using htons/ntohs functions - these are the most portable option since /usr/include/arpa/inet.h should be on every UNIX system. There are some better options in endian.h but this file is not fully standard across all UNIX variants.

src/scsiencrypt.h Outdated Show resolved Hide resolved
src/scsiencrypt.h Outdated Show resolved Hide resolved
src/scsiencrypt.h Outdated Show resolved Hide resolved
on = 2u,
};

enum class decrypt_mode: std::uint8_t {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Would Scott M. suggest auto instead of uint8_t here? Just curious...
The enum is so much cleaner already. Nice.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

By default enums are sized as int but the underlying field in the SCSI page is a single byte, so it is necessary to require a smaller size.

@jonasstein
Copy link
Collaborator

Is this well tested already? What is left to do before merging?

@jmwilson
Copy link
Contributor Author

Will need to rewrite much of scsiencrypt.cpp to use these structures before it makes sense to merge. The new tests should ensure strong coverage.

@lgtm-com
Copy link

lgtm-com bot commented May 13, 2022

This pull request introduces 2 alerts when merging 40e97bb into 1508f43 - view on LGTM.com

new alerts:

  • 2 for Catching by value

@jmwilson jmwilson marked this pull request as ready for review May 13, 2022 20:19
@jmwilson jmwilson changed the title Preview: endian clean structures using shifts and masks instead of bitfields Introduce portable, endian-clean structures using shifts and masks instead of bitfields May 13, 2022
@jmwilson
Copy link
Contributor Author

Since this is a large PR, I have broken it into 5 commits:

  1. introduces the new structures in the scsi namespace and adds some unit tests
  2. introduces a modernized version of SCSIExecute using RAII and exceptions that will replace the old versions. Since the device code can't be unit tested, this was tested on both Linux and FreeBSD
  3. Switches the SP-IN code (everything that is used when running stenc --detail) over to use the new structures.
  4. Switches the SP-OUT (changes to the encryption setting) over to use the new structures
  5. Deletes the old code

At each commit, the tests pass showing the SCSI data written and the interpretation of data received remains stable regardless of the representation used. This was accompanied by live testing on Linux and FreeBSD to verify normal and error behavior.

@jmwilson
Copy link
Contributor Author

This will also close #63 since the new scsi::get_nbes will not move the tape.

@jonasstein jonasstein merged commit eeb7d72 into scsitape:master May 13, 2022
@jmwilson jmwilson deleted the endian branch June 6, 2022 05:33
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.

3 participants