svn commit: r194847 - projects/libprocstat/sys/sys

[Bruce Evans]

On Thu, 25 Jun 2009, Ulf Lilleengen wrote:

> On Wednesday 24 June 2009 21:45:43 Bruce Evans wrote:
>> On Wed, 24 Jun 2009, John Baldwin wrote:
>>> On Wednesday 24 June 2009 11:44:04 am Ulf Lilleengen wrote:
>>> =========================================================================
>>> =====
>>>
>>>> --- projects/libprocstat/sys/sys/user.h	Wed Jun 24 15:41:21
>>>> 2009	(r194846) +++ projects/libprocstat/sys/sys/user.h	Wed Jun 24
>>>> 15:44:04 2009	(r194847) @@ -312,6 +312,7 @@ struct kinfo_ofile {
>>>>
>>>>  struct kinfo_file {
>>>>  	int	kf_structsize;			/* Variable size of record. */
>>>> +	uint16_t	kf_status;		/* Status flags. */
>>>>  	int	kf_type;			/* Descriptor type. */
>>>>  	int	kf_fd;				/* Array index. */
>>>>  	int	kf_ref_count;			/* Reference count. */
> *** SNIP ***
>>
>>>>  	int	_kf_ispare[9];			/* Space for more stuff. */
>>
>> Back to normal indentation.
>
> The indentation was fixed in the next commit, as i wanted to handle that in a
> separate commit.

No, it was broken further in the next commit, but changing all the normal
indents to match the broken ones.

>> Also, almost everything is disordered on size (really alignment).  This
>> seems to break any possibility of the structs having the same size for
>> amd64 and i386 like the size macro still says they have:
>> ...
>> - "mode_t kf_file_mode;" has size 16 bits and "off_t kf_file_size;" has
>>    size 64 bits and maximal alignment requirements, so between these fields
>>    there is 48 bits of padding on amd64 and 16 bits of padding on i386.
>> ...

> How about packing them like this:
>        dev_t           kf_file_fsid;           /* Vnode filesystem id. */
>        dev_t           kf_file_rdev;           /* File device. */
> 64
>
>        uint64_t        kf_file_fileid;         /* Global file id. */
> 64
>        off_t           kf_file_size;           /* File size. */
> 64
>        mode_t          kf_file_mode;           /* File mode. */
>        uint16_t        kf_status;              /* Status flags. */
> 32
> 	 spare[9]
>
> Seems good?

This seems reasonable, but I would put all the new 64-bit fields first, and
I think there is another mode_t that belongs next to the one here.  Since
this gives an odd number of 16-bit fields, there will be 16 bits of padding
before the 32-bit int spares, and this padding should be explicit.

Now I wonder why we are worrying about the packing or are using spares
at all.  New APIs shouldn't repeat the mistakes for struct kinfo_proc.
They should be designed for expansion.  This seems to require mainly
not using any nested structs (especially kernel ones whose size cannot
be controlled), and only adding fields to the end, and userland being
either aware of the possibility for expansion or not care about it
(sysctl will tell if there is more data but the ENOMEM error for this
shouldn't be fatal).  The former rule is already mostly followed, but
spares are incompatible with adding to the end, and require delicate
packing.  Oh, and all padding should be explicit, especially at the
end, and not MD.  This means for example that mode_t's should always
be added 4 at a time to reach the next maximal alignment boundary of
64 bits (128 bits is hopefully not needed).  Packing can be simplified
by using uintmax_t to represent almost everything.  For example, instead
of 4 mode_t's (3 unused padding), you might have 1 mode_t represented
as a uintmax_t.  rwatson mentioned using a kptr_t type to represent
kernel pointers (void * is no good for 32-bit applications on 64-bit
kernels).  uintmax_t would work well for that.

Bruce