From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from us-smtp-delivery-124.mimecast.com (us-smtp-delivery-124.mimecast.com [216.205.24.124]) by mx.groups.io with SMTP id smtpd.web11.7971.1623246391026833104 for ; Wed, 09 Jun 2021 06:46:31 -0700 Authentication-Results: mx.groups.io; dkim=pass header.i=@redhat.com header.s=mimecast20190719 header.b=SmOWC5Xc; spf=pass (domain: redhat.com, ip: 216.205.24.124, mailfrom: lersek@redhat.com) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1623246390; h=from:from:reply-to:subject:subject:date:date:message-id:message-id: to:to:cc:cc:mime-version:mime-version:content-type:content-type: content-transfer-encoding:content-transfer-encoding: in-reply-to:in-reply-to:references:references; bh=OBFsVHXV2cIC9sfH8jB8YalrI/n4qUd/M1Q3lZRL/rI=; b=SmOWC5Xc1JB+STaCBlGMDoO6B0WdmI0EDH1aE88xDD0nfswNnVM2i5bHEEEfX3BB2awmU4 bhtbfh9K+khlDHIu6JwuKemL/Z4GlFBs9BCyH/von0HrtBGzxMbXobbxut0qQMTWa93geh wwBF5e/LDdhmaJRchuvhIY31zoj8gtM= 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-393-ooKpVY-iPtWPhzGsPsHLwg-1; Wed, 09 Jun 2021 09:46:27 -0400 X-MC-Unique: ooKpVY-iPtWPhzGsPsHLwg-1 Received: from smtp.corp.redhat.com (int-mx08.intmail.prod.int.phx2.redhat.com [10.5.11.23]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mimecast-mx01.redhat.com (Postfix) with ESMTPS id 4D6F0801B1D; Wed, 9 Jun 2021 13:46:25 +0000 (UTC) Received: from lacos-laptop-7.usersys.redhat.com (ovpn-113-249.ams2.redhat.com [10.36.113.249]) by smtp.corp.redhat.com (Postfix) with ESMTPS id 21B4919C46; Wed, 9 Jun 2021 13:46:23 +0000 (UTC) Subject: Re: [PATCH 3/6] NetworkPkg/IScsiDxe: distinguish "maximum" and "selected" CHAP digest sizes To: =?UTF-8?Q?Philippe_Mathieu-Daud=c3=a9?= , edk2-devel-groups-io Cc: Jiaxin Wu , Maciej Rabeda , Siyuan Fu References: <20210608130652.2434-1-lersek@redhat.com> <20210608130652.2434-4-lersek@redhat.com> <299ff69a-7d20-c2bc-ca51-c07743a48075@redhat.com> From: "Laszlo Ersek" Message-ID: Date: Wed, 9 Jun 2021 15:46:22 +0200 MIME-Version: 1.0 In-Reply-To: <299ff69a-7d20-c2bc-ca51-c07743a48075@redhat.com> X-Scanned-By: MIMEDefang 2.84 on 10.5.11.23 Authentication-Results: relay.mimecast.com; auth=pass smtp.auth=CUSA124A263 smtp.mailfrom=lersek@redhat.com X-Mimecast-Spam-Score: 0 X-Mimecast-Originator: redhat.com Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 8bit On 06/09/21 12:43, Philippe Mathieu-Daudé wrote: > Hi Laszlo, > > On 6/8/21 3:06 PM, Laszlo Ersek wrote: >> IScsiDxe uses the ISCSI_CHAP_RSP_LEN macro for expressing the size of the >> digest (16) that it solely supports at this point (MD5). >> ISCSI_CHAP_RSP_LEN is used for both (a) *allocating* digest-related >> buffers (binary buffers and hex encodings alike), and (b) *processing* >> binary digest buffers (comparing them, filling them, reading them). >> >> In preparation for adding other hash algorithms, split purpose (a) from >> purpose (b). For purpose (a) -- buffer allocation --, introduce >> ISCSI_CHAP_MAX_DIGEST_SIZE. For purpose (b) -- processing --, rely on >> MD5_DIGEST_SIZE from . > > Matter of taste probably, I'd rather see this patch split in 2, as you > identified. (b) first then (a). Regardless: > Reviewed-by: Philippe Mathieu-Daude Interesting; I thought that showing all uses of ISCSI_CHAP_RSP_LEN in one patch, and classifying each use one way or another at once, was the best for reviewer understanding. Basically it's a single "mental loop" over all uses, and in the "loop body", we have an "if" (classification) -- allocation vs. processing. What you propose is basically "two loops". In that approach, in the first patch (= the first "mental loop"), only "processing" uses would be updated; the "allocation sites" wouldn't be shown at all. I feel that this approach is counter-intuitive: >>From the body of the first patch, - the reviewer can check the *correctness* of the patch (that is, whether everything that I converted is indeed "processing"), - but they can't check the *completeness* of the patch (that is, whether there is a "processing" site that I should have converted, but missed). For the reviewer to verify the completeness of the first patch, they have to apply it (or check out the branch at that stage), and go over all the *remaining* ISCSI_CHAP_RSP_LEN instances, to see if I missed something. And, if the reviewer has to check every single instance of ISCSI_CHAP_RSP_LEN right after the first patch, they end up doing the same work as if they had just reviewed this particular patch. I think your approach boils down to the following idea: The completeness of the first patch would be proved by the correctness of the second patch. That is, *after* you reviewed the second patch (and see that every site converted is indeed an allocation site, and that the ISCSI_CHAP_RSP_LEN macro definition is being removed, so no other ISCSI_CHAP_RSP_LEN instance remains), you could be sure that no processing site was missed in the first patch. Technically / mathematically, this is entirely true; I just prefer avoiding situations where you have to review patch (N+X) to see the validity (completeness) of patch (N). I quite dislike jumping between patches during review. Does my explanation make sense? If you still prefer the split, I'm OK to do it. Thanks! Laszlo > >> Distinguishing these purposes is justified because purpose (b) -- >> processing -- must depend on the hashing algorithm negotiated between >> initiator and target, while for purpose (a) -- allocation --, using the >> maximum supported digest size is suitable. For now, because only MD5 is >> supported, introduce ISCSI_CHAP_MAX_DIGEST_SIZE *as* MD5_DIGEST_SIZE. >> >> Note that the argument for using the digest size as the size of the >> outgoing challenge (in case mutual authentication is desired by the >> initiator) remains in place. Because of this, the above two purposes are >> distinguished for the "ISCSI_CHAP_AUTH_DATA.OutChallenge" field as well. >> >> This patch is functionally a no-op, just yet. >> >> Cc: Jiaxin Wu >> Cc: Maciej Rabeda >> Cc: Philippe Mathieu-Daudé >> Cc: Siyuan Fu >> Ref: https://bugzilla.tianocore.org/show_bug.cgi?id=3355 >> Signed-off-by: Laszlo Ersek >> --- >> NetworkPkg/IScsiDxe/IScsiCHAP.h | 17 +++++++++------ >> NetworkPkg/IScsiDxe/IScsiCHAP.c | 22 ++++++++++---------- >> 2 files changed, 22 insertions(+), 17 deletions(-) >