From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from us-smtp-delivery-124.mimecast.com (us-smtp-delivery-124.mimecast.com [170.10.133.124]) by mx.groups.io with SMTP id smtpd.web11.12142.1624891458767231656 for ; Mon, 28 Jun 2021 07:44:18 -0700 Authentication-Results: mx.groups.io; dkim=pass header.i=@redhat.com header.s=mimecast20190719 header.b=KY1OjbsR; spf=pass (domain: redhat.com, ip: 170.10.133.124, mailfrom: lersek@redhat.com) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1624891457; 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=sMtD3Lu84oyLIsPMMwS5j9x6J4sndMXxvoNZ43XbtvY=; b=KY1OjbsRJ5MJfgMvZZE/z4c4BXRu7gb5VgW3tlTd/znOLpsf/qgqk4hbIU4BP950FmrR3Q cp+MfQOFlCQMH+bcpqTWudbd86KZrZoagpnGgZ3pwKLfppr3kLMXYyGQsgV3ytHcRsiiIK 3W4n1GbovcwRRLlC9cqotcHsO2dvV4Y= 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-450-TjrK-2JJPuO5SSBjjKp-GQ-1; Mon, 28 Jun 2021 10:44:14 -0400 X-MC-Unique: TjrK-2JJPuO5SSBjjKp-GQ-1 Received: from smtp.corp.redhat.com (int-mx06.intmail.prod.int.phx2.redhat.com [10.5.11.16]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mimecast-mx01.redhat.com (Postfix) with ESMTPS id 13DE581C85F; Mon, 28 Jun 2021 14:44:13 +0000 (UTC) Received: from lacos-laptop-7.usersys.redhat.com (ovpn-114-81.ams2.redhat.com [10.36.114.81]) by smtp.corp.redhat.com (Postfix) with ESMTPS id 98A5E5C1D0; Mon, 28 Jun 2021 14:44:11 +0000 (UTC) Subject: Re: [edk2-devel] [PATCH 4/6] NetworkPkg/IScsiDxe: support multiple hash algorithms for CHAP To: "Rabeda, Maciej" , devel@edk2.groups.io Cc: Jiaxin Wu , =?UTF-8?Q?Philippe_Mathieu-Daud=c3=a9?= , Siyuan Fu References: <20210608130652.2434-1-lersek@redhat.com> <20210608130652.2434-5-lersek@redhat.com> <518cfc75-7816-c8e1-21f6-5ade8cbc9b39@redhat.com> From: "Laszlo Ersek" Message-ID: <2123d827-22e4-0abe-c8ff-8ad70d624e00@redhat.com> Date: Mon, 28 Jun 2021 16:44:09 +0200 MIME-Version: 1.0 In-Reply-To: X-Scanned-By: MIMEDefang 2.79 on 10.5.11.16 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: 7bit Hi Maciej, [snipping liberally, comments below] On 06/25/21 16:56, Rabeda, Maciej wrote: > On 22-Jun-21 17:57, Laszlo Ersek wrote: >> On 06/11/21 13:54, Rabeda, Maciej wrote: >>> On 08-Jun-21 15:06, Laszlo Ersek wrote: >>>> +typedef struct { >>>> + UINT8 Algorithm; // >>>> ISCSI_CHAP_ALGORITHM_*, CHAP_A >>> Any chance to define an enum type for Algorithm? IMHO it brings more >>> meaning to that number and limits the set of values it is expected >>> to have. >> I picked UINT8 intentionally; I wanted to stay close to the >> definition at >> >> https://www.iana.org/assignments/ppp-numbers/ppp-numbers.xhtml#ppp-numbers-9 >> >> >> which says, "A one octet field". (Hence the reference to "CHAP_A" in >> the comment as well.) >> >> If we want an enum here, then I need to prepend a patch, for first >> replacing the existent ISCSI_CHAP_ALGORITHM_MD5 macro with an enum >> type / constant. >> >> I still like UINT8 for this, but if you strongly prefer the enum, I >> can certainly do it. Please advise :) > Okay, let's stick to UINT8 based on IANA definition. Thanks! >>>> - if (CompareMem (VerifyRsp, TargetResponse, MD5_DIGEST_SIZE) != >>>> 0) { >>>> + if (CompareMem (VerifyRsp, TargetResponse, >>>> + AuthData->Hash->DigestSize) != 0) { >>>> Status = EFI_SECURITY_VIOLATION; >>>> } >>> Either break like regular function call or leave it in a single line >>> for readability, since UEFI coding standard section 5.1.1 allows for >>> lines up to 120. I opt for the latter, since param break will look >>> ugly, but if it looks better in your editor than a line over 80 >>> chars in size - go for it. >> My mistake, sorry -- I forgot that the "condensed style" that we >> actually prefer in ArmVirtPkg and OvmfPkg is not accepted in other >> packages. > Making it the other way around. Apart from historical reasons (EDK2 > coding style evolving) - why is a different style accepted in other > packages? We have a EDK2 coding style document for a reason. > If it is not perfect, it does not suit our needs or simply hurts our > eyes (which 'param per line' in if statements does to me), we can try > to change it to the "condensed style". * Submitted on 11 Aug 2017: [edk2] [edk2-CCodingStandardsSpecification PATCH 0/2] improvements related to line wrapping [edk2] [edk2-CCodingStandardsSpecification PATCH 1/2] Source Files / General Rules: limit line lengths to 80 columns [edk2] [edk2-CCodingStandardsSpecification PATCH 2/2] Source Files / Spacing / Multi-line func. calls: allow condensed arguments http://mid.mail-archive.com/20170811164851.9466-1-lersek@redhat.com http://mid.mail-archive.com/20170811164851.9466-2-lersek@redhat.com http://mid.mail-archive.com/20170811164851.9466-3-lersek@redhat.com The condensed arguments style is described in patch 2/2. * Filed in September 2017, from the discussion under patch 2/2: https://bugzilla.tianocore.org/show_bug.cgi?id=714 In CONFIRMED status currently. (The link to the original email discussion (in comment#0 of the BZ) no longer works, because Intel has killed off the old archive links meanwhile. But the links should still work.) Thanks Laszlo