linux-kernel - Re: [PATCH] mctp.7: Add man page for Linux MCTP support

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [day] [month] [year] [list]

Message-ID: <3946e062-8e97-78a2-b1db-2a7d92c4730b@gmail.com>
Date:   Mon, 18 Oct 2021 08:59:07 +0200
From:   "Alejandro Colomar (man-pages)" <alx.manpages@...il.com>
To:     Jeremy Kerr <jk@...econstruct.com.au>
Cc:     Michael Kerrisk <mtk.manpages@...il.com>,
        linux-man@...r.kernel.org, Dwaipayan Ray <dwaipayanray1@...il.com>,
        Joe Perches <joe@...ches.com>,
        Andy Whitcroft <apw@...onical.com>,
        Lukas Bulwahn <lukas.bulwahn@...il.com>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH] mctp.7: Add man page for Linux MCTP support

[CC += checkpatch.pl maintainers (see reason below)]


Hi Jeremy,

On 10/18/21 7:05 AM, Jeremy Kerr wrote:
> Hi Alex,
> 
>> Thanks for the manual page!
> 
> And thanks for the review! In general, I've updated to suit your
> comments, just a couple of queries inline.
> 
>>> +.SH SYNOPSIS
>>> +.nf
>>> +.B #include <sys/socket.h>
>>> +.B #include <linux/mctp.h>
>>> +.PP
>>> +.B mctp_socket = socket(AF_MCTP, SOCK_DGRAM, 0);
>>
>> mctp_socket is a variable name.  See socket.7 for an example.
>> It should be in italics.
> 
> This was based on udp.7; want me to send a patch for that too?

Sure. Thanks!

> 
>>> +Packets between a local and remote endpoint are identified by the
>>> source
>>> +and destination EIDs, plus a three-bit tag value.
>>> +.PP
>>> +Addressing data is passed in socket system calls through
>>> +.B struct sockaddr\_mctp
>>
>> That escape is unnecessary.  Did you see it in another page perhaps?
> 
> I thought I'd seen some odd line-breaks at the underscore, but can't
> replicate that now. Will remove.
> 
>>> +typedef uint8_t        mctp_eid_t;
>>> +
>>> +struct mctp_addr {
>>> +    mctp_eid_t         s_addr;
>>> +};
>>> +
>>> +struct sockaddr_mctp {
>>> +    unsigned short int smctp_family;  /* = AF_MCTP */
>>
>> We only use 'int' in 'unsigned int', as the kernel does (or attempts
>> to do).  checkpatch.pl warns about 'unsigned short int', IIRC.
> 
> No, there are no warnings from checkpatch there; that's just copied from
> the current kernel header.

Huh!  That's weird; 'unsigned long int' does, so I expected the same 
with 'short'.  Maybe a bug in checkpatch?


WARNING:UNNECESSARY_INT: Prefer 'unsigned long' over 'unsigned long int' 
as the int is unnecessary
#42: FILE: /home/user/src/alx/test/unsigned_short_int.c:42:
+	unsigned long int a;

total: 0 errors, 1 warnings, 0 checks, 65 lines checked


> 
> However, I have just sent a separate patch to change that to
> __kernel_sa_family_t. Should I use that here (keeping this an exact
> match of the kernel header), or stick to the more familiar unsigned
> short?


I prefer 'unsigned short' for consistency with 'unsigned long'.

> 
> Cheers,
> 
> 
> Jeremy
> 

Cheers,

Alex


-- 
Alejandro Colomar
Linux man-pages comaintainer; https://www.kernel.org/doc/man-pages/
http://www.alejandro-colomar.es/