svn commit: r322969 - in head: sbin/mdconfig sys/dev/md sys/sys

[Bruce Evans]

On Tue, 29 Aug 2017, John Baldwin wrote:

> On Tuesday, August 29, 2017 12:18:18 PM Maxim Sobolev wrote:
>> John, OK, maybe you are right and the current status quo was just an
>> accident. I am curious what do you and other people think about expressing
>> expected structure size and padding more explicitly instead of trying to
>> accommodate for sometimes intricate play between alignment and type size
>> with something like char[N]? I.e. along the following lines:
>>
>> #if __WORDSIZE < 64
>
> Use #ifdef __LP64__
>
>> #define MD_IOCTL_LEN    436
>> #else
>> #define MD_IOCTL_LEN    448
>> #endif

It is better to not use any ifdefs.  Ones like this guarantee that the
struct depends on __LP64__, so that it will need translation layer to
work with 32-bit binaries.

>> struct md_ioctl {
>>     union {
>>         struct _md_ioctl_payload {
>>             unsigned        version;     /* Structure layout version */
>>             unsigned        unit;        /* unit number */

Even unsigned is unportable in theory.  It is portable in practice since
all systems are Vaxes, so they have 32-bit ints.

>>             enum md_types   type ;       /* type of disk */

The size of an enum is very unportable, enums should never be used in ABIs.

>>             char            *file;       /* pathname of file to mount */

Pointers are very unportable.  uintptr_t is no better, since its size depends
on the arch and is usually the same as sizeof(void *).  ABIs should use some
semi-portable representation like uint64_t.  This might still require a
translation layer to convert it to a pointer.

>>             off_t           mediasize;   /* size of disk in bytes */

off_t is unportable in theory.  It exists so that its size can be changed.
It is portable in practice because systems are superVaxes, so they have
62-bit off_t's.

uintmax_t is only portable among superVaxes too.  This will break sooner
than off_t.  The existence of __uint128_t already breaks uintmax_t if
you use __uintmax_t.  (uintmax_t is supposed to be the largest type,
but it isn't if it is 64 bits and __uint128_t exists.  This can't be
fixed simply by expanding uintmax_t, since Standard libraries are required
to support uintmax_t but have little support for 128-bit integers, and
more seriously the expansion would be a larger pessimization than using
64-bit uintmax_t and would expose the brokenness of ABIs with uintmax_t
in them.)

>>             unsigned        sectorsize;  /* sectorsize */
>>             unsigned        options;     /* options */
>>             u_int64_t       base;        /* base address */

This is a portable ABI, but hard-codes some limits, but 64 bits should
be enough for anyone.

>>             int             fwheads;     /* firmware heads */
>>             int             fwsectors;   /* firmware sectors */
>>             char            *label;      /* label of the device */

A new pointer.

>>         } md;
>>         char raw[MD_IOCTL_LEN]; /* payload + padding for future ideas */
>>     };
>> };
>> CTASSERT(sizeof(struct md_ioctl) == MD_IOCTL_LEN);
>
> This is not the style we use in other structures in FreeBSD.  Simply making
> the existing MDNPAD depend on the #ifdef would be more consistent.  For a
> really good example of how to handle padding, see kinfo_proc which has
> separate "spare" arrays for int, long, void *, and char.

Those arrays are bugs.  They guarantee that the ABI depends on the register
size.  KINFO_PROC_SIZE indeed is very different on amd64 and i386.  But the
i386 ps and other statistics utilities using kinfo work on amd64, so there
must be a translation layer.

Newer structs in <sys/user.h> are better designed and use mostly integer
fields of type uint64_t, uint32_t and int.  The main kinfo struct has
many historical mistakes.  It starts with 2 ints, then has many dubious
(kernel-only?) pointers, then uses pid_t.  pid_t is unportable like off_t.
Later, many more typedefed integer types.  The worst old mistakes are the
rusage struct types.  The worst new mistake is the priority struct type
(old versions of kinfo had 3 (?) integer priorities of more interest to
applications, while the struct has has kernel internals in an inconvenient
form).

Bruce