From owner-svn-src-head@freebsd.org Tue Aug 29 04:19:37 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 8968FE1E793; Tue, 29 Aug 2017 04:19:37 +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 1A3B57258C; Tue, 29 Aug 2017 04:19:36 +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 05A6D3D080D; Tue, 29 Aug 2017 14:19:28 +1000 (AEST) Date: Tue, 29 Aug 2017 14:19:28 +1000 (EST) From: Bruce Evans X-X-Sender: bde@besplex.bde.org To: Ryan Libby cc: Maxim Sobolev , John Baldwin , 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: Message-ID: <20170829140445.S2496@besplex.bde.org> References: <201708281554.v7SFs8fr014268@repo.freebsd.org> <7384187.efIiCynxO3@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=LI0WeNe9 c=1 sm=1 tr=0 a=PalzARQSbocsUSjMRkwAPg==:117 a=PalzARQSbocsUSjMRkwAPg==:17 a=kj9zAlcOel0A:10 a=6I5d2MoRAAAA:8 a=YptDlVL9CQ67nPSwNbMA:9 a=lWk6GuVCh-BrDwvE:21 a=QeoftfXBIHTYb7TF:21 a=CjuIK1q_8ugA:10 a=IjZwj45LgO3ly-622nXo:22 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: Tue, 29 Aug 2017 04:19:37 -0000 On Mon, 28 Aug 2017, Ryan Libby wrote: > On Mon, Aug 28, 2017 at 11:24 AM, Maxim Sobolev wrote: >> Hi John, >> >> Thanks for your feedback! To address the points that you've raised: >> >> 1. I've tested on both 32 and 64 bit platforms, it seems not to be the >> case. See imp's comment and my reply here >> https://reviews.freebsd.org/D10457#216855 . Did I miss something? Can you >> post piece of C code that produces different sizeof(struct old) vs. >> sizeof(struct new) on some platform? > [...] >> On Mon, Aug 28, 2017 at 9:19 AM, John Baldwin wrote: >> >>> On Monday, August 28, 2017 03:54:08 PM Maxim Sobolev wrote: >>>> Author: sobomax >>>> Date: Mon Aug 28 15:54:07 2017 >>>> New Revision: 322969 >>>> URL: https://svnweb.freebsd.org/changeset/base/322969 >>>> >>>> Log: >>>> Add ability to label md(4) devices. This patch has some style bugs (mainly unsorting of options list in different ways by not always adding to the end of almost-sorted lists). >>>> Modified: head/sys/sys/mdioctl.h >>>> ============================================================ >>> ================== >>>> --- head/sys/sys/mdioctl.h Mon Aug 28 14:49:26 2017 (r322968) >>>> +++ head/sys/sys/mdioctl.h Mon Aug 28 15:54:07 2017 (r322969) >>>> @@ -49,7 +49,7 @@ enum md_types {MD_MALLOC, MD_PRELOAD, MD_VNODE, MD_SWA >>>> * Ioctl definitions for memory disk pseudo-device. >>>> */ >>>> >>>> -#define MDNPAD 97 >>>> +#define MDNPAD 96 >>>> struct md_ioctl { >>>> unsigned md_version; /* Structure layout version */ >>>> unsigned md_unit; /* unit number */ >>>> @@ -61,6 +61,7 @@ struct md_ioctl { >>>> u_int64_t md_base; /* base address */ >>>> int md_fwheads; /* firmware heads */ >>>> int md_fwsectors; /* firmware sectors */ >>>> + char *md_label; /* label of the device */ >>>> int md_pad[MDNPAD]; /* padding for future ideas */ >>>> }; >>> >>> This isn't correct on 64-bit platforms. MDNPAD needs to be 95 on those >>> platforms. > [...] > > Can you report sizeof(md_ioctl) before and after for 32-bit and 64-bit? > I think it may be: > 32-bit before: 440 > 32-bit after: 440 > 64-bit before: 448 > 64-bit after: 448 > > In other words, it looks like it used to produce different sizes on the > different architectures, and still does. It also looks like 32-bit > before and after and 64-bit before included some undeclared padding > after md_pad, so that this would fail: > CTASSERT(sizeof(md_ioctl) == offsetof(struct md_ioctl, md_pad) + > sizeof(((struct md_ioctl *)NULL)->md_pad)); > > In any case a CTASSERT(sizeof(md_ioctl) == XXX) may increase confidence > in the ABI here. Indeed, the odd count for the padding was nonsense on 64-bit arches. It followed a uint64_t which gives 64-bit alignment, then 2 ints which maintain 64-bit alignment. Then the odd count gets rounded up to even on 64-bit arcesh. The padding could be expressed in intmax_t's, but that causes different problems -- odd padding becomes impossible and char/short/int/long/long long padding might be needed to prepare for the intmax_t padding. char padding is most flexible, but harder to count. It's surprising how many i386 utilities now work on amd64. This requires things like struct layouts for ioctl data to be the same User (?) pointers in structs like md_label in the above might be the largest unportability. Bruce