Skip site navigation (1)Skip section navigation (2)
Date:      Thu, 25 Jun 2009 00:21:45 +0200
From:      Ulf Lilleengen <ulf.lilleengen@gmail.com>
To:        Bruce Evans <brde@optusnet.com.au>
Cc:        svn-src-projects@freebsd.org, src-committers@freebsd.org
Subject:   Re: svn commit: r194847 - projects/libprocstat/sys/sys
Message-ID:  <200906250021.46249.lulf@freebsd.org>
In-Reply-To: <20090625052324.I33482@delplex.bde.org>
References:  <200906241544.n5OFi43Q019124@svn.freebsd.org> <200906241204.51117.jhb@freebsd.org> <20090625052324.I33482@delplex.bde.org>

next in thread | previous in thread | raw e-mail | index | archive | help
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.

>
> >>  	/* 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.
>

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?

-- 
Ulf Lilleengen



Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?200906250021.46249.lulf>