From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from us-smtp-delivery-44.mimecast.com (us-smtp-delivery-44.mimecast.com [205.139.111.44]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by smtp.subspace.kernel.org (Postfix) with ESMTPS id 2B8281BC065 for ; Wed, 31 Jul 2024 17:12:01 +0000 (UTC) Authentication-Results: smtp.subspace.kernel.org; arc=none smtp.client-ip=205.139.111.44 ARC-Seal:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1722445924; cv=none; b=SBHMtl3KMfksuasBxuLEiEbWUO1L3xcmUWnCWQXwuyCfjDX4q4ipe2B6o7Kyqn9cr5kIbPh9sRNWI6n1U3LwxWWuvomCSUlrcUlbe6jCUxlIbZQtYq+ILDuwW8lGCHK4A+rnlXCM+nIFbjMJk9XVsgHYwkkqNUY3iuEMcOxJJJw= ARC-Message-Signature:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1722445924; c=relaxed/simple; bh=C+IfCW7zU1DmYobnFVqdAzVw94QfgfE22ecn9Qjmzqo=; h=Date:From:To:Cc:Subject:Message-ID:References:MIME-Version: In-Reply-To:Content-Type:Content-Disposition; b=EpGHaLjrP2XFf+VZVO+77jHjrvZHEmJdAtufD5XZIds263HjYdkrxgz4zbMBANsjLyLrgLV8Sx3TkBmH099Ff49whMe2qcNvi9q9DGmwc5DOMx0Q7MSMyxO1yMLee+umQDswuioxFe8VCXIeahQYH9lWnugm8AyIhCDsldQ6gVI= ARC-Authentication-Results:i=1; smtp.subspace.kernel.org; dmarc=none (p=none dis=none) header.from=queasysnail.net; spf=none smtp.mailfrom=queasysnail.net; arc=none smtp.client-ip=205.139.111.44 Authentication-Results: smtp.subspace.kernel.org; dmarc=none (p=none dis=none) header.from=queasysnail.net Authentication-Results: smtp.subspace.kernel.org; spf=none smtp.mailfrom=queasysnail.net Received: from mx-prod-mc-02.mail-002.prod.us-west-2.aws.redhat.com (ec2-54-186-198-63.us-west-2.compute.amazonaws.com [54.186.198.63]) by relay.mimecast.com with ESMTP with STARTTLS (version=TLSv1.3, cipher=TLS_AES_256_GCM_SHA384) id us-mta-656-ZM_ZAE1jOqqMVVa6i-34Yw-1; Wed, 31 Jul 2024 13:10:39 -0400 X-MC-Unique: ZM_ZAE1jOqqMVVa6i-34Yw-1 Received: from mx-prod-int-03.mail-002.prod.us-west-2.aws.redhat.com (mx-prod-int-03.mail-002.prod.us-west-2.aws.redhat.com [10.30.177.12]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature RSA-PSS (2048 bits) server-digest SHA256) (No client certificate requested) by mx-prod-mc-02.mail-002.prod.us-west-2.aws.redhat.com (Postfix) with ESMTPS id 76E481956088; Wed, 31 Jul 2024 17:10:37 +0000 (UTC) Received: from hog (unknown [10.39.192.3]) by mx-prod-int-03.mail-002.prod.us-west-2.aws.redhat.com (Postfix) with ESMTPS id 18D3719560AE; Wed, 31 Jul 2024 17:10:33 +0000 (UTC) Date: Wed, 31 Jul 2024 19:10:31 +0200 From: Sabrina Dubroca To: Christian Hopps Cc: devel@linux-ipsec.org, Steffen Klassert , netdev@vger.kernel.org, Christian Hopps Subject: Re: [PATCH ipsec-next v5 06/17] xfrm: add mode_cbs module functionality Message-ID: References: <20240714202246.1573817-1-chopps@chopps.org> <20240714202246.1573817-7-chopps@chopps.org> Precedence: bulk X-Mailing-List: netdev@vger.kernel.org List-Id: List-Subscribe: List-Unsubscribe: MIME-Version: 1.0 In-Reply-To: X-Scanned-By: MIMEDefang 3.0 on 10.30.177.12 X-Mimecast-Spam-Score: 0 X-Mimecast-Originator: queasysnail.net Content-Type: text/plain; charset=UTF-8 Content-Disposition: inline Content-Transfer-Encoding: quoted-printable 2024-07-30, 17:29:06 -0400, Christian Hopps wrote: >=20 > Sabrina Dubroca writes: >=20 > > 2024-07-14, 16:22:34 -0400, Christian Hopps wrote: > > > +struct xfrm_mode_cbs { > >=20 > > It would be nice to add kdoc for the whole thing. >=20 > Ok, I'll move the inline comments to a kdoc. FWIW, all the other structs = in this header, including the main `xfrm_state` struct use the same inline = comment documentation style I copied. Sure, but I don't think we should model new code on old habits. > > > diff --git a/net/xfrm/xfrm_input.c b/net/xfrm/xfrm_input.c > > > index 7cee9c0a2cdc..6ff05604f973 100644 > > > --- a/net/xfrm/xfrm_input.c > > > +++ b/net/xfrm/xfrm_input.c > > > @@ -494,6 +497,10 @@ int xfrm_input(struct sk_buff *skb, int nexthdr,= __be32 spi, int encap_type) > > >=20 > > > =09=09family =3D x->props.family; > > >=20 > > > +=09=09/* An encap_type of -3 indicates reconstructed inner packet */ > >=20 > > And I think it's time to document all the encap_types above the > > function (and in particular, how xfrm_inner_mode_input/encap_type=3D-3 > > pair together), and/or define some constants. Also, is -2 used > > anywhere (I only see -1 and -3)? If not, then why -3? >=20 > At the time this was added ISTR that there was some belief that -2 > was used perhaps in an upcoming patch, so I picked -3. I can't find > a -2 use case though so I will switch to -2 instead. >=20 > Re documentation: I think the inline comments where encap_type is > used is sufficient documentation for the 2 negative values. I don't think it is. Inline comments are good to explain the internal behavior, but that's more external behavior. > There's > a lot going on in this function and someone wishing to change (or > understand) something is going to have to walk the code and use > cases regardless of a bit of extra verbiage on the encap_value > beyond what's already there. Fully documenting how xfrm_input works > (in all it's use cases) seems beyond the scope of this patch to me. Sure, and that's really not what I'm asking for here. Something like "encap_type=3D-3 makes xfrm_input jump right back to where it stopped when xfrm_inner_mode_input returned -EINPROGRESS" is useful without having to dive into the mess that is xfrm_input. --=20 Sabrina