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

["Laszlo Ersek" <lersek@redhat.com>]

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(-)
>