Date: Thu, 25 Jun 2009 05:45:43 +1000 (EST) From: Bruce Evans <brde@optusnet.com.au> To: John Baldwin <jhb@FreeBSD.org> Cc: svn-src-projects@FreeBSD.org, Ulf Lilleengen <lulf@FreeBSD.org>, src-committers@FreeBSD.org Subject: Re: svn commit: r194847 - projects/libprocstat/sys/sys Message-ID: <20090625052324.I33482@delplex.bde.org> In-Reply-To: <200906241204.51117.jhb@freebsd.org> References: <200906241544.n5OFi43Q019124@svn.freebsd.org> <200906241204.51117.jhb@freebsd.org>
next in thread | previous in thread | raw e-mail | index | archive | help
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. */
Normal indentation here.
>> @@ -324,11 +325,11 @@ struct kinfo_file {
>> int kf_sock_protocol; /* Socket protocol. */
>> struct sockaddr_storage kf_sa_local; /* Socket address. */
>> struct sockaddr_storage kf_sa_peer; /* Peer address. */
>> - uint32_t kf_fsid; /* Vnode filesystem id. */
>> - uint64_t kf_fileid; /* Global file id. */
>> - uint32_t kf_mode; /* File mode. */
>> - int64_t kf_size; /* File size. */
>> - uint32_t kf_rdev; /* File device. */
>> + dev_t kf_file_fsid; /* Vnode filesystem id. */
Style bug: extra indentation which is no longer needed.
>> + uint64_t kf_file_fileid; /* Global file id. */
Style bug: extra indentation which is needed.
>> + mode_t kf_file_mode; /* File mode. */
>> + off_t kf_file_size; /* File size. */
>> + dev_t kf_file_rdev; /* File device. */
>> + mode_t kf_file_mode; /* File mode. */
Style bugs: extra indentation which is no longer needed (4 cases).
>> int _kf_ispare[9]; /* Space for more stuff. */
Back to normal indentation.
>> /* Truncated before copyout in sysctl */
>> char kf_path[PATH_MAX]; /* Path to file, if any. */
>
> You probably don't want to add kf_status where you did as it disturbs the ABI
> of all the fields after it. New fields should be added in the spare region.
> Given that mode_t is 16-bits I would just stick it next to kf_file_mode.
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:
- padding might be needed on amd64 only after "dev_t kf_file_fsid;",
depending on whether this field ends at a 32-bit or 64-bit boundary.
- "uint64_t kf_file_fileid;" gives maximal alignment on both amd64 and
i386, so the padding for the next few fields is clear:
- "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.
- "off_t kf_file_size;" has size 64 and gives maximal alignment again
- "dev_t kf_file_rdev;" has size 32 bits
- "mode_t kf_file_mode;" has size 16 bits and "int _kf_ispare[9];" has
alignment 32 bits, so between these fields there is 16 bits of padding
on both amd64 and i386. Unlike the previous padding, this is sufficiently
MD, but it is still wasteful. The mode_t's should be packed together.
Bruce
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?20090625052324.I33482>