Re: [PATCH 3/6] NetworkPkg/IScsiDxe: distinguish "maximum" and "selected" CHAP digest sizes

["Maciej Rabeda" <maciej.rabeda@linux.intel.com>]

To cut to the chase on this patch:
Reviewed-by: Maciej Rabeda <maciej.rabeda@linux.intel.com>

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 <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 <philmd@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.wu@intel.com>
>>> Cc: Maciej Rabeda <maciej.rabeda@linux.intel.com>
>>> Cc: Philippe Mathieu-Daudé <philmd@redhat.com>
>>> Cc: Siyuan Fu <siyuan.fu@intel.com>
>>> Ref: https://bugzilla.tianocore.org/show_bug.cgi?id=3355
>>> Signed-off-by: Laszlo Ersek <lersek@redhat.com>
>>> ---
>>>   NetworkPkg/IScsiDxe/IScsiCHAP.h | 17 +++++++++------
>>>   NetworkPkg/IScsiDxe/IScsiCHAP.c | 22 ++++++++++----------
>>>   2 files changed, 22 insertions(+), 17 deletions(-)