From c73d13015dd3971f720ab9e9a894fe403e43b579 Mon Sep 17 00:00:00 2001 From: Michael Lotz Date: Sun, 8 Nov 2015 19:51:28 +0100 Subject: [PATCH] kernel: Use anonymous namespaces to avoid type collisions. The anonymous namespace makes type definitions local to the translation unit (like static does for objects). For pretty much any type not shared across multiple files this is what one wants to happen (and might erroneously expect to happen automatically). This commit solves some actual collisions that were present: * The VFS and the rootfs both used an incompatible VnodeHash struct for their BOpenHashTable. * XSI semaphores and message queues both used queued_thread, Ipc and IpcHashTableDefinition. For release builds these did not cause problems as the types were fully inlined. Debug builds would crash at boot however because parts of a BOpenHashTable from the rootfs meant to operate on struct rootfs_vnode would be applied to one from the VFS expecting struct vnode. As such collisions are violations of the one definition rule, the code is at fault and unfortunatley the compiler isn't required to diagnose such problems across translation units (which isn't actually trivial). This can lead to subtle and hard to debug problems and it's therefore best to avoid leaking types into the global namespace whenever possible. --- src/system/kernel/arch/x86/arch_vm.cpp | 5 ++ src/system/kernel/cache/block_cache.cpp | 4 + src/system/kernel/device_manager/devfs.cpp | 12 +-- .../kernel/device_manager/legacy_drivers.cpp | 41 +++++----- src/system/kernel/elf.cpp | 5 ++ src/system/kernel/fs/rootfs.cpp | 14 ++-- src/system/kernel/fs/vfs.cpp | 25 ++++-- src/system/kernel/image.cpp | 4 + src/system/kernel/module.cpp | 78 +++++++++---------- src/system/kernel/port.cpp | 15 ++-- src/system/kernel/posix/realtime_sem.cpp | 4 + src/system/kernel/posix/xsi_message_queue.cpp | 6 ++ src/system/kernel/posix/xsi_semaphore.cpp | 13 ++++ src/system/kernel/vm/VMAddressSpace.cpp | 5 ++ src/system/kernel/vm/vm.cpp | 4 + 15 files changed, 154 insertions(+), 81 deletions(-) diff --git a/src/system/kernel/arch/x86/arch_vm.cpp b/src/system/kernel/arch/x86/arch_vm.cpp index ae063057a5..95b608d836 100644 --- a/src/system/kernel/arch/x86/arch_vm.cpp +++ b/src/system/kernel/arch/x86/arch_vm.cpp @@ -59,6 +59,8 @@ void *gDmaAddress; +namespace { + struct memory_type_range : DoublyLinkedListLinkImpl { uint64 base; uint64 size; @@ -89,6 +91,9 @@ struct update_mtrr_info { typedef DoublyLinkedList MemoryTypeRangeList; +} // namespace + + static mutex sMemoryTypeLock = MUTEX_INITIALIZER("memory type ranges"); static MemoryTypeRangeList sMemoryTypeRanges; static int32 sMemoryTypeRangeCount = 0; diff --git a/src/system/kernel/cache/block_cache.cpp b/src/system/kernel/cache/block_cache.cpp index ce08c5dba9..9482bec2de 100644 --- a/src/system/kernel/cache/block_cache.cpp +++ b/src/system/kernel/cache/block_cache.cpp @@ -50,6 +50,8 @@ static const bigtime_t kTransactionIdleTime = 2000000LL; // a transaction is considered idle after 2 seconds of inactivity +namespace { + struct cache_transaction; struct cached_block; struct block_cache; @@ -322,6 +324,8 @@ public: typedef AutoLocker TransactionLocker; +} // namespace + #if BLOCK_CACHE_BLOCK_TRACING && !defined(BUILDING_USERLAND_FS_SERVER) namespace BlockTracing { diff --git a/src/system/kernel/device_manager/devfs.cpp b/src/system/kernel/device_manager/devfs.cpp index 9f8c742c89..af6c7f7a89 100644 --- a/src/system/kernel/device_manager/devfs.cpp +++ b/src/system/kernel/device_manager/devfs.cpp @@ -50,6 +50,8 @@ #endif +namespace { + struct devfs_partition { struct devfs_vnode* raw_device; partition_info info; @@ -158,11 +160,11 @@ enum { ITERATION_STATE_BEGIN = ITERATION_STATE_DOT, }; -// extern and in a private namespace only to make forward declaration possible -namespace { - extern fs_volume_ops kVolumeOps; - extern fs_vnode_ops kVnodeOps; -} +// extern only to make forward declaration possible +extern fs_volume_ops kVolumeOps; +extern fs_vnode_ops kVnodeOps; + +} // namespace static status_t get_node_for_path(struct devfs* fs, const char* path, diff --git a/src/system/kernel/device_manager/legacy_drivers.cpp b/src/system/kernel/device_manager/legacy_drivers.cpp index 602e1a26c7..cbd792c2a6 100644 --- a/src/system/kernel/device_manager/legacy_drivers.cpp +++ b/src/system/kernel/device_manager/legacy_drivers.cpp @@ -219,27 +219,6 @@ public: }; -} // unnamed namespace - - -static status_t unload_driver(legacy_driver *driver); -static status_t load_driver(legacy_driver *driver); - - -static DriverWatcher sDriverWatcher; -static int32 sDriverEventsPending; -static DriverEventList sDriverEvents; -static mutex sDriverEventsLock = MUTEX_INITIALIZER("driver events"); - // inner lock, protects the sDriverEvents list only -static DirectoryWatcher sDirectoryWatcher; -static DirectoryNodeHash sDirectoryNodeHash; -static recursive_lock sLock; -static bool sWatching; - - -// #pragma mark - driver private - - struct DriverHash { typedef const char* KeyType; typedef legacy_driver ValueType; @@ -268,9 +247,29 @@ struct DriverHash { typedef BOpenHashTable DriverTable; +} // unnamed namespace + + +static status_t unload_driver(legacy_driver *driver); +static status_t load_driver(legacy_driver *driver); + + +static DriverWatcher sDriverWatcher; +static int32 sDriverEventsPending; +static DriverEventList sDriverEvents; +static mutex sDriverEventsLock = MUTEX_INITIALIZER("driver events"); + // inner lock, protects the sDriverEvents list only +static DirectoryWatcher sDirectoryWatcher; +static DirectoryNodeHash sDirectoryNodeHash; +static recursive_lock sLock; +static bool sWatching; + static DriverTable* sDriverHash; +// #pragma mark - driver private + + /*! Collects all published devices of a driver, compares them to what the driver would publish now, and then publishes/unpublishes the devices as needed. diff --git a/src/system/kernel/elf.cpp b/src/system/kernel/elf.cpp index 2ed0240787..544fb5a8df 100644 --- a/src/system/kernel/elf.cpp +++ b/src/system/kernel/elf.cpp @@ -53,6 +53,8 @@ #endif +namespace { + #define IMAGE_HASH_SIZE 16 struct ImageHashDefinition { @@ -77,6 +79,9 @@ struct ImageHashDefinition { typedef BOpenHashTable ImageHash; +} // namespace + + static ImageHash *sImagesHash; static struct elf_image_info *sKernelImage = NULL; diff --git a/src/system/kernel/fs/rootfs.cpp b/src/system/kernel/fs/rootfs.cpp index 5bfb782fa7..2dbd5b1112 100644 --- a/src/system/kernel/fs/rootfs.cpp +++ b/src/system/kernel/fs/rootfs.cpp @@ -46,6 +46,8 @@ #endif +namespace { + struct rootfs_stream { mode_t type; struct stream_dir { @@ -124,11 +126,13 @@ enum { ITERATION_STATE_BEGIN = ITERATION_STATE_DOT, }; -// extern and in a private namespace only to make forward declaration possible -namespace { - extern fs_volume_ops sVolumeOps; - extern fs_vnode_ops sVnodeOps; -} + +// extern only to make forward declaration possible +extern fs_volume_ops sVolumeOps; +extern fs_vnode_ops sVnodeOps; + +} // namespace + #define ROOTFS_HASH_SIZE 16 diff --git a/src/system/kernel/fs/vfs.cpp b/src/system/kernel/fs/vfs.cpp index 963eb371cc..feb3044016 100644 --- a/src/system/kernel/fs/vfs.cpp +++ b/src/system/kernel/fs/vfs.cpp @@ -111,11 +111,6 @@ const static size_t kMaxPathLength = 65536; // on PATH_MAX -struct vnode_hash_key { - dev_t device; - ino_t vnode; -}; - typedef DoublyLinkedList VnodeList; /*! \brief Structure to manage a mounted file system @@ -171,6 +166,9 @@ struct fs_mount { bool owns_file_device; }; + +namespace { + struct advisory_lock : public DoublyLinkedListLinkImpl { list_link link; team_id team; @@ -182,6 +180,9 @@ struct advisory_lock : public DoublyLinkedListLinkImpl { typedef DoublyLinkedList LockList; +} // namespace + + struct advisory_locking { sem_id lock; sem_id wait_sem; @@ -251,6 +252,13 @@ static rw_lock sVnodeLock = RW_LOCK_INITIALIZER("vfs_vnode_lock"); static mutex sIOContextRootLock = MUTEX_INITIALIZER("io_context::root lock"); +namespace { + +struct vnode_hash_key { + dev_t device; + ino_t vnode; +}; + struct VnodeHash { typedef vnode_hash_key KeyType; typedef struct vnode ValueType; @@ -311,6 +319,8 @@ struct MountHash { typedef BOpenHashTable MountTable; +} // namespace + #define VNODE_HASH_TABLE_SIZE 1024 static VnodeTable* sVnodeTable; @@ -520,7 +530,8 @@ static struct fd_ops sQueryOps = { }; -// VNodePutter +namespace { + class VNodePutter { public: VNodePutter(struct vnode* vnode = NULL) : fVNode(vnode) {} @@ -597,6 +608,8 @@ private: bool fKernel; }; +} // namespace + #if VFS_PAGES_IO_TRACING diff --git a/src/system/kernel/image.cpp b/src/system/kernel/image.cpp index bb89b208b3..58526c0090 100644 --- a/src/system/kernel/image.cpp +++ b/src/system/kernel/image.cpp @@ -33,6 +33,8 @@ #define ADD_DEBUGGER_COMMANDS +namespace { + struct ImageTableDefinition { typedef image_id KeyType; typedef struct image ValueType; @@ -68,6 +70,8 @@ public: } }; +} // namespace + static image_id sNextImageID = 1; static mutex sImageMutex = MUTEX_INITIALIZER("image"); diff --git a/src/system/kernel/module.cpp b/src/system/kernel/module.cpp index af601e2d56..829c2513be 100644 --- a/src/system/kernel/module.cpp +++ b/src/system/kernel/module.cpp @@ -275,45 +275,6 @@ private: NotificationList fNotifications; }; -} // namespace Module - -using namespace Module; - -/* These are the standard base paths where we start to look for modules - * to load. Order is important, the last entry here will be searched - * first. - */ -static const directory_which kModulePaths[] = { - B_BEOS_ADDONS_DIRECTORY, - B_SYSTEM_NONPACKAGED_ADDONS_DIRECTORY, - B_USER_ADDONS_DIRECTORY, - B_USER_NONPACKAGED_ADDONS_DIRECTORY, -}; - -static const uint32 kNumModulePaths = sizeof(kModulePaths) - / sizeof(kModulePaths[0]); -static const uint32 kFirstNonSystemModulePath = 1; - - -static ModuleNotificationService sModuleNotificationService; -static bool sDisableUserAddOns = false; - -/* Locking scheme: There is a global lock only; having several locks - makes trouble if dependent modules get loaded concurrently -> - they have to wait for each other, i.e. we need one lock per module; - also we must detect circular references during init and not dead-lock. - - Reference counting: get_module() increments the ref count of a module, - put_module() decrements it. When a B_KEEP_LOADED module is initialized - the ref count is incremented once more, so it never gets - uninitialized/unloaded. A referenced module, unless it's built-in, has a - non-null module_image and owns a reference to the image. When the last - module reference is put, the image's reference is released and module_image - zeroed (as long as the boot volume has not been mounted, it is not zeroed). - An unreferenced module image is unloaded (when the boot volume is mounted). -*/ -static recursive_lock sModulesLock; - struct ModuleHash { typedef const char* KeyType; @@ -364,6 +325,45 @@ struct ImageHash { typedef BOpenHashTable ImageTable; +} // namespace Module + +using namespace Module; + +/* These are the standard base paths where we start to look for modules + * to load. Order is important, the last entry here will be searched + * first. + */ +static const directory_which kModulePaths[] = { + B_BEOS_ADDONS_DIRECTORY, + B_SYSTEM_NONPACKAGED_ADDONS_DIRECTORY, + B_USER_ADDONS_DIRECTORY, + B_USER_NONPACKAGED_ADDONS_DIRECTORY, +}; + +static const uint32 kNumModulePaths = sizeof(kModulePaths) + / sizeof(kModulePaths[0]); +static const uint32 kFirstNonSystemModulePath = 1; + + +static ModuleNotificationService sModuleNotificationService; +static bool sDisableUserAddOns = false; + +/* Locking scheme: There is a global lock only; having several locks + makes trouble if dependent modules get loaded concurrently -> + they have to wait for each other, i.e. we need one lock per module; + also we must detect circular references during init and not dead-lock. + + Reference counting: get_module() increments the ref count of a module, + put_module() decrements it. When a B_KEEP_LOADED module is initialized + the ref count is incremented once more, so it never gets + uninitialized/unloaded. A referenced module, unless it's built-in, has a + non-null module_image and owns a reference to the image. When the last + module reference is put, the image's reference is released and module_image + zeroed (as long as the boot volume has not been mounted, it is not zeroed). + An unreferenced module image is unloaded (when the boot volume is mounted). +*/ +static recursive_lock sModulesLock; + /* We store the loaded modules by directory path, and all known modules * by module name in a hash table for quick access diff --git a/src/system/kernel/port.cpp b/src/system/kernel/port.cpp index 278fc924f7..e9dff1780b 100644 --- a/src/system/kernel/port.cpp +++ b/src/system/kernel/port.cpp @@ -86,11 +86,7 @@ // has a reference to a deleted port. -struct port_message; - - -static void put_port_message(port_message* message); - +namespace { struct port_message : DoublyLinkedListLinkImpl { int32 code; @@ -103,6 +99,13 @@ struct port_message : DoublyLinkedListLinkImpl { typedef DoublyLinkedList MessageList; +} // namespace + + +static void put_port_message(port_message* message); + + +namespace { struct Port : public KernelReferenceable { enum State { @@ -233,6 +236,8 @@ public: void Notify(uint32 opcode, port_id team); }; +} // namespace + // #pragma mark - tracing diff --git a/src/system/kernel/posix/realtime_sem.cpp b/src/system/kernel/posix/realtime_sem.cpp index c6442ab020..4bd66525b6 100644 --- a/src/system/kernel/posix/realtime_sem.cpp +++ b/src/system/kernel/posix/realtime_sem.cpp @@ -24,6 +24,8 @@ #include +namespace { + class SemInfo { public: SemInfo() @@ -364,6 +366,8 @@ struct TeamSemHashDefinition { } }; +} // namespace + struct realtime_sem_context { realtime_sem_context() diff --git a/src/system/kernel/posix/xsi_message_queue.cpp b/src/system/kernel/posix/xsi_message_queue.cpp index 00046cb906..b2807ab156 100644 --- a/src/system/kernel/posix/xsi_message_queue.cpp +++ b/src/system/kernel/posix/xsi_message_queue.cpp @@ -33,6 +33,9 @@ # define TRACE_ERROR(x) dprintf x #endif + +namespace { + // Queue for holding blocked threads struct queued_thread : DoublyLinkedListLinkImpl { queued_thread(Thread *_thread, int32 _message_length) @@ -374,6 +377,9 @@ struct IpcHashTableDefinition { } }; +} // namespace + + // Arbitrary limits #define MAX_XSI_MESSAGE 4096 #define MAX_XSI_MESSAGE_QUEUE 1024 diff --git a/src/system/kernel/posix/xsi_semaphore.cpp b/src/system/kernel/posix/xsi_semaphore.cpp index 1cf30cd47f..918d58a8f5 100644 --- a/src/system/kernel/posix/xsi_semaphore.cpp +++ b/src/system/kernel/posix/xsi_semaphore.cpp @@ -34,6 +34,9 @@ # define TRACE_ERROR(x) dprintf x #endif + +namespace { + // Queue for holding blocked threads struct queued_thread : DoublyLinkedListLinkImpl { queued_thread(Thread *thread, int32 count) @@ -72,6 +75,10 @@ typedef DoublyLinkedList UndoList; typedef DoublyLinkedList > TeamList; +} // namespace + + +// Forward declared in global namespace. struct xsi_sem_context { xsi_sem_context() { @@ -87,6 +94,9 @@ struct xsi_sem_context { mutex lock; }; + +namespace { + // Xsi semaphore definition class XsiSemaphore { public: @@ -621,6 +631,9 @@ struct IpcHashTableDefinition { } }; +} // namespace + + // Arbitrary limit #define MAX_XSI_SEMAPHORE 4096 #define MAX_XSI_SEMAPHORE_SET 2048 diff --git a/src/system/kernel/vm/VMAddressSpace.cpp b/src/system/kernel/vm/VMAddressSpace.cpp index cb61239f4d..7c58a05e56 100644 --- a/src/system/kernel/vm/VMAddressSpace.cpp +++ b/src/system/kernel/vm/VMAddressSpace.cpp @@ -42,6 +42,8 @@ // #pragma mark - AddressSpaceHashDefinition +namespace { + struct AddressSpaceHashDefinition { typedef team_id KeyType; typedef VMAddressSpace ValueType; @@ -69,6 +71,9 @@ struct AddressSpaceHashDefinition { typedef BOpenHashTable AddressSpaceTable; +} // namespace + + static AddressSpaceTable sAddressSpaceTable; static rw_lock sAddressSpaceTableLock; diff --git a/src/system/kernel/vm/vm.cpp b/src/system/kernel/vm/vm.cpp index 8bb5167b0c..553f937ced 100644 --- a/src/system/kernel/vm/vm.cpp +++ b/src/system/kernel/vm/vm.cpp @@ -73,6 +73,8 @@ #endif +namespace { + class AreaCacheLocking { public: inline bool Lock(VMCache* lockable) @@ -229,6 +231,8 @@ private: VMCache* fBottomCache; }; +} // namespace + // The memory reserve an allocation of the certain priority must not touch. static const size_t kMemoryReserveForPriority[] = {