SCSI Dump Usability Enhancements & Cleanup by akuker · Pull Request #1026 · PiSCSI/piscsi

akuker · 2022-12-08T16:50:19Z

Fix for weird corner case - when executing the arm binary on an x86_64 host, PiSCSI will throw an exception while trying to initialize the GPIO bus. This isn't a valid use case, so just handling the exception and aborting execution. (TIL - Ubuntu 22 on x86_64 will still execute an ARM binary. It seems to spin up Qemu in the background to try to run the executable)
Rename RasDump to ScsiDump
Add total time and transfer rate to the output of scsidump
Create a .properties file when backing up a drive. This can be used to more closely emulate the original drive.
Added examples to man page + some cleanup/clarification

…/RASCSI into feature_scsidump_usability

uweseimet · 2022-12-08T19:09:20Z

cpp/scsidump/scsidump_core.h

+
+    int run(const vector<char *> &);
+
+    struct inquiry_info_struct {


Does this struct have to be public instead of being private?

Its used in the test procedure. Suggestions for a better way to make it accessible to the test procedure?

In this case (tests also using it) there might be no alternative. In general an implementation should not have weak points just because of unit tests. If datat structures are/should be private from the implementation perspective (like in this case) tests should be coded in a way that they do not need to access these private structures. This is best practices as far as I know, and it makes sense. Otherwise you end up with a lot of data being public just because of the tests, which is defeating the idea of encapsulation.

Just one anecdotal note on this, because I just remembered: In a job interview I had a long time ago, how to deal with a similar issue was a topic. It was essentially a private constant string that was part of the implementation. The string content was also required in a unit test. The preferred solution was to have a copy of the string (duplication) in the test, so that the original string could stay private.
In this particular case using a copy has also another benefit: If you use the same public string for your test, if somebody changes the string declaration your code may be wrong, but the test still passes. You will miss the code modification. With a copy for the test it would have been revealed that something in the code had been changed,.

In this case its a struct, not data. Another solution would be to move it out of the class definition completely.

I don't object keeping the code as it is if there is no convenient alternative.

uweseimet · 2022-12-08T19:13:37Z

cpp/test/test_shared.cpp


 // TODO Replace old-fashinoned C I/O by C++ streams I/O.
 // This also avoids potential issues with data type sizes and there is no need for c_str().
-void CreateTempFileWithData(const string& filename, vector<uint8_t> &data)


I would appreciate if we were not reverting C++-style conventions for pointers and references back to C-style. We have C++ style almost everywhere now, and it's not just Bjarne Stroustrup who says:

A typical C programmer writes int *p; and explains it *p is what is the int emphasizing syntax, and may point to the C (and C++) declaration grammar to argue for the correctness of the style. Indeed, the * binds to the name p in the grammar. A typical C++ programmer writes int* p; and explains it p is a pointer to an int emphasizing type. Indeed the type of p is int*. I clearly prefer that emphasis and see it as important for using the more advanced parts of C++ well.

but also IBM, cpppreference.com, ...

Are you saying that you prefer this:

void CreateTempFileWithData(const string& filename, vector<uint8_t>& data)

over this?

void CreateTempFileWithData(const string &filename, vector<uint8_t> &data)

Currently, its the clang formatting configuration that is doing this, so I'll need to investigate. But, I wanted to double check I'm understanding the comment first.

Yes, that's what I (and others like Stroustrup) are saying for pointers (*) and references (&) in C++. (If you google for it you will find several hits.) When using "&" as address operator it's different, but for "&" denoting a reference it should directly follow the type.

Sounds good. I'll work on updating that configuration.

Should now be updated. See changes in .clang-format. This change will be applicable to pointers and references

uweseimet · 2022-12-08T19:14:33Z

cpp/test/test_shared.cpp

+string ReadTempFileToString(const string &filename)
+{
+    path temp_file = test_data_temp_path / path(filename);
+    std::ifstream in_fs(temp_file);


Since we usually declare namespace std to be used we should omit std:: for simplicity and readability sake.

uweseimet · 2022-12-08T19:16:10Z

cpp/scsidump/scsidump_core.h

+
+    bool restore = false;
+
+    const string divider_str = "----------------------------------------";


Can this string be static?

uweseimet · 2022-12-08T19:18:50Z

doc/scsidump.1

 .TP
 .BR \-r\fI
-Run in restore mode.
+Run in restore mode. Defaults to download mode if not specified.


I don't think "download" is a good term. The common terms are dump/restore, just as the names of the respective UNIX commands. Downloading is quite specific for the file downloads in the internet.

Changed to dump

uweseimet · 2022-12-08T19:20:30Z

doc/scsidump_man_page.txt

+       scsidump has two modes of operation: download and restore. These can be used with physical storage media, including hard drives and magneto optical drives. Download mode can be used with read-only media such as CD/DVD drives.

-       Set its own ID with the BID option. Defaults to 7 if ommitted.
+       When operating in download mode, scsidump will copy all data from a remote SCSI drive to an image on the local filesystem. Additionally, it will generate a .properties file that can be used to more closely emulate the source drive.


Is the creation of the .properties file mandatory? It should not be because not everybody (not just not me) would want unexpected and potentially invisible (dot) files to be created.
Making the file creation optional and the filename configurable would IMO be best. One should also consider not to overwrite existing files.

Now requires '-p' argument in order to enable this feature.

uweseimet

Please see my comments.

uweseimet · 2022-12-08T20:12:12Z

A general remark, not specific to this PR, but also concerning this PR: For functional extensions like in this case (property file) it might be good to have a ticket first because:

Others (not only somebody involved in the review or not even a developer) can provide useful comments
Changes to already implemented new code can be avoided, because the implementation goal can be adjusted before starting the implemention. This saves time because you do not implement something (at least not that often) that you later may have to change.
Comments in tickets IMHO are more informal than in reviews, which makes it easier to talk about changes. Once you have a PR, as a reviewer I usually do not want to block something, and requesting changes is more unpleasant than in a ticket. (At least for me.)
Tickets are potentially read by more developers/users than a PR, which improves the overall knowledge about a project.
Personally, if I were to search for something that has already been discussed in a project I would search the tickets, not the PRs.

Just my two cents.

uweseimet · 2022-12-09T09:30:54Z

@akuker Thank you four addressing my comments.

akuker · 2022-12-09T15:50:24Z

@uweseimet - Thank you for reviewing!! I'll try to do a better job of writing up issues before implementing things

Tony Kuker and others added 13 commits December 7, 2022 21:49

Usability + documentation improvements for scsidump

e5e3934

Updated notes

270d3f2

Added tests for generating properties file

66ab3d7

Fix sonar smell

a75d223

Update gpiobus_factory.cpp

48381d1

Fix sonar smells

e64dde8

Use tmp directory for test procedures (switch to use fopen)

2ab0851

Merge branch 'feature_scsidump_usability' of http://github.com/akuker…

f9f8520

…/RASCSI into feature_scsidump_usability

remove extraneous semicolon

6f922ae

prevent closing invalid filehandle

76649bf

Rename class

dc6a1da

sonar updates

447c291

Rename RasDump to Scsidump

6306f2b

akuker requested a review from uweseimet December 8, 2022 19:05

akuker marked this pull request as ready for review December 8, 2022 19:05

akuker requested review from erichelgeson and rdmark as code owners December 8, 2022 19:05

uweseimet reviewed Dec 8, 2022

View reviewed changes

uweseimet suggested changes Dec 8, 2022

View reviewed changes

Tony Kuker added 3 commits December 8, 2022 17:07

Left-align pointers and reference. Change divider string to static

c3157bb

Make properties file creation optional with -p arg. PR comments.

67c7311

Fix typo

abacd99

uweseimet approved these changes Dec 9, 2022

View reviewed changes

akuker merged commit 5b3626d into develop Dec 9, 2022

akuker deleted the feature_scsidump_usability branch December 9, 2022 15:50


		int run(const vector<char *> &);

		struct inquiry_info_struct {


		bool restore = false;

		const string divider_str = "----------------------------------------";

Uh oh!

Conversation

akuker commented Dec 8, 2022

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

uweseimet Dec 8, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

akuker Dec 8, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

uweseimet Dec 8, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

uweseimet left a comment

Choose a reason for hiding this comment

Uh oh!

uweseimet commented Dec 8, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

uweseimet commented Dec 9, 2022

Uh oh!

akuker commented Dec 9, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

uweseimet Dec 8, 2022 •

edited

Loading

akuker Dec 8, 2022 •

edited

Loading

uweseimet Dec 8, 2022 •

edited

Loading

uweseimet commented Dec 8, 2022 •

edited

Loading