From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from mga02.intel.com (mga02.intel.com [134.134.136.20]) by mx.groups.io with SMTP id smtpd.web08.7281.1623411500157833882 for ; Fri, 11 Jun 2021 04:38:20 -0700 Authentication-Results: mx.groups.io; dkim=missing; spf=none, err=permanent DNS error (domain: linux.intel.com, ip: 134.134.136.20, mailfrom: maciej.rabeda@linux.intel.com) IronPort-SDR: ouAOCn40o4yZi0eMkpfMTCVuE+FL2xZi/ohhWLiruX+Ba4/IPNcgOpcGZzcossXhyLeBFruUqB VKKRD7Xt/lQg== X-IronPort-AV: E=McAfee;i="6200,9189,10011"; a="192619808" X-IronPort-AV: E=Sophos;i="5.83,265,1616482800"; d="scan'208";a="192619808" Received: from orsmga001.jf.intel.com ([10.7.209.18]) by orsmga101.jf.intel.com with ESMTP/TLS/ECDHE-RSA-AES256-GCM-SHA384; 11 Jun 2021 04:38:19 -0700 IronPort-SDR: 8GVq5Gf8zJUqzsxXtzDlgrYNlk4AODWINoaNfuL1Hmy0fT+VAhsTnwZd7T23Htb4uZ/0X8YsgV +j6nspe1b/9A== X-IronPort-AV: E=Sophos;i="5.83,265,1616482800"; d="scan'208";a="483237357" Received: from mrabeda-mobl.ger.corp.intel.com (HELO [10.213.2.3]) ([10.213.2.3]) by orsmga001-auth.jf.intel.com with ESMTP/TLS/ECDHE-RSA-AES256-GCM-SHA384; 11 Jun 2021 04:38:17 -0700 Subject: Re: [PATCH 3/6] NetworkPkg/IScsiDxe: distinguish "maximum" and "selected" CHAP digest sizes To: Laszlo Ersek , =?UTF-8?Q?Philippe_Mathieu-Daud=c3=a9?= , edk2-devel-groups-io Cc: Jiaxin Wu , Siyuan Fu References: <20210608130652.2434-1-lersek@redhat.com> <20210608130652.2434-4-lersek@redhat.com> <299ff69a-7d20-c2bc-ca51-c07743a48075@redhat.com> From: "Maciej Rabeda" Message-ID: <648a6743-0311-d696-2f21-543578023e9f@linux.intel.com> Date: Fri, 11 Jun 2021 13:38:14 +0200 User-Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:78.0) Gecko/20100101 Thunderbird/78.11.0 MIME-Version: 1.0 In-Reply-To: Content-Type: text/plain; charset=utf-8; format=flowed Content-Transfer-Encoding: 8bit Content-Language: pl To cut to the chase on this patch: Reviewed-by: Maciej Rabeda On 09-Jun-21 15:46, Laszlo Ersek wrote: > 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(-)