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 <BaseCryptLib.h>. > > 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 <phi...@redhat.com>
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 <jiaxin...@intel.com> >> Cc: Maciej Rabeda <maciej.rab...@linux.intel.com> >> Cc: Philippe Mathieu-Daudé <phi...@redhat.com> >> Cc: Siyuan Fu <siyuan...@intel.com> >> Ref: https://bugzilla.tianocore.org/show_bug.cgi?id=3355 >> Signed-off-by: Laszlo Ersek <ler...@redhat.com> >> --- >> NetworkPkg/IScsiDxe/IScsiCHAP.h | 17 +++++++++------ >> NetworkPkg/IScsiDxe/IScsiCHAP.c | 22 ++++++++++---------- >> 2 files changed, 22 insertions(+), 17 deletions(-) > -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#76275): https://edk2.groups.io/g/devel/message/76275 Mute This Topic: https://groups.io/mt/83395026/21656 Group Owner: devel+ow...@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com] -=-=-=-=-=-=-=-=-=-=-=-
