Re: [PATCH v2] mctp.7: Add man page for Linux MCTP support

[Jeremy Kerr <jk@codeconstruct.com.au>]

Hi Alex,

Thanks for the review! I've updated in line with most of your comments,
and will send a v3 soon. However, I do have a couple of queries, mainly
for my own understanding:

> > +.SH SYNOPSIS
> > +.nf
> > +.B #include <sys/socket.h>
> > +.B #include <linux/mctp.h>
> 
> I prefer alphabetic sorting of includes.

OK, does that take priority over the convention to list the header(s)
specific to this man page last?

In that case, we end up with:

    #include <linux/mctp.h>
    #include <sys/socket.h> /* Definition of socket() & SOCK_DGRAM */

> > +.PP
> > +The API for MCTP messaging uses a standard sockets interface, using the
> > +.BR sendto (2)
> > +and
> > +.BR recvfrom (2)
> > +classes of system calls to transfer messages.
> > +Messages may be fragmented into packets before transmission, and reassembled at
> > +the remote endpoint.
> 
> Break at the comma.

Just this comma? or all of them? There's a couple of sentences right
before this one that would seem to have a similar style - if it's the
former, for my own learning here: what makes this one different?

[and you mean a line-break, right? or a break-point escape sequence?]

> > +Packets between a local and remote endpoint are identified by the source
> 
> Break after "by" (or perhaps just before it).

Same as the above, why is this?

> > +struct sockaddr_mctp {
> > +    unsigned short     smctp_family;  /* = AF_MCTP */
> > +    uint16_t           __smctp_pad0;  /* pad, must be zero */
> > +    int                smctp_network; /* local network identifier */
> > +    struct mctp_addr   smctp_addr;    /* EID */
> > +    uint8_t            smctp_type;    /* message type byte */
> > +    uint8_t            smctp_tag;     /* tag value, including TO flag */
> > +    uint8_t            __smctp_pad1;  /* pad, must be zero */
> 
> Do we want to tie the implementation to this pad?

Yes. The pad will be there anyway, due to the natural alignment of the
struct. Since we want to be explicit about the padding (and require it
to be zeroed), I would strongly suggest keeping it documented.

There is an 'extended' MCTP addressing struct, which encapsulates a
struct sockaddr_mctp. For us to be absolutely clear about the layout of
that structure, the explicit pad here makes that unambiguous.

[unless, for man pages, we don't care about the ABI, only the API?]

> Future implementations of sockaddr_mctp are not going to use that
> byte for anything else?

They might, hence requiring zero at present.

Cheers,


Jeremy