From owner-svn-src-all@FreeBSD.ORG Sun Oct 5 01:52:18 2014 Return-Path: Delivered-To: svn-src-all@freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [IPv6:2001:1900:2254:206a::19:1]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by hub.freebsd.org (Postfix) with ESMTPS id D38FCB81; Sun, 5 Oct 2014 01:52:17 +0000 (UTC) Received: from mail108.syd.optusnet.com.au (mail108.syd.optusnet.com.au [211.29.132.59]) by mx1.freebsd.org (Postfix) with ESMTP id 952B7E3C; Sun, 5 Oct 2014 01:52:17 +0000 (UTC) Received: from c122-106-147-133.carlnfd1.nsw.optusnet.com.au (c122-106-147-133.carlnfd1.nsw.optusnet.com.au [122.106.147.133]) by mail108.syd.optusnet.com.au (Postfix) with ESMTPS id AF07F1A69C5; Sun, 5 Oct 2014 12:52:06 +1100 (EST) Date: Sun, 5 Oct 2014 12:51:56 +1100 (EST) From: Bruce Evans X-X-Sender: bde@besplex.bde.org To: "Bjoern A. Zeeb" Subject: Re: svn commit: r272505 - in head/sys: kern sys In-Reply-To: <42180557-0119-4597-9492-662E1671A840@FreeBSD.org> Message-ID: <20141005123549.A1162@besplex.bde.org> References: <201410040808.s9488uAI099166@svn.freebsd.org> <42180557-0119-4597-9492-662E1671A840@FreeBSD.org> MIME-Version: 1.0 Content-Type: TEXT/PLAIN; charset=US-ASCII; format=flowed X-Optus-CM-Score: 0 X-Optus-CM-Analysis: v=2.1 cv=BdjhjNd2 c=1 sm=1 tr=0 a=7NqvjVvQucbO2RlWB8PEog==:117 a=PO7r1zJSAAAA:8 a=kj9zAlcOel0A:10 a=JzwRw_2MAAAA:8 a=6I5d2MoRAAAA:8 a=vBSQY5nq6lOT3kLbb88A:9 a=CjuIK1q_8ugA:10 a=SV7veod9ZcQA:10 Cc: svn-src-head@freebsd.org, svn-src-all@freebsd.org, src-committers@freebsd.org, Konstantin Belousov , Mateusz Guzik X-BeenThere: svn-src-all@freebsd.org X-Mailman-Version: 2.1.18-1 Precedence: list List-Id: "SVN commit messages for the entire src tree \(except for " user" and " projects" \)" List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Sun, 05 Oct 2014 01:52:18 -0000 On Sat, 4 Oct 2014, Bjoern A. Zeeb wrote: > On 04 Oct 2014, at 08:08 , Mateusz Guzik wrote: >> ... >> Log: >> Plug capability races. > ... > > This file is included from user space. There is no opt_capsicum.h there. > Including an opt_* in the header file seems wrong in a lot of ways usually. > > I tried to add a bandaid for the moment with r272523 which (to be honest) makes it worse. > > This needs a better fix. > > I also wonder why the (conditional) fde_seq ended up at the beginning of the structure rather than the end? It adds to the already-massive namespace pollution in other ways. >> Modified: head/sys/sys/filedesc.h >> ============================================================================== >> --- head/sys/sys/filedesc.h Sat Oct 4 08:05:39 2014 (r272504) >> +++ head/sys/sys/filedesc.h Sat Oct 4 08:08:56 2014 (r272505) >> @@ -33,11 +33,14 @@ >> #ifndef _SYS_FILEDESC_H_ >> #define _SYS_FILEDESC_H_ >> >> +#include "opt_capsicum.h" >> + Fatal namspace pollution. >> #include >> #include >> #include >> #include >> #include Old massive namespace pollution. >> +#include New namespace pollution. >> #include >> >> #include Old massive namespace pollution, continued. The pollution is nested. at least has a _KERNEL ifdef around most of its pollution. >> @@ -50,6 +53,9 @@ struct filecaps { >> }; >> >> struct filedescent { >> +#ifdef CAPABILITIES >> + seq_t fde_seq; /* if you need fde_file and fde_caps in sync */ >> +#endif This ifdef makes the struct not really a struct. The layout depends on visible options, and the visible options may depend on pollution. The worst sort of pollution bug would occur if something else has a similar ifdef but doesn't include the options header. Then when different pollution there results in including this header, the include of the options header here gives different visible options and thus a different struct layout there. >> struct file *fde_file; /* file structure for open file */ >> struct filecaps fde_caps; /* per-descriptor rights */ >> uint8_t fde_flags; /* per-process open file flags */ >> @@ -58,6 +64,13 @@ struct filedescent { >> #define fde_fcntls fde_caps.fc_fcntls >> #define fde_ioctls fde_caps.fc_ioctls >> #define fde_nioctls fde_caps.fc_nioctls >> +#ifdef CAPABILITIES >> +#define fde_change(fde) ((char *)(fde) + sizeof(seq_t)) >> +#define fde_change_size (sizeof(struct filedescent) - sizeof(seq_t)) >> +#else >> +#define fde_change(fde) ((fde)) >> +#define fde_change_size (sizeof(struct filedescent)) >> +#endif >> >> /* >> * This structure is used for the management of descriptors. It may be >> @@ -82,6 +95,9 @@ struct filedesc { >> int fd_holdleaderscount; /* block fdfree() for shared close() */ >> int fd_holdleaderswakeup; /* fdfree() needs wakeup */ >> }; >> +#ifdef CAPABILITIES >> +#define fd_seq(fdp, fd) (&(fdp)->fd_ofiles[(fd)].fde_seq) >> +#endif The Ifdefs of the macros are not even wrong. Macros are not as polluting as inline functions, so always defining them doen't cause many problems. They just give relatively minor namespace pollution and are unusable if the headers needed to use them are not included. The patch has some style bugs, e.g., long lines. Bruce