[qemu] Make logging code terser #3653
Conversation
Condense code to log mismatches between suspension snapshot and VM state.
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## force_stop_on_no_suspend_tag #3653 +/- ##
================================================================
- Coverage 88.89% 88.88% -0.01%
================================================================
Files 254 254
Lines 14258 14230 -28
================================================================
- Hits 12675 12649 -26
+ Misses 1583 1581 -2 ☔ View full report in Codecov by Sentry. |
There was a problem hiding this comment.
@ricab , it looks good, only comments on the code, no requested change.
| fmt::format("{} has the image suspension snapshot, but it is not in the suspended state", vm_name)); | ||
| } | ||
| const auto has_suspend_snapshot = mp::backend::instance_image_has_snapshot(desc.image.image_path, suspend_tag); | ||
| if (has_suspend_snapshot != (state == State::suspended)) // clang-format off |
There was a problem hiding this comment.
Nice trick to utilize the symmetry.
| if (has_suspend_snapshot != (state == State::suspended)) // clang-format off | ||
| mpl::log(mpl::Level::warning, vm_name, fmt::format("Image has {} suspension snapshot, but the state is {}", | ||
| has_suspend_snapshot ? "a" : "no", | ||
| static_cast<short>(state))); // clang-format on | ||
|
|
There was a problem hiding this comment.
The only problem of this is the integer value returns to us via static_cast<short>(state), however, enum -> string map or a switch statement may not worth it.
There was a problem hiding this comment.
Right, this is what my comment above about formatters is about. Ideally, we'd write a Formatter for VM::State... see above.
Condense code to log mismatches between suspension snapshot and VM state. Just trying to reduce the space that is taken by logging code, which dilutes actual logic for the reader.
This could be further improved by adding a formatter for the State enum to
multipass/format.h. That would be useful in other places too, but we should then probably also move the corresponding logic from the CLI formatters, so leaving that for now.