Re: [edk2-devel] [PATCH] OvmfPkg/PvScsiDxe: Fix VS2019 build error because of implicit cast

["Laszlo Ersek" <lersek@redhat.com>]

On 04/01/20 00:54, Sean via Groups.Io wrote:
> I agree that safeintlib is not doing anything too interesting in this case but that's not really the point.  The argument for it is that it becomes the central point of code to check for safe conversions and an indicator that the developer was thoughtful about this conversion and didn't just cast to avoid the compiler complaining.  If everyone starts putting their own checks in place it leads to more code reviews, diversity in solutions, and opportunities for bugs.  All that said those are soft reasons for the change and that is up to you.
> 
> @Laszlo - On the ASSERT part, I have a different view point and am more curious about yours.  For release builds, I don't want to see CpuDeadLoops anywhere unless I am ok with the device being returned/refunded.  Our error path would be to exit the function with an error code and potentially log a ReportStatusCode.   I don't think you should continue in an invalid state as that just makes resolving the bug much much harder.    Given that the system can boot to at least a menu without this driver, it seems that failing out of the function would provide a better "RELEASE" experience.

Good points!

(1) There is a big difference between physical firmware and virtual
firmware. Similar difference between physical hardware and virtual
hardware. With virtualization, the "return/refund" case maps to "fix
OVMF, or fix QEMU". In particular the "fix QEMU" part is important,
because the assertion failure would show that QEMU is really broken, and
needs fixing. Thankfully, QEMU is fixable, and in that case, it *should*
be fixed; the "return/refund" path is both the right one, and not as
painful as with physical devices.

To exaggerate your point a bit, I would agree that ASSERT()s have no
place in space-craft software! :)

(2) I have actually considered -- at length -- returning an error code
from this function, instead of the failed assert / CpuDeadLoop(). The
error code from this function would be propagated out through the ext
scsi PassThru method. For that reason, I reviewed the error codes
defined for that protocol member function in the UEFI spec. All error
codes except EFI_TIMEOUT claim that "the SCSI operation was not sent /
not executed", therefore they do not apply in this case -- per spec, we
could only return EFI_TIMEOUT in case we fail to understand the PVSCSI
device model's response (because the device breaks "data sheet
compliance"). While I do think EFI_TIMEOUT is a *good* error code in
this case ultimately -- it shows that we simply can't tell whatever
happened on the device's side --, I figured (given point (1)) it would
be a very unwelcome complication for the code. Because upon returning
EFI_TIMEOUT, we also have to set HostAdapterStatus / TargetStatus to
something sensible (i.e., "fake"), and SenseDataLength to zero. Doable,
but didn't seem worth the churn.

(3) In RHEL OVMF packages, I exclusively ship DEBUG builds. For two
reasons: (a) many places in edk2 core code still use ASSERT() in place
of run-of-the-mill error checking (unfortunately), i.e., not explicit
"if"s; (b) bugs in software are the norm, not the exception, therefore
IMO debugging features should be an unalienable part of every software
package ever shipped to users. (If I could by my consumer electronics /
gadgets with full debug code enabled, I would, too.) When a RHEL user
reports an error, I don't want to start every one of my analyses with
the question, "can you please reproduce this with a debug build first".

I'm aware that debug code has additional cost (larger code size, perhaps
more CPU cycles spent, which is especially important on mobile devices,
yes). In my use case, the slowdown is not egregious -- first, most of
the boot time impact due to DEBUGs can be attributed to QEMU IO port
accesses, and that port is dynamically configurable (disabled by
default), so if it's not enabled, there's virtually no impact; second,
there don't seem to be insanely costly invariant checks built into OVMF
for the DEBUG target either. So I'm consciously committed to shipping
DEBUG only, and I admit that it does bias my thinking about RELEASE --
not that I'm against RELEASE *in general* for edk2, or ignoring RELEASE
willfully, it's just that I likely have blind spots wrt. RELEASE builds.

> 
> Finally, given that this is contained in OVMF I am fine with whatever makes the most sense for your platform and usecase.

Thanks a lot for confirming! In this case I'll go ahead with this patch.

Thanks!
Laszlo