From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from us-smtp-delivery-1.mimecast.com (us-smtp-delivery-1.mimecast.com [207.211.31.81]) by mx.groups.io with SMTP id smtpd.web10.357.1585691819775337846 for ; Tue, 31 Mar 2020 14:56:59 -0700 Authentication-Results: mx.groups.io; dkim=pass header.i=@redhat.com header.s=mimecast20190719 header.b=MnfTgJVQ; spf=pass (domain: redhat.com, ip: 207.211.31.81, mailfrom: lersek@redhat.com) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1585691818; 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=vLQOWIjeRqhAwAEnIMSvXgYeMXcLy4mnMienne8t49o=; b=MnfTgJVQKMgK7HTZPZniH4gR8U23gUPXKPQUXHd/g9czB9FTej1fYDcRkmWV9CFxdrUU70 IbWaGND1/jMyiIxC8Bx5JPPEkCk3u4EzEJ89EjqmlrYoiz8P9dygje2DWue/ZCC7yftNNr LMH8tsQn1dCh0/EHatro6wDpZ5lmoB0= 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-457-Frukzs8AOLCMcU13MsDl6w-1; Tue, 31 Mar 2020 17:56:54 -0400 X-MC-Unique: Frukzs8AOLCMcU13MsDl6w-1 Received: from smtp.corp.redhat.com (int-mx03.intmail.prod.int.phx2.redhat.com [10.5.11.13]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mimecast-mx01.redhat.com (Postfix) with ESMTPS id 71B9CDB82; Tue, 31 Mar 2020 21:56:53 +0000 (UTC) Received: from lacos-laptop-7.usersys.redhat.com (ovpn-115-131.ams2.redhat.com [10.36.115.131]) by smtp.corp.redhat.com (Postfix) with ESMTP id A6F2160BFE; Tue, 31 Mar 2020 21:56:52 +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: <20200331110452.51992-1-liran.alon@oracle.com> <22866.1585670000047029434@groups.io> From: "Laszlo Ersek" Message-ID: <4b90b41a-2cf1-8b6e-6619-1c652820265e@redhat.com> Date: Tue, 31 Mar 2020 23:56:51 +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: <22866.1585670000047029434@groups.io> X-Scanned-By: MIMEDefang 2.79 on 10.5.11.13 X-Mimecast-Spam-Score: 0 X-Mimecast-Originator: redhat.com Content-Language: en-US Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: 7bit On 03/31/20 17:53, Sean via Groups.Io wrote: > A couple of thoughts. > 1. I would suggest that ASSERT should not be the only protection for an invalid operation as ASSERT is usually disabled on release builds. > 2. We do have a library to make this more explicit and common. https://github.com/tianocore/edk2/blob/master/MdePkg/Include/Library/SafeIntLib.h#L548 In this case, when "Response->ScsiStatus" does not fit in "Packet->TargetStatus", the device model is obviously (and blatantly) misbehaving, so I would agree with Liran that trying to recover from that (or to cover it up with a nice error code passed out) is futile. I do agree with the observation however that ASSERT()s disappear from RELEASE builds. Mike Kinney taught me a pattern to deal with this. There are various ways to write it; one example (for this case) is: ASSERT (Response->ScsiStatus <= MAX_UINT8); if (Response->ScsiStatus > MAX_UINT8) { CpuDeadLoop (); } Packet->TargetStatus = (UINT8)Response->ScsiStatus; An alternative way to write it is (by moving the ASSERT into the block): if (Response->ScsiStatus > MAX_UINT8) { ASSERT (Response->ScsiStatus <= MAX_UINT8); CpuDeadLoop (); } Packet->TargetStatus = (UINT8)Response->ScsiStatus; Yet another (simply assert FALSE in the block): if (Response->ScsiStatus > MAX_UINT8) { ASSERT (FALSE); CpuDeadLoop (); } Packet->TargetStatus = (UINT8)Response->ScsiStatus; Why: - in DEBUG builds, the assertion failure will be logged, and the proper assertion failure action will be triggered (CpuDeadLoop / exception / ..., as configured by the platform) - in RELEASE builds, we'll still hang, and might have a chance to investigate (get a stack dump perhaps). Regarding SafeIntLib, I'm a fan in general. In this case, I did not think of it (possible integer truncations seem so rare in this driver). For this patch, I'm OK either way (with or without using SafeIntLib), as long as we add both the ASSERT and the explicit CpuDeadLoop (either variant of the three). Thanks Laszlo