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

["Alejandro Colomar (man-pages)" <alx.manpages@gmail.com>]

Hi Jeremy,

On 11/18/21 06:08, Jeremy Kerr wrote:
> Hi Alex,
> 
>> I didn't even know there was such a convention, but if there is, yes,
>> I explicitly want to override it.
> 
> OK, I'll update to suit.
> 
>> man-pages(7) recommends breaking long lines at clause boundaries
>> (commas, semicolons, and so on), but somethimes clauses (if you don't
>> know the difference between phrases and clauses, which you don't need
>> to, basically clauses are made up of phrases) are too long, and you
>> can have a single clause that uses more than a single line.  In those
>> cases, the most sensible place to break the line is at the next level:
>> phrase boundaries.
> 
> OK, understood, thanks for that.
> 
>> What I mean is, if in the future this structure will have additional
>> trailing fields, documenting this padding is unnecessary, since that
>> may
>> vary.  Code should not rely on this structure having _only_ that
>> padding.  And if code handles any arbitrary extra stuff in this
>> structure, it will implicitly also handle that __smctp_pad1 field, so
>> there's no need to mention it.
>>
>> Example:
>>
>> 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            foo;           /* was __smctp_pad1 */
>>       uint8_t            bar;           /* extra stuff */
>> };
>>
>> Here I got rid of the pad, and even added an extra field.
> 
> Right, but you've also broken the ABI if that previous padding byte
> wasn't explicitly present, and required to be zero.

Ahh, that's why I require in such cases a memset(p, 0, sizeof(*p));
That covers any undocumented padding; present or future.

> In that future ABI
> implementation, the kernel can't distinguish between 'foo' being properly
> initialised, and not just random stack garbage from an old-ABI caller.
> 
> That's why we have the _pad1 field, and why we require it to be zero.
> Since that's enforced by the kernel, I'd rather have it documented,
> rather than users seeing their calls fail for "invisible" reasons, when
> a call's _pad1 happens to contain a non-zero byte due to not being
> initialised.
> 
>> Code should be written to be compatible with this case, right?
> 
> I'm not 100% clear on you mean by compatible there - you want to prevent
> the case where __smctp_pad1 is removed from the header, and that code is
> now referencing an invalid struct member?
> 
> That's somewhat unavoidable, and also applies to _pad0; I'm not sure why
> _pad1 needs to be different.

I want that programmers zero the structure not by p->_pad0 = 0,
but by bzero(3) or memset(3).  That's how it covers any ammount of padding.

Nevertheless, I don't have a strong feeling about this, and if you 
prefer to document the padding, I'm fine with that.

Cheers!
Alex


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