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

[Bruce Evans]

On Mon, 28 Aug 2017, Ryan Libby wrote:

> On Mon, Aug 28, 2017 at 11:24 AM, Maxim Sobolev <sobomax at freebsd.org> 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 <jhb at freebsd.org> 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