Improve how commands are internally executed #1247
Conversation
uweseimet
commented
Oct 15, 2023
•
uweseimet
commented
- Use const CommandContext for execution
- Update error handling
- Fix SonarQube issues
- Remove duplicate code
- Use mutex instead of atomic_bool
* Use const CommandContext for execution * Update error handling * Fix SonarQube issues * Remove duplicate code * Use mutex instead of atomic_bool * Add hasher * Add param_map
uweseimet
requested review from
akuker,
erichelgeson and
rdmark
as code owners
| if (const string error = service.Init([this] (CommandContext& context) { | ||
| context.SetDefaultFolder(piscsi_image.GetDefaultFolder()); | ||
| return ExecuteCommand(context); | ||
| }, port); !error.empty()) { | ||
| cerr << "Error: " << error << endl; |
There was a problem hiding this comment.
Elsewhere I see reference to a logging function (spdlog::info, etc). Is this write to cerr leftover from debugging and should it be converted to a proper log message?
There was a problem hiding this comment.
In general, everything that is logged to a logfile (usually by the PiSCSI core) is logged with the spdlog library. Messages the result from the user launching command line tools (e.g. scsictl, piscsi) are usually written to cout or cerr, in order to provide direct feedback to the user (e.g. because a command line parameter was wrong). There is no need for those to be logged with timestamps etc.
In the old C++ code you may still find macros use for logging, but they should gradually be replaced by direct usage of spdlog. The macros have issues, e.g. there might be a buffer overflow. Current C++ tries to reduce the use of macros anyway because of several reasons.
Which IDE or editor are you using, by the way? And which kind of Pi? My main development environment is Eclipse on Linux, a Pi 4, and mainly Atari computers for testing that requires SCSI functionality.
There was a problem hiding this comment.
Thanks for the explanation. Yes, down with macros!
Typically, I use vim as an editor, but have also been using CLion lately. I note that you have a clang-format configuration file defining the formatting style, and CLion plays nicely with that, because it can enforce formatting rules. Thus, I am likely to use CLion more for this project.
In "production", my PiSCSI is connected to a Raspberry Pi 3 Model A+, but I also have a 4 lying around. I am using this as a drive emulator for my Akai S3000XL and Akai S6000 samplers, and will also be able to test it on a Kurzweil K2000R sampler at some point.
While the Tindie store is currently out of stock, I am just going to invest in another PiSCSI when they are available so that I can maintain a development system separate from the one that I use for music stuff.
I've forked the repo and have been poking around. Are there build instructions anywhere for Debian amd64? Or is the procedure to run that script?
There was a problem hiding this comment.
I think the formatting style file was added by @akuker. I have never used it, and I do not know how closely it reflects the current style.
If you want to compile anywhere, e.g. on a regular PC, there are notes how to do this at the bottom of https://github.com/PiSCSI/piscsi/wiki/Setup-Instructions.
I guess running easysetup might work (@rdmark probably knows more), but when I set up a new compile environment I usually just check out the sources, install the libraries/headers that may be required for compiling (there is a list of them in the easysetup script), and then just run make.
There was a problem hiding this comment.
I think the formatting style file was added by @akuker. I have never used it, and I do not know how closely it reflects the current style.
@uweseimet How about we review the coding guidelines and agree upon a shared set of stylistic rules? With a new developer onboarding, I think it would be productive to have this to avoid f.e. unexpected line changes when someone's IDE automatically changes indentation or line breaks, etc.
I think this is the file in question: https://github.com/PiSCSI/piscsi/blob/main/cpp/.clang-format
Let me create a new ticket for this.
There was a problem hiding this comment.
@rdmark I agree. But I would like to merge all pending PRs and the tickets that are already ready for PRs but already may need merging (there are several) first. Otherwise formatting changes will make the existing changed code unusable. Does this make sense?
There was a problem hiding this comment.
@uweseimet Yes I agree with that priority. I will create the ticket so that we can take action on this a little bit later.
| // TODO Check why there are rare cases where bus is NULL on a remote interface shutdown | ||
| // even though it is never set to NULL anywhere |
There was a problem hiding this comment.
No, but I don't think it is needed. The null check is a working solution, but not one I am satisfied with. All in all this is just something that should be improved, but nothing that affects the user. This is why I think a TODO is sufficient.
There was a problem hiding this comment.
I will merge after resolving the merge conflicts.
|
SonarCloud Quality Gate failed.
|
