From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from smtp.chopps.org (smtp.chopps.org [54.88.81.56]) by smtp.subspace.kernel.org (Postfix) with ESMTP id 30EF41EB3E for ; Wed, 31 Jul 2024 18:40:32 +0000 (UTC) Authentication-Results: smtp.subspace.kernel.org; arc=none smtp.client-ip=54.88.81.56 ARC-Seal:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1722451234; cv=none; b=MceaP/hbE6LHar0P6wDG6MuwFFrd0MKWJmCsl0sfLsh1pcMBQJ1Br9NNwfYGGp1Ndd26UvxWalZd0fF9i/tdv0wJfA2fizdceYpKKssoyEkfnIGuQDJB0QEsuwuWfeRjmScAW3Uzty9CZ3cT/x6XLnnb2wP3ffR8DlV0vwIWxWc= ARC-Message-Signature:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1722451234; c=relaxed/simple; bh=0j8D4cHWRI/Bl/VUymN1MRLJKU1rhggHOYBbEa4xNGM=; h=References:From:To:Cc:Subject:Date:In-reply-to:Message-ID: MIME-Version:Content-Type; b=mMqMeVe7FlzFtE2ZeymwYMiDWth0d/yBpey+s+hcmoiMTP4kMve0QoQJ5ej8XRyv5LZBQfHx1EhynQ+zpzSC0B+fUVXk84v1LmrvRbFExekwDD44cqZU71ST3M2hSGt+MJ6pVbIgdN77kVAF1vPsDQ1UVKpS6dHCIGZBgNcJPzo= ARC-Authentication-Results:i=1; smtp.subspace.kernel.org; dmarc=none (p=none dis=none) header.from=chopps.org; spf=fail smtp.mailfrom=chopps.org; arc=none smtp.client-ip=54.88.81.56 Authentication-Results: smtp.subspace.kernel.org; dmarc=none (p=none dis=none) header.from=chopps.org Authentication-Results: smtp.subspace.kernel.org; spf=fail smtp.mailfrom=chopps.org Received: from ja.int.chopps.org.chopps.org (syn-172-222-091-149.res.spectrum.com [172.222.91.149]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange ECDHE (P-256) server-signature RSA-PSS (2048 bits) server-digest SHA256) (Client did not present a certificate) by smtp.chopps.org (Postfix) with ESMTPSA id BFDC77D052; Wed, 31 Jul 2024 18:40:31 +0000 (UTC) References: <20240714202246.1573817-1-chopps@chopps.org> <20240714202246.1573817-7-chopps@chopps.org> User-agent: mu4e 1.8.14; emacs 28.3 From: Christian Hopps To: Sabrina Dubroca Cc: Christian Hopps , 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 Date: Wed, 31 Jul 2024 14:32:41 -0400 In-reply-to: Message-ID: Precedence: bulk X-Mailing-List: netdev@vger.kernel.org List-Id: List-Subscribe: List-Unsubscribe: MIME-Version: 1.0 Content-Type: text/plain; format=flowed Sabrina Dubroca writes: > 2024-07-30, 17:29:06 -0400, Christian Hopps wrote: >> >> Sabrina Dubroca writes: >> >> > 2024-07-14, 16:22:34 -0400, Christian Hopps wrote: >> > > +struct xfrm_mode_cbs { >> > >> > It would be nice to add kdoc for the whole thing. >> >> 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) >> > > >> > > family = x->props.family; >> > > >> > > + /* An encap_type of -3 indicates reconstructed inner packet */ >> > >> > 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=-3 >> > pair together), and/or define some constants. Also, is -2 used >> > anywhere (I only see -1 and -3)? If not, then why -3? >> >> 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. >> >> 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=-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. If I'm not adding your suggested text into an inline comment where am I doing this? Bear in mind that encap_type can also have non-negative values, am I documenting all these cases too? It just seems like going down this path is asking for the entire function to be documented, perhaps I'm missing something though. Are other people going to be OK with a top of function comment that only documents the single (now) `-2` value for encap_type? Thanks, Chris.