From owner-svn-src-head@freebsd.org Wed Aug 30 00:10:44 2017 Return-Path: Delivered-To: svn-src-head@mailman.ysv.freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [IPv6:2001:1900:2254:206a::19:1]) by mailman.ysv.freebsd.org (Postfix) with ESMTP id 1B6C9DE930D; Wed, 30 Aug 2017 00:10:44 +0000 (UTC) (envelope-from brde@optusnet.com.au) Received: from mail106.syd.optusnet.com.au (mail106.syd.optusnet.com.au [211.29.132.42]) by mx1.freebsd.org (Postfix) with ESMTP id BECD47684E; Wed, 30 Aug 2017 00:10:42 +0000 (UTC) (envelope-from brde@optusnet.com.au) Received: from [192.168.0.102] (c110-21-101-228.carlnfd1.nsw.optusnet.com.au [110.21.101.228]) by mail106.syd.optusnet.com.au (Postfix) with ESMTPS id 8FB883C7C79; Wed, 30 Aug 2017 10:10:38 +1000 (AEST) Date: Wed, 30 Aug 2017 10:10:37 +1000 (EST) From: Bruce Evans X-X-Sender: bde@besplex.bde.org To: John Baldwin cc: Maxim Sobolev , Ryan Libby , svn-src-head@freebsd.org, svn-src-all@freebsd.org, src-committers Subject: Re: svn commit: r322969 - in head: sbin/mdconfig sys/dev/md sys/sys In-Reply-To: <5654024.G33lbUi2LU@ralph.baldwin.cx> Message-ID: <20170830093547.K1563@besplex.bde.org> References: <201708281554.v7SFs8fr014268@repo.freebsd.org> <6350259.n2rmZ9RnEY@ralph.baldwin.cx> <5654024.G33lbUi2LU@ralph.baldwin.cx> MIME-Version: 1.0 Content-Type: TEXT/PLAIN; charset=US-ASCII; format=flowed X-Optus-CM-Score: 0 X-Optus-CM-Analysis: v=2.2 cv=bc8baKHB c=1 sm=1 tr=0 a=PalzARQSbocsUSjMRkwAPg==:117 a=PalzARQSbocsUSjMRkwAPg==:17 a=kj9zAlcOel0A:10 a=y0MklxSdXDZzXm4uNcMA:9 a=jJujdnyBXyARHypj:21 a=_TP-T7HD0tPAxYq6:21 a=CjuIK1q_8ugA:10 X-BeenThere: svn-src-head@freebsd.org X-Mailman-Version: 2.1.23 Precedence: list List-Id: SVN commit messages for the src tree for head/-current List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Wed, 30 Aug 2017 00:10:44 -0000 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 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