Date: Wed, 30 Aug 2017 10:10:37 +1000 (EST) From: Bruce Evans <brde@optusnet.com.au> To: John Baldwin <jhb@freebsd.org> Cc: Maxim Sobolev <sobomax@sippysoft.com>, Ryan Libby <rlibby@freebsd.org>, svn-src-head@freebsd.org, svn-src-all@freebsd.org, src-committers <src-committers@freebsd.org> Subject: Re: svn commit: r322969 - in head: sbin/mdconfig sys/dev/md sys/sys Message-ID: <20170830093547.K1563@besplex.bde.org> In-Reply-To: <5654024.G33lbUi2LU@ralph.baldwin.cx> References: <201708281554.v7SFs8fr014268@repo.freebsd.org> <6350259.n2rmZ9RnEY@ralph.baldwin.cx> <CAH7qZfvGL-UgZX5VzZXB%2B6zPznA9-GvEg%2B=X2roiTSUUj5jxSA@mail.gmail.com> <5654024.G33lbUi2LU@ralph.baldwin.cx>
next in thread | previous in thread | raw e-mail | index | archive | help
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
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?20170830093547.K1563>