Changeset 24809

Timestamp:

04/05/08 08:24:11 (8 months ago)

Author:

axeld

Message:

* Added checks to (hopefully) all write/create functions to bail out early

if the volume is read-only. This should help with #2028.

* Removed unused bfs_[de]select() functions.
* Removed most paranoia checks - now that we control both sides of code,

this would just hide eventual problems in the VFS code.

* Cleanup.

Files:

• haiku/trunk/src/add-ons/kernel/file_systems/bfs/kernel_interface.cpp (modified) (72 diffs)

haiku/trunk/src/add-ons/kernel/file_systems/bfs/kernel_interface.cpp

@@ -131 +131 @@
 
 static status_t
-bfs_unmount(void *ns)
-{
-        FUNCTION();
-        Volume* volume = (Volume *)ns;
+bfs_unmount(void *_fs)
+{
+        FUNCTION();
+        Volume* volume = (Volume *)_fs;
 
         status_t status = volume->Unmount();
@@ -143 +143 @@
 
 
-/**     Fill in bfs_info struct for device.
- */
-
-static status_t
-bfs_read_fs_stat(void *_ns, struct fs_info *info)
-{
-        FUNCTION();
-        if (_ns == NULL || info == NULL)
-                return B_BAD_VALUE;
-
-        Volume *volume = (Volume *)_ns;
-
+static status_t
+bfs_read_fs_stat(void *_fs, struct fs_info *info)
+{
+        FUNCTION();
+
+        Volume *volume = (Volume *)_fs;
         RecursiveLocker locker(volume->Lock());
 
         // File system flags.
-        info->flags = B_FS_IS_PERSISTENT | B_FS_HAS_ATTR | B_FS_HAS_MIME | B_FS_HAS_QUERY |
-                        (volume->IsReadOnly() ? B_FS_IS_READONLY : 0);
+        info->flags = B_FS_IS_PERSISTENT | B_FS_HAS_ATTR | B_FS_HAS_MIME
+                | B_FS_HAS_QUERY | (volume->IsReadOnly() ? B_FS_IS_READONLY : 0);
 
         info->io_size = BFS_IO_SIZE;
-                // whatever is appropriate here? Just use the same value as BFS (and iso9660) for now
+                // whatever is appropriate here?
 
         info->block_size = volume->BlockSize();
@@ -179 +173 @@
 
 static status_t
-bfs_write_fs_stat(void *_ns, const struct fs_info *info, uint32 mask)
+bfs_write_fs_stat(void *_fs, const struct fs_info *info, uint32 mask)
 {
         FUNCTION_START(("mask = %ld\n", mask));
 
-        Volume *volume = (Volume *)_ns;
-        disk_super_block &superBlock = volume->SuperBlock();
+        Volume *volume = (Volume *)_fs;
+        if (volume->IsReadOnly())
+                return B_READ_ONLY_DEVICE;
 
         RecursiveLocker locker(volume->Lock());
@@ -191 +186 @@
 
         if (mask & FS_WRITE_FSINFO_NAME) {
-                strncpy(superBlock.name, info->volume_name, sizeof(superBlock.name) - 1);
+                disk_super_block &superBlock = volume->SuperBlock();
+
+                strncpy(superBlock.name, info->volume_name,
+                        sizeof(superBlock.name) - 1);
                 superBlock.name[sizeof(superBlock.name) - 1] = '\0';
 
@@ -201 +199 @@
 
 static status_t
-bfs_sync(void *_ns)
-{
-        FUNCTION();
-        if (_ns == NULL)
-                return B_BAD_VALUE;
-
-        Volume *volume = (Volume *)_ns;
-
+bfs_sync(void *_fs)
+{
+        FUNCTION();
+
+        Volume *volume = (Volume *)_fs;
         return volume->Sync();
 }
@@ -219 +214 @@
 */
 static status_t
-bfs_get_vnode(void *_ns, ino_t id, void **_node, bool reenter)
+bfs_get_vnode(void *_fs, ino_t id, void **_node, bool reenter)
 {
         //FUNCTION_START(("ino_t = %Ld\n", id));
-        Volume *volume = (Volume *)_ns;
+        Volume *volume = (Volume *)_fs;
 
         // first inode may be after the log area, we don't go through
@@ -285 +280 @@
 
 static status_t
-bfs_remove_vnode(void *_ns, void *_node, bool reenter)
-{
-        FUNCTION();
-
-        if (_ns == NULL || _node == NULL)
-                return B_BAD_VALUE;
-
-        Volume *volume = (Volume *)_ns;
+bfs_remove_vnode(void *_fs, void *_node, bool reenter)
+{
+        FUNCTION();
+
+        Volume *volume = (Volume *)_fs;
         Inode *inode = (Inode *)_node;
 
@@ -326 +318 @@
 bfs_can_page(fs_volume _fs, fs_vnode _v, fs_cookie _cookie)
 {
-        // ToDo: we're obviously not even asked...
+        // TODO: we're obviously not even asked...
         return false;
 }
@@ -383 +375 @@
         Volume *volume = (Volume *)_fs;
         Inode *inode = (Inode *)_node;
+
+        if (volume->IsReadOnly())
+                return B_READ_ONLY_DEVICE;
 
         if (inode->FileCache() == NULL)
@@ -453 +448 @@
                                 // make sure the extent ends with the last official file
                                 // block (without taking any preallocations into account)
-                                vecs[index].length = (inode->Size() - fileOffset + volume->BlockSize() - 1)
-                                        & ~(volume->BlockSize() - 1);
+                                vecs[index].length = (inode->Size() - fileOffset
+                                        + volume->BlockSize() - 1) & ~(volume->BlockSize() - 1);
                         }
                         *_count = index + 1;
@@ -479 +474 @@
 
 static status_t
-bfs_lookup(void *_ns, void *_directory, const char *file, ino_t *_vnodeID, int *_type)
+bfs_lookup(void *_fs, void *_directory, const char *file, ino_t *_vnodeID,
+        int *_type)
 {
         //FUNCTION_START(("file = %s\n", file));
-        if (_ns == NULL || _directory == NULL || file == NULL || _vnodeID == NULL)
-                return B_BAD_VALUE;
-
-        Volume *volume = (Volume *)_ns;
+
+        Volume *volume = (Volume *)_fs;
         Inode *directory = (Inode *)_directory;
 
@@ -497 +491 @@
                 RETURN_ERROR(B_BAD_VALUE);
 
-        if ((status = tree->Find((uint8 *)file, (uint16)strlen(file), _vnodeID)) < B_OK) {
+        status = tree->Find((uint8 *)file, (uint16)strlen(file), _vnodeID);
+        if (status < B_OK) {
                 //PRINT(("bfs_walk() could not find %Ld:\"%s\": %s\n", directory->BlockNumber(), file, strerror(status)));
                 return status;
@@ -507 +502 @@
 
         Inode *inode;
-        if ((status = get_vnode(volume->ID(), *_vnodeID, (void **)&inode)) != B_OK) {
+        status = get_vnode(volume->ID(), *_vnodeID, (void **)&inode);
+        if (status != B_OK) {
                 REPORT_ERROR(status);
                 return B_ENTRY_NOT_FOUND;
@@ -592 +588 @@
                 case 56742:
                 {
-                        // allocate all free blocks and zero them out (a test for the BlockAllocator)!
+                        // allocate all free blocks and zero them out
+                        // (a test for the BlockAllocator)!
                         BlockAllocator &allocator = volume->Allocator();
                         Transaction transaction(volume, 0);
                         CachedBlock cached(volume);
                         block_run run;
-                        while (allocator.AllocateBlocks(transaction, 8, 0, 64, 1, run) == B_OK) {
+                        while (allocator.AllocateBlocks(transaction, 8, 0, 64, 1, run)
+                                        == B_OK) {
                                 PRINT(("write block_run(%ld, %d, %d)\n", run.allocation_group,
                                         run.start, run.length));
@@ -627 +625 @@
 
 
-/** Sets the open-mode flags for the open file cookie - only
- *      supports O_APPEND currently, but that should be sufficient
- *      for a file system.
- */
-
-static status_t
-bfs_set_flags(void *_ns, void *_node, void *_cookie, int flags)
+/*!     Sets the open-mode flags for the open file cookie - only
+        supports O_APPEND currently, but that should be sufficient
+        for a file system.
+*/
+static status_t
+bfs_set_flags(void *_fs, void *_node, void *_cookie, int flags)
 {
         FUNCTION_START(("node = %p, flags = %d", _node, flags));
@@ -644 +641 @@
 
 
-#if 0
-static status_t
-bfs_select(void *ns, void *node, void *cookie, uint8 event, uint32 ref, selectsync *sync)
-{
-        FUNCTION_START(("event = %d, ref = %lu, sync = %p\n", event, ref, sync));
-        notify_select_event(sync, ref);
-
-        return B_OK;
-}
-
-
-static status_t
-bfs_deselect(void *ns, void *node, void *cookie, uint8 event, selectsync *sync)
-{
-        FUNCTION();
-        return B_OK;
-}
-#endif
-
-static status_t
-bfs_fsync(void *_ns, void *_node)
-{
-        FUNCTION();
-        if (_node == NULL)
-                return B_BAD_VALUE;
+static status_t
+bfs_fsync(void *_fs, void *_node)
+{
+        FUNCTION();
 
         Inode *inode = (Inode *)_node;
@@ -681 +657 @@
 
 
-/**     Fills in the stat struct for a node
- */
-
-static status_t
-bfs_read_stat(void *_ns, void *_node, struct stat *stat)
+static status_t
+bfs_read_stat(void *_fs, void *_node, struct stat *stat)
 {
         FUNCTION();
@@ -697 +670 @@
 
 static status_t
-bfs_write_stat(void *_ns, void *_node, const struct stat *stat, uint32 mask)
-{
-        FUNCTION();
-
-        if (_ns == NULL || _node == NULL || stat == NULL)
-                RETURN_ERROR(B_BAD_VALUE);
-
-        Volume *volume = (Volume *)_ns;
+bfs_write_stat(void *_fs, void *_node, const struct stat *stat, uint32 mask)
+{
+        FUNCTION();
+
+        Volume *volume = (Volume *)_fs;
         Inode *inode = (Inode *)_node;
+
+        if (volume->IsReadOnly())
+                return B_READ_ONLY_DEVICE;
 
         // TODO: we should definitely check a bit more if the new stats are
@@ -789 +762 @@
 
 status_t
-bfs_create(void *_ns, void *_directory, const char *name, int openMode,
+bfs_create(void *_fs, void *_directory, const char *name, int openMode,
         int mode, void **_cookie, ino_t *_vnodeID)
 {
         FUNCTION_START(("name = \"%s\", perms = %d, openMode = %d\n", name, mode, openMode));
 
-        if (_ns == NULL || _directory == NULL || _cookie == NULL
-                || name == NULL || *name == '\0')
-                RETURN_ERROR(B_BAD_VALUE);
-
-        Volume *volume = (Volume *)_ns;
+        Volume *volume = (Volume *)_fs;
         Inode *directory = (Inode *)_directory;
+
+        if (volume->IsReadOnly())
+                return B_READ_ONLY_DEVICE;
 
         if (!directory->IsDirectory())
@@ -827 +799 @@
                 *_cookie = cookie;
 
-                if (created)
-                        notify_entry_created(volume->ID(), directory->ID(), name, *_vnodeID);
+                if (created) {
+                        notify_entry_created(volume->ID(), directory->ID(), name,
+                                *_vnodeID);
+                }
         } else
                 free(cookie);
@@ -837 +811 @@
 
 static status_t
-bfs_create_symlink(void *_ns, void *_directory, const char *name,
+bfs_create_symlink(void *_fs, void *_directory, const char *name,
         const char *path, int mode)
 {
         FUNCTION_START(("name = \"%s\", path = \"%s\"\n", name, path));
 
-        if (_ns == NULL || _directory == NULL || path == NULL
-                || name == NULL || *name == '\0')
-                RETURN_ERROR(B_BAD_VALUE);
-
-        Volume *volume = (Volume *)_ns;
+        Volume *volume = (Volume *)_fs;
         Inode *directory = (Inode *)_directory;
+
+        if (volume->IsReadOnly())
+                return B_READ_ONLY_DEVICE;
 
         if (!directory->IsDirectory())
@@ -900 +873 @@
 
 status_t
-bfs_link(void *ns, void *dir, const char *name, void *node)
+bfs_link(void *_fs, void *dir, const char *name, void *node)
 {
         FUNCTION_START(("name = \"%s\"\n", name));
@@ -911 +884 @@
 
 status_t
-bfs_unlink(void *_ns, void *_directory, const char *name)
+bfs_unlink(void *_fs, void *_directory, const char *name)
 {
         FUNCTION_START(("name = \"%s\"\n", name));
 
-        if (_ns == NULL || _directory == NULL || name == NULL || *name == '\0')
-                return B_BAD_VALUE;
         if (!strcmp(name, "..") || !strcmp(name, "."))
                 return B_NOT_ALLOWED;
 
-        Volume *volume = (Volume *)_ns;
+        Volume *volume = (Volume *)_fs;
         Inode *directory = (Inode *)_directory;
 
@@ -940 +911 @@
 
 status_t
-bfs_rename(void *_ns, void *_oldDir, const char *oldName, void *_newDir, const char *newName)
+bfs_rename(void *_fs, void *_oldDir, const char *oldName, void *_newDir,
+        const char *newName)
 {
         FUNCTION_START(("oldDir = %p, oldName = \"%s\", newDir = %p, newName = \"%s\"\n", _oldDir, oldName, _newDir, newName));
 
         // there might be some more tests needed?!
-        if (_ns == NULL || _oldDir == NULL || _newDir == NULL
-                || oldName == NULL || *oldName == '\0'
-                || newName == NULL || *newName == '\0'
-                || !strcmp(oldName, ".") || !strcmp(oldName, "..")
+        if (!strcmp(oldName, ".") || !strcmp(oldName, "..")
                 || !strcmp(newName, ".") || !strcmp(newName, "..")
                 || strchr(newName, '/') != NULL)
                 RETURN_ERROR(B_BAD_VALUE);
 
-        Volume *volume = (Volume *)_ns;
+        Volume *volume = (Volume *)_fs;
         Inode *oldDirectory = (Inode *)_oldDir;
         Inode *newDirectory = (Inode *)_newDir;
@@ -970 +939 @@
         RecursiveLocker locker(volume->Lock());
 
-        // get the directory's tree, and a pointer to the inode which should be changed
+        // Get the directory's tree, and a pointer to the inode which should be
+        // changed
         BPlusTree *tree;
         status = oldDirectory->GetTree(&tree);
@@ -1024 +994 @@
         }
 
-        status = newTree->Insert(transaction, (const uint8 *)newName, strlen(newName), id);
+        status = newTree->Insert(transaction, (const uint8 *)newName,
+                strlen(newName), id);
         if (status == B_NAME_IN_USE) {
                 // If there is already a file with that name, we have to remove
                 // it, as long it's not a directory with files in it
                 off_t clobber;
-                if (newTree->Find((const uint8 *)newName, strlen(newName), &clobber) < B_OK)
+                if (newTree->Find((const uint8 *)newName, strlen(newName), &clobber)
+                                < B_OK)
                         return B_NAME_IN_USE;
                 if (clobber == id)
@@ -1039 +1011 @@
                         return B_NAME_IN_USE;
 
-                status = newDirectory->Remove(transaction, newName, NULL, other->IsDirectory());
+                status = newDirectory->Remove(transaction, newName, NULL,
+                        other->IsDirectory());
                 if (status < B_OK)
                         return status;
 
-                notify_entry_removed(volume->ID(), newDirectory->ID(), newName, clobber);
-
-                status = newTree->Insert(transaction, (const uint8 *)newName, strlen(newName), id);
+                notify_entry_removed(volume->ID(), newDirectory->ID(), newName,
+                        clobber);
+
+                status = newTree->Insert(transaction, (const uint8 *)newName,
+                        strlen(newName), id);
         }
         if (status < B_OK)
@@ -1066 +1041 @@
 
         if (status == B_OK) {
-                status = tree->Remove(transaction, (const uint8 *)oldName, strlen(oldName), id);
+                status = tree->Remove(transaction, (const uint8 *)oldName,
+                        strlen(oldName), id);
                 if (status == B_OK) {
                         inode->Parent() = newDirectory->BlockRun();
@@ -1075 +1051 @@
                         if (oldDirectory != newDirectory
                                 && inode->IsDirectory()
-                                && (status = inode->GetTree(&movedTree)) == B_OK)
-                                status = movedTree->Replace(transaction, (const uint8 *)"..", 2, newDirectory->ID());
+                                && (status = inode->GetTree(&movedTree)) == B_OK) {
+                                status = movedTree->Replace(transaction, (const uint8 *)"..",
+                                        2, newDirectory->ID());
+                        }
 
                         if (status == B_OK)
@@ -1095 +1073 @@
                         // Anyway, if we overwrote a file in the target directory
                         // this is lost now (only in-memory, not on-disk)...
-                        bailStatus = tree->Insert(transaction, (const uint8 *)oldName, strlen(oldName), id);
-                        if (movedTree != NULL)
-                                movedTree->Replace(transaction, (const uint8 *)"..", 2, oldDirectory->ID());
+                        bailStatus = tree->Insert(transaction, (const uint8 *)oldName,
+                                strlen(oldName), id);
+                        if (movedTree != NULL) {
+                                movedTree->Replace(transaction, (const uint8 *)"..", 2,
+                                        oldDirectory->ID());
+                        }
                 }
         }
@@ -1112 +1093 @@
         }
 
-        if (bailStatus == B_OK)
-                bailStatus = newTree->Remove(transaction, (const uint8 *)newName, strlen(newName), id);
+        if (bailStatus == B_OK) {
+                bailStatus = newTree->Remove(transaction, (const uint8 *)newName,
+                        strlen(newName), id);
+        }
 
         if (bailStatus < B_OK)
@@ -1134 +1117 @@
         if (inode->IsDirectory() && openMode & O_RWMASK) {
                 openMode = openMode & ~O_RWMASK;
-                // ToDo: for compatibility reasons, we don't return an error here...
+                // TODO: for compatibility reasons, we don't return an error here...
                 // e.g. "copyattr" tries to do that
                 //return B_IS_A_DIRECTORY;
@@ -1144 +1127 @@
                 RETURN_ERROR(status);
 
-        // we could actually use the cookie to keep track of:
+        // We could actually use the cookie to keep track of:
         //      - the last block_run
         //      - the location in the data_stream (indirect, double indirect,
@@ -1186 +1169 @@
 
 static status_t
-bfs_read(void *_ns, void *_node, void *_cookie, off_t pos, void *buffer,
+bfs_read(void *_fs, void *_node, void *_cookie, off_t pos, void *buffer,
         size_t *_length)
 {
@@ -1202 +1185 @@
 
 static status_t
-bfs_write(void *_ns, void *_node, void *_cookie, off_t pos, const void *buffer,
+bfs_write(void *_fs, void *_node, void *_cookie, off_t pos, const void *buffer,
         size_t *_length)
 {
         //FUNCTION();
-        Volume *volume = (Volume *)_ns;
+        Volume *volume = (Volume *)_fs;
         Inode *inode = (Inode *)_node;
+
+        if (volume->IsReadOnly())
+                return B_READ_ONLY_DEVICE;
 
         if (!inode->HasUserAccessableStream()) {
@@ -1234 +1220 @@
 
                 // periodically notify if the file size has changed
-                // ToDo: should we better test for a change in the last_modified time only?
+                // TODO: should we better test for a change in the last_modified time only?
                 if (!inode->IsDeleted() && cookie->last_size != inode->Size()
-                        && system_time() > cookie->last_notification + INODE_NOTIFICATION_INTERVAL) {
+                        && system_time() > cookie->last_notification
+                                        + INODE_NOTIFICATION_INTERVAL) {
                         notify_stat_changed(volume->ID(), inode->ID(),
                                 B_STAT_MODIFICATION_TIME | B_STAT_SIZE | B_STAT_INTERIM_UPDATE);
@@ -1248 +1235 @@
 
 
-/**     Do whatever is necessary to close a file, EXCEPT for freeing
- *      the cookie!
- */
-
-static status_t
-bfs_close(void *_ns, void *_node, void *_cookie)
-{
-        FUNCTION();
-        if (_ns == NULL || _node == NULL || _cookie == NULL)
-                return B_BAD_VALUE;
-
-        return B_OK;
-}
-
-
-static status_t
-bfs_free_cookie(void *_ns, void *_node, void *_cookie)
-{
-        FUNCTION();
-
-        if (_ns == NULL || _node == NULL || _cookie == NULL)
-                return B_BAD_VALUE;
+static status_t
+bfs_close(void *_fs, void *_node, void *_cookie)
+{
+        FUNCTION();
+        return B_OK;
+}
+
+
+static status_t
+bfs_free_cookie(void *_fs, void *_node, void *_cookie)
+{
+        FUNCTION();
 
         file_cookie *cookie = (file_cookie *)_cookie;
-        Volume *volume = (Volume *)_ns;
+        Volume *volume = (Volume *)_fs;
         Inode *inode = (Inode *)_node;
 
         Transaction transaction;
-        bool needsTrimming;
-
-        {
+        bool needsTrimming = false;
+
+        if (!volume->IsReadOnly()) {
                 ReadLocked locker(inode->Lock());
                 needsTrimming = inode->NeedsTrimming();
@@ -1292 +1269 @@
         }
 
-        WriteLocked locker(inode->Lock());
         status_t status = transaction.IsStarted() ? B_OK : B_ERROR;
 
         if (status == B_OK) {
+                WriteLocked locker(inode->Lock());
+
                 // trim the preallocated blocks and update the size,
                 // and last_modified indices if needed
@@ -1343 +1321 @@
 
 
-/**     Checks access permissions, return B_NOT_ALLOWED if the action
- *      is not allowed.
- */
-
-static status_t
-bfs_access(void *_ns, void *_node, int accessMode)
+/*!     Checks access permissions, return B_NOT_ALLOWED if the action
+        is not allowed.
+*/