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
_______________________________________________
svn-src-head@freebsd.org mailing list
https://lists.freebsd.org/mailman/listinfo/svn-src-head
To unsubscribe, send any mail to "svn-src-head-unsubscr...@freebsd.org"
