From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from us-smtp-delivery-1.mimecast.com (us-smtp-delivery-1.mimecast.com [205.139.110.61]) by mx.groups.io with SMTP id smtpd.web11.3649.1585738949194985901 for ; Wed, 01 Apr 2020 04:02:29 -0700 Authentication-Results: mx.groups.io; dkim=pass header.i=@redhat.com header.s=mimecast20190719 header.b=Tl4/ZjKH; spf=pass (domain: redhat.com, ip: 205.139.110.61, mailfrom: lersek@redhat.com) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1585738948; h=from:from:reply-to:subject:subject:date:date:message-id:message-id: to:to:cc:mime-version:mime-version:content-type:content-type: content-transfer-encoding:content-transfer-encoding: in-reply-to:in-reply-to:references:references; bh=Ey1NuKGRP8sDFq4XjzNtDROtX+8H1vWkUJXFxYUHE/s=; b=Tl4/ZjKHzHqTcszJrcjauOqlG/Yl38HH7nHln9uesgd5jX42Eq5fuJ/WcQhal5VuNPRDO4 qHcpCAvvka89MjHmC8f11UZXJwm8ljp33pUtrQJi4i+xAB54ZFccTqeSDTXjdd33jrHcjk LbdX3TiIlpnbT2YNexZRmyymlFVDCqU= Received: from mimecast-mx01.redhat.com (mimecast-mx01.redhat.com [209.132.183.4]) (Using TLS) by relay.mimecast.com with ESMTP id us-mta-298-oJ2m5-bUPbGxeBV8rfVqYw-1; Wed, 01 Apr 2020 07:02:23 -0400 X-MC-Unique: oJ2m5-bUPbGxeBV8rfVqYw-1 Received: from smtp.corp.redhat.com (int-mx06.intmail.prod.int.phx2.redhat.com [10.5.11.16]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mimecast-mx01.redhat.com (Postfix) with ESMTPS id 992F518C8C0E; Wed, 1 Apr 2020 11:02:22 +0000 (UTC) Received: from lacos-laptop-7.usersys.redhat.com (ovpn-114-94.ams2.redhat.com [10.36.114.94]) by smtp.corp.redhat.com (Postfix) with ESMTP id 80FFD5C1B0; Wed, 1 Apr 2020 11:02:21 +0000 (UTC) Subject: Re: [edk2-devel] [PATCH] OvmfPkg/PvScsiDxe: Fix VS2019 build error because of implicit cast To: devel@edk2.groups.io, sean.brogan@microsoft.com, Liran Alon References: <30a57406-0c09-043f-6b19-7e6e94afcb44@oracle.com> <7506.1585695267608228111@groups.io> From: "Laszlo Ersek" Message-ID: Date: Wed, 1 Apr 2020 13:02:20 +0200 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Thunderbird/52.9.1 MIME-Version: 1.0 In-Reply-To: <7506.1585695267608228111@groups.io> X-Scanned-By: MIMEDefang 2.79 on 10.5.11.16 X-Mimecast-Spam-Score: 0 X-Mimecast-Originator: redhat.com Content-Language: en-US Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: quoted-printable On 04/01/20 00:54, Sean via Groups.Io wrote: > I agree that safeintlib is not doing anything too interesting in this cas= e but that's not really the point.=C2=A0 The argument for it is that it bec= omes the central point of code to check for safe conversions and an indicat= or that the developer was thoughtful about this conversion and didn't just = cast to avoid the compiler complaining.=C2=A0 If everyone starts putting th= eir own checks in place it leads to more code reviews, diversity in solutio= ns, and opportunities for bugs.=C2=A0 All that said those are soft reasons = for the change and that is up to you. >=20 > @Laszlo - On the ASSERT part, I have a different view point and am more c= urious about yours.=C2=A0 For release builds, I don't want to see CpuDeadLo= ops anywhere unless I am ok with the device being returned/refunded.=C2=A0 = Our error path would be to exit the function with an error code and potenti= ally log a ReportStatusCode.=C2=A0 =C2=A0I don't think you should continue = in an invalid state as that just makes resolving the bug much much harder.= =C2=A0 =C2=A0 Given that the system can boot to at least a menu without thi= s 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. >=20 > Finally, given that this is contained in OVMF I am fine with whatever mak= es 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