[PATCH libnetfilter_log] src: doc: revise doxygen for module "Netlink message helper functions"

[PATCH libnetfilter_log] src: doc: revise doxygen for module "Netlink message helper functions"

[Duncan Roe]

Adjust style to work better in a man page.
Document actual return values.

Signed-off-by: Duncan Roe <duncan_roe@optusnet.com.au>
---
 src/nlmsg.c | 53 +++++++++++++++++++++++++----------------------------
 1 file changed, 25 insertions(+), 28 deletions(-)

diff --git a/src/nlmsg.c b/src/nlmsg.c
index 3ebb364..399b19a 100644
--- a/src/nlmsg.c
+++ b/src/nlmsg.c
@@ -18,15 +18,15 @@
  */
 
 /**
- * nflog_nlmsg_put_header - reserve and prepare room for nflog Netlink header
- * \param buf memory already allocated to store the Netlink header
- * \param type message type one of the enum nfulnl_msg_types
- * \param family protocol family to be an object of
- * \param qnum queue number to be an object of
+ * nflog_nlmsg_put_header - convert memory buffer into an nflog Netlink header
+ * \param buf pointer to memory buffer
+ * \param type either NFULNL_MSG_PACKET or NFULNL_MSG_CONFIG
+ * \param family protocol family
+ * \param qnum queue number
  *
- * This function creates Netlink header in the memory buffer passed
- * as parameter that will send to nfnetlink log. This function
- * returns a pointer to the Netlink header structure.
+ * Creates a Netlink header in _buf_ followed by
+ * a log\-subsystem\-specific extra header.
+ * \return pointer to created Netlink header structure
  */
 struct nlmsghdr *
 nflog_nlmsg_put_header(char *buf, uint8_t type, uint8_t family, uint16_t qnum)
@@ -47,12 +47,11 @@ nflog_nlmsg_put_header(char *buf, uint8_t type, uint8_t family, uint16_t qnum)
 
 /**
  * nflog_attr_put_cfg_mode - add a mode attribute to nflog netlink message
- * \param nlh pointer to the netlink message
+ * \param nlh pointer to netlink message
  * \param mode copy mode defined in linux/netfilter/nfnetlink_log.h
  * \param range copy range
  *
- * this function returns -1 and errno is explicitly set on error.
- * On success, this function returns 1.
+ * \return 0
  */
 int nflog_attr_put_cfg_mode(struct nlmsghdr *nlh, uint8_t mode, uint32_t range)
 {
@@ -68,12 +67,11 @@ int nflog_attr_put_cfg_mode(struct nlmsghdr *nlh, uint8_t mode, uint32_t range)
 }
 
 /**
- * nflog_attr_put_cfg_cmd - add a cmd attribute to nflog netlink message
- * \param nlh pointer to the netlink message
- * \param cmd command one of the enum nfulnl_msg_config_cmds
+ * nflog_attr_put_cfg_cmd - add a command attribute to nflog netlink message
+ * \param nlh pointer to netlink message
+ * \param cmd one of the enum nfulnl_msg_config_cmds
  *
- * this function returns -1 and errno is explicitly set on error.
- *  On success, this function returns 1.
+ * \return 0
  */
 int nflog_attr_put_cfg_cmd(struct nlmsghdr *nlh, uint8_t cmd)
 {
@@ -148,11 +146,10 @@ static int nflog_parse_attr_cb(const struct nlattr *attr, void *data)
 
 /**
  * nflog_nlmsg_parse - set nlattrs from netlink message
- * \param nlh netlink message that you want to read.
- * \param attr pointer to the array of nlattr which size is NFULA_MAX + 1
+ * \param nlh pointer to netlink message
+ * \param attr pointer to an array of nlattr of size NFULA_MAX + 1
  *
- * This function returns MNL_CB_ERROR if any error occurs, or MNL_CB_OK on
- * success.
+ * \return 0
  */
 int nflog_nlmsg_parse(const struct nlmsghdr *nlh, struct nlattr **attr)
 {
@@ -164,12 +161,12 @@ int nflog_nlmsg_parse(const struct nlmsghdr *nlh, struct nlattr **attr)
  * nflog_nlmsg_snprintf - print a nflog nlattrs to a buffer
  * \param buf buffer used to build the printable nflog
  * \param bufsiz size of the buffer
- * \param nlh netlink message (to get queue num in the futuer)
- * \param attr pointer to a nflog attrs
+ * \param nlh pointer to netlink message (to get queue num in the future)
+ * \param attr pointer to an array of nlattr of size NFULA_MAX + 1
  * \param type print message type in enum nflog_output_type
  * \param flags The flag that tell what to print into the buffer
  *
- * This function supports the following type - flags:
+ * This function supports the following types / flags:
  *
  *   type: NFLOG_OUTPUT_XML
  *	- NFLOG_XML_PREFIX: include the string prefix
@@ -181,12 +178,12 @@ int nflog_nlmsg_parse(const struct nlmsghdr *nlh, struct nlattr **attr)
  *	- NFLOG_XML_TIME: include the timestamp
  *	- NFLOG_XML_ALL: include all the logging information (all flags set)
  *
- * You can combine this flags with an binary OR.
+ * You can combine these flags with a bitwise OR.
  *
- * this function returns -1 and errno is explicitly set in case of
- * failure, otherwise the length of the string that would have been
- * printed into the buffer (in case that there is enough room in
- * it). See snprintf() return value for more information.
+ * \return -1 on failure else same as snprintf
+ * \par Errors
+ * __EOPNOTSUPP__ _type_ is unsupported (i.e. not __NFLOG_OUTPUT_XML__)
+ * \sa __snprintf__(3)
  */
 int nflog_nlmsg_snprintf(char *buf, size_t bufsiz, const struct nlmsghdr *nlh,
 			 struct nlattr **attr, enum nflog_output_type type,
-- 
2.17.5

Re: [PATCH libnetfilter_log] src: doc: revise doxygen for module "Netlink message helper functions"

[Pablo Neira Ayuso]

On Sun, Sep 05, 2021 at 12:33:20PM +1000, Duncan Roe wrote:
> Adjust style to work better in a man page.
> Document actual return values.

All good, except one chunk I'm ambivalent.

> Signed-off-by: Duncan Roe <duncan_roe@optusnet.com.au>
> ---
>  src/nlmsg.c | 53 +++++++++++++++++++++++++----------------------------
>  1 file changed, 25 insertions(+), 28 deletions(-)
> 
> diff --git a/src/nlmsg.c b/src/nlmsg.c
> index 3ebb364..399b19a 100644
> --- a/src/nlmsg.c
> +++ b/src/nlmsg.c
> @@ -18,15 +18,15 @@
>   */
>  
>  /**
> - * nflog_nlmsg_put_header - reserve and prepare room for nflog Netlink header
> - * \param buf memory already allocated to store the Netlink header
> - * \param type message type one of the enum nfulnl_msg_types
> - * \param family protocol family to be an object of
> - * \param qnum queue number to be an object of
> + * nflog_nlmsg_put_header - convert memory buffer into an nflog Netlink header

this is not just converting as in a cast, I understand "reserve and
prepare room" might not be so clear to understand.

> + * \param buf pointer to memory buffer
> + * \param type either NFULNL_MSG_PACKET or NFULNL_MSG_CONFIG

I'd keep above a reference to 'enum nfulnl_msg_types'.

> + * \param family protocol family
> + * \param qnum queue number

I remember you posted a patch to rename qh to gh, from queue handler
to group handler. You could rename this to 'group number'.

>   *
> - * This function creates Netlink header in the memory buffer passed
> - * as parameter that will send to nfnetlink log. This function
> - * returns a pointer to the Netlink header structure.
> + * Creates a Netlink header in _buf_ followed by
> + * a log\-subsystem\-specific extra header.

This function is adding the netlink + nfnetlink headers to the buffer
as well as setting up the header fields accordingly.

> + * \return pointer to created Netlink header structure
>   */
>  struct nlmsghdr *
>  nflog_nlmsg_put_header(char *buf, uint8_t type, uint8_t family, uint16_t qnum)

Re: [PATCH libnetfilter_log] src: doc: revise doxygen for module "Netlink message helper functions"

[Duncan Roe]

On Tue, Sep 07, 2021 at 01:04:24AM +0200, Pablo Neira Ayuso wrote:
> On Sun, Sep 05, 2021 at 12:33:20PM +1000, Duncan Roe wrote:
> > Adjust style to work better in a man page.
> > Document actual return values.
>
> All good, except one chunk I'm ambivalent.
>
> > Signed-off-by: Duncan Roe <duncan_roe@optusnet.com.au>
> > ---
> >  src/nlmsg.c | 53 +++++++++++++++++++++++++----------------------------
> >  1 file changed, 25 insertions(+), 28 deletions(-)
> >
> > diff --git a/src/nlmsg.c b/src/nlmsg.c
> > index 3ebb364..399b19a 100644
> > --- a/src/nlmsg.c
> > +++ b/src/nlmsg.c
> > @@ -18,15 +18,15 @@
> >   */
> >
> >  /**
> > - * nflog_nlmsg_put_header - reserve and prepare room for nflog Netlink header
> > - * \param buf memory already allocated to store the Netlink header
> > - * \param type message type one of the enum nfulnl_msg_types
> > - * \param family protocol family to be an object of
> > - * \param qnum queue number to be an object of
> > + * nflog_nlmsg_put_header - convert memory buffer into an nflog Netlink header
>
> this is not just converting as in a cast, I understand "reserve and
> prepare room" might not be so clear to understand.

v2 substitutes populate ... with for convert ... into.
>
> > + * \param buf pointer to memory buffer
> > + * \param type either NFULNL_MSG_PACKET or NFULNL_MSG_CONFIG
>
> I'd keep above a reference to 'enum nfulnl_msg_types'.

v2 appends enum to line. I thought with only 2 enum members one might as well
say what they are so man page user can highlight and paste into code. enum
definition has comments in case member names are not self-explanatory, but man
page user would likely only ever go there once.
>
> > + * \param family protocol family
> > + * \param qnum queue number
>
> I remember you posted a patch to rename qh to gh, from queue handler
> to group handler. You could rename this to 'group number'.

Done in v2. (2 other files involved - hope that's OK with you).
>
> >   *
> > - * This function creates Netlink header in the memory buffer passed
> > - * as parameter that will send to nfnetlink log. This function
> > - * returns a pointer to the Netlink header structure.
> > + * Creates a Netlink header in _buf_ followed by
> > + * a log\-subsystem\-specific extra header.
>
> This function is adding the netlink + nfnetlink headers to the buffer
> as well as setting up the header fields accordingly.

v2 re-words
>
> > + * \return pointer to created Netlink header structure
> >   */
> >  struct nlmsghdr *
> >  nflog_nlmsg_put_header(char *buf, uint8_t type, uint8_t family, uint16_t qnum)

Cheers ... Duncan.