Re: [Cbor] 🔔 WGLC with request for reviews on cbor-network-addresses-05

Carsten Bormann <cabo@tzi.org> Sat, 17 July 2021 20:31 UTC

Return-Path: <cabo@tzi.org>
X-Original-To: cbor@ietfa.amsl.com
Delivered-To: cbor@ietfa.amsl.com
Received: from localhost (localhost [127.0.0.1]) by ietfa.amsl.com (Postfix) with ESMTP id 367823A203B for <cbor@ietfa.amsl.com>; Sat, 17 Jul 2021 13:31:54 -0700 (PDT)
X-Virus-Scanned: amavisd-new at amsl.com
X-Spam-Flag: NO
X-Spam-Score: -1.899
X-Spam-Level:
X-Spam-Status: No, score=-1.899 tagged_above=-999 required=5 tests=[BAYES_00=-1.9, RCVD_IN_DNSWL_BLOCKED=0.001, SPF_HELO_NONE=0.001, SPF_PASS=-0.001] autolearn=unavailable autolearn_force=no
Received: from mail.ietf.org ([4.31.198.44]) by localhost (ietfa.amsl.com [127.0.0.1]) (amavisd-new, port 10024) with ESMTP id kosq7WCItE_D for <cbor@ietfa.amsl.com>; Sat, 17 Jul 2021 13:31:49 -0700 (PDT)
Received: from gabriel-smtp.zfn.uni-bremen.de (gabriel-smtp.zfn.uni-bremen.de [IPv6:2001:638:708:32::15]) (using TLSv1.2 with cipher ADH-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by ietfa.amsl.com (Postfix) with ESMTPS id 4D0503A203C for <cbor@ietf.org>; Sat, 17 Jul 2021 13:31:49 -0700 (PDT)
Received: from [192.168.217.118] (p548dcc89.dip0.t-ipconnect.de [84.141.204.137]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by gabriel-smtp.zfn.uni-bremen.de (Postfix) with ESMTPSA id 4GS0Bn5fFTz2xL3; Sat, 17 Jul 2021 22:31:45 +0200 (CEST)
Content-Type: text/plain; charset=utf-8
Mime-Version: 1.0 (Mac OS X Mail 13.4 \(3608.120.23.2.7\))
From: Carsten Bormann <cabo@tzi.org>
In-Reply-To: <06a083dc-b8d4-6057-ddd0-1b81943bd326@ri.se>
Date: Sat, 17 Jul 2021 22:31:45 +0200
Cc: cbor@ietf.org
X-Mao-Original-Outgoing-Id: 648246705.257023-4738e25a183472f0488773225d553143
Content-Transfer-Encoding: quoted-printable
Message-Id: <1B30F98F-F8E5-4A51-AF85-B48D5D16F559@tzi.org>
References: <YPGg6rnN6H3feUYx@hephaistos.amsuess.com> <CALaySJK4-xQLpcaucyMb=w1b27X+VEwMQn-8ONawRJ4bJ=rWQQ@mail.gmail.com> <06a083dc-b8d4-6057-ddd0-1b81943bd326@ri.se>
To: Marco Tiloca <marco.tiloca=40ri.se@dmarc.ietf.org>
X-Mailer: Apple Mail (2.3608.120.23.2.7)
Archived-At: <https://mailarchive.ietf.org/arch/msg/cbor/SkkIJUj01zYox-fPXQn4ZvTsb5g>
Subject: Re: [Cbor] =?utf-8?q?=F0=9F=94=94_WGLC_with_request_for_reviews_on_c?= =?utf-8?q?bor-network-addresses-05?=
X-BeenThere: cbor@ietf.org
X-Mailman-Version: 2.1.29
Precedence: list
List-Id: "Concise Binary Object Representation \(CBOR\)" <cbor.ietf.org>
List-Unsubscribe: <https://www.ietf.org/mailman/options/cbor>, <mailto:cbor-request@ietf.org?subject=unsubscribe>
List-Archive: <https://mailarchive.ietf.org/arch/browse/cbor/>
List-Post: <mailto:cbor@ietf.org>
List-Help: <mailto:cbor-request@ietf.org?subject=help>
List-Subscribe: <https://www.ietf.org/mailman/listinfo/cbor>, <mailto:cbor-request@ietf.org?subject=subscribe>
X-List-Received-Date: Sat, 17 Jul 2021 20:31:54 -0000

Hi Marco,

Thank you for your quick and detailed comments.

> [Section 1]
> 
> * The normative statement "Prefixes MUST omit ..." feels strange for the introduction, also as the recap of BCP14 terminology comes after that. I suggest to rather use "must omit" or even just "omit"; the intended normative text comes later on in Section 3.1 and 3.2 anyway.

I like the simple “omit”.

> [Section 3]
> 
> * Maybe some details can be made clearer in the third from last paragraph. For instance, it can be easier here to think only in terms of bit lengths. Proposed rephrasing
> 
>    "Then, taking whichever is smaller between the length in bits of the included byte-string and the prefix-length possibly rounded up to the next multiple of 8, copy that many bits from the byte-string into the array."

People won’t know how to copy bits.

So here is my merged text:

Then taking whichever is smaller between (a) the length of the
included byte-string, and (b) the number of bytes covered by the
prefix-length rounded up to the next multiple of 8: fail if that
number is greater than 16 (or 4), and then copy that many bytes from
the byte-string into the array.

Might be easier in pseudocode :-)

> * I guess "of 8 values" should be "with value 8" ?

Well, I’d not use a static array of 8 values here (and why let the readers guess about that array?), I’d simply use 0xFF << unused_bits:

unused_bits = (-prefix_length_in_bits) & 7;
if (length_in_bytes > 0)
  address_bytes[length_in_bytes - 1] &= (0xFF << unused_bits);

Or if you want to check:

if (length_in_bytes > 0 &&
    (address_bytes[length_in_bytes - 1] & ~(0xFF << unused_bits))
    != 0)
  fail();

Writing a test for this code is left as an exercise for the reader :-)

I’m not sure we even want to cover the masking out case; we should tell people to fail on incorrectly set bits (i.e., tag is invalid).

> == Nits ==

(Done unless mentioned:)

> In the second and third paragraph, I suggest to use only "document" rather than "specification", consistently with the rest of the text.

I went the other direction (document ➔ specification).

> s/set it/and then set it

I rephrased this and the next nit as above.

Now in https://github.com/cbor-wg/cbor-network-address/pull/5

Grüße, Carsten