Add descriptive error messages to unwrap()s

[aghecenco]

There are remaining unwrap()s in the code that cannot be avoided, which lead to a not-so-user-friendly error message being printed. All such failure cases should print a descriptive message along with the panic location.

[raduweiss]

Lowering priority since this is not very impactful in practice.

[cmannett85]

Is this just a case of replacing the unwrap() calls with expect()? Is there a documented requirement for the formatting of the error message?

[acatangiu]

Hi @cmannett85, we do not have formal requirements for formatting error messages. We do however try to keep them short and clear and also try to reuse the same static strings so that we minimize the binary size.

For example most expect()s following a mutex.lock() use "Poisoned lock" as the description with no extra sugarcoating. We rely on the stacktrace to provide the location of the error, and use the description as context.

This issue is indeed a case of replacing unwrap() calls with expect().

[cmannett85]

I've made the changes I think are required, but I've hit across an unexpected issue. Because I've added an error string, this has pushed some lines over the column limit for the rust style, so cargo fmt has split them over multiple lines. Unfortunately this has pushed the code coverage percentage below the minimum:

=============================================== FAILURES ================================================
_____________________________________________ test_coverage _____________________________________________
integration_tests/build/test_coverage.py:94: in test_coverage
    assert coverage >= min_coverage, coverage_low_msg
E   AssertionError: Current code coverage (82.32%) is below the target (82.46%).
E   assert 82.32124920098622 >= 82.41

What do I do? I haven't made any changes that warrant new unit tests.

[bbros-dev]

@cmannett85, did you ever get a response, or did these changes land?

[cmannett85]

@cmannett85, did you ever get a response, or did these changes land?

Yes, they're in the PR listed above.

[bbros-dev]

@iulianbarbu makes a good point re user guidance:
#1740 (review)

I wonder if the approach of rustc and various linters can be adopted, where a error 'code', FC001, is returned with a url setting things out in more detail?

This way the error messages are not burdened by having to tell a sensible/useful "story" about what went wrong when these messages are read in context/sequence. All that context and detail can be set out at the URL

A further advantage of this is these error strings are kept brief and that can help relieve the overhead in tracing contexts where a error message is recorded in a span field.

[roypat]

We've discussed this with the team today, and decided to close this issue without further action. The preferred way to handle user-facing errors is by propagating them all the way up to main, so that Firecracker can exit gracefully. Firecracker should never panic. Thus, whether we use .unwrap() or .expect() does not really make a difference - in either case, it means something has fundamentally gone wrong. While .expect() can document what exactly assumption was violated, a normal code comment gets this job done just as well. So we see fairly little value in converting all unwraps to expects (also because a lot of our unwraps are due to numerical conversions that we know statically cannot fail, and where expect would just add noise to the code).