vfs: change VOP_UNLINK parameters, more fixes

kernel-src/fs/ext2.c

@@ -1176,7 +1176,8 @@ static int ext2_lookup(vnode_t *vnode, char *name, vnode_t **result, cred_t *cre
 
 	// VOP_LOOKUP returns the vnode locked
 	*result = &newnode->vnode;
-	VOP_LOCK(*result);
+	if (newnode != node)
+		VOP_LOCK(*result);
 
 	cleanup:
 	if (err && newnode) {
@@ -1487,9 +1488,7 @@ static int ext2_symlink(vnode_t *vnode, char *name, vattr_t *attr, char *path, c
 	if (err)
 		return err;
 
-	// no lock needed, not gonna be accessed by anyone else as the direcotry lock is held
 	ext2node_t *newnode = (ext2node_t *)newvnode;
-	VOP_UNLOCK(newvnode);
 
 	if (linklen <= 60) {
 		// store it in the inode
@@ -1511,8 +1510,12 @@ static int ext2_symlink(vnode_t *vnode, char *name, vattr_t *attr, char *path, c
 	VOP_RELEASE(newvnode);
 
 	cleanup:
-	if (err)
+	if (err) {
-		ASSERT_UNCLEAN(fs, VOP_UNLINK(vnode, name, cred) == 0);
+		ASSERT_UNCLEAN(fs, VOP_UNLINK(vnode, newvnode, name, cred) == 0);
+	} else {
+		VOP_UNLOCK(newvnode); // locked by internalcreate
+	}
+
 	return err;
 }
 
@@ -1582,22 +1585,16 @@ static int handleinodeunlink(ext2fs_t *fs, ext2node_t *node, int inode) {
 	return err;
 }
 
-static int internalunlink(ext2fs_t *fs, ext2node_t *node, char *name) {
+static int ext2_unlink(vnode_t *vnode, vnode_t *child, char *name, cred_t *cred) {
+	ext2node_t *node = (ext2node_t *)vnode;
+	ext2fs_t *fs = (ext2fs_t *)vnode->vfs;
+
 	int inode = 0;
 	int err = removedent(fs, node, name, &inode);
 	if (err)
 		return err;
 
 	err = handleinodeunlink(fs, node, inode);
-	return err;
-}
-
-// TODO name -> child, name
-static int ext2_unlink(vnode_t *vnode, char *name, cred_t *cred) {
-	ext2node_t *node = (ext2node_t *)vnode;
-	ext2fs_t *fs = (ext2fs_t *)vnode->vfs;
-
-	int err = internalunlink(fs, node, name);
 
 	return err;
 }
@@ -1686,7 +1683,7 @@ static int ext2_rename(vnode_t *sourcedir, char *oldname, vnode_t *targetdir, ch
 
 	if (ext2sourcenode->id == oldinode) {
 		// link still points to the same inode, unlink it
-		err = internalunlink(fs, ext2sourcedirnode, oldname);
+		err = VOP_UNLINK(sourcedir, source, oldname, NULL);
 	}
 
 	cleanup:

kernel-src/fs/tmpfs.c

@@ -235,7 +235,7 @@ static int tmpfs_access(vnode_t *vnode, mode_t mode, cred_t *cred) {
 	return auth_filesystem_check(cred, auth_filesystem_convertaccess(mode), vnode) ? EACCES : 0;
 }
 
-static int tmpfs_unlink(vnode_t *node, char *name, cred_t *cred) {
+static int tmpfs_unlink(vnode_t *node, vnode_t *child, char *name, cred_t *cred) {
 	tmpfsnode_t *tmpnode = (tmpfsnode_t *)node;
 	if (node->type != V_TYPE_DIR)
 		return ENOTDIR;
@@ -254,6 +254,7 @@ static int tmpfs_unlink(vnode_t *node, char *name, cred_t *cred) {
 
 	vnode_t *unlinknode = r;
 	tmpfsnode_t *unlinktmpnode = r;
+	__assert(child == unlinknode);
 
 	err = hashtable_remove(&tmpnode->children, name, namelen);
 	if (err)

kernel-src/fs/vfs.c

@@ -40,6 +40,15 @@ int vfs_pollstub(vnode_t *node, struct polldata *, int events) {
 	return revents;
 }
 
+static int noop() {
+	return 0;
+}
+
+static vops_t vnops = {
+	.lock = noop,
+	.unlock = noop
+};
+
 void vfs_init() {
 	__assert(hashtable_init(&fstable, 20) == 0);
 	vfsroot = alloc(sizeof(vnode_t));
@@ -47,6 +56,7 @@ void vfs_init() {
 	SPINLOCK_INIT(listlock);
 	vfsroot->type = V_TYPE_DIR;
 	vfsroot->refcount = 1;
+	vfsroot->ops = &vnops;
 }
 
 int vfs_register(vfsops_t *ops, char *name) {
@@ -131,7 +141,7 @@ int vfs_open(vnode_t *ref, char *path, int flags, vnode_t **res) {
 		return err;
 	}
 
-	vnode_t *new = ref;
+	vnode_t *new = tmp;
 	err = VOP_OPEN(&new, flags, getcred());
 	if (err) {
 		VOP_RELEASE(tmp);
@@ -455,7 +465,25 @@ int vfs_unlink(vnode_t *ref, char *path) {
 	if (err)
 		goto cleanup;
 
-	VOP_UNLINK(parent, component, getcred());
+	vnode_t *child = NULL;
+	err = VOP_LOOKUP(parent, component, &child, getcred());
+	if (err) {
+		VOP_UNLOCK(parent);
+		goto cleanup;
+	}
+
+	// TODO auth check
+	if (0) {
+		// locked by VOP_LOOKUP
+		VOP_UNLOCK(child);
+		// locked by vfs_lookup
+		VOP_UNLOCK(parent);
+		return err;
+	}
+
+	err = VOP_UNLINK(parent, child, component, getcred());
+	// locked by VOP_LOOKUP
+	VOP_UNLOCK(child);
 	// locked by vfs_lookup
 	VOP_UNLOCK(parent);
 	VOP_RELEASE(parent);
@@ -509,7 +537,9 @@ int vfs_lookup(vnode_t **result, vnode_t *start, char *path, char *lastcomp, int
 			return e;
 
 		if ((flags & VFS_LOOKUP_INPUTLOCKED) == 0) {
+			arch_e9_puts("lock1\n");
 			VOP_LOCK(r);
+			arch_e9_puts("lock1ok\n");
 		}
 
 		VOP_HOLD(r);
@@ -540,8 +570,10 @@ int vfs_lookup(vnode_t **result, vnode_t *start, char *path, char *lastcomp, int
 	error = 0;
 	VOP_HOLD(current);
 
+	arch_e9_puts("lock2\n");
 	if ((flags & VFS_LOOKUP_INPUTLOCKED) == 0 || current != start)
 		VOP_LOCK(current);
+	arch_e9_puts("lock2ok\n");
 
 	for (int i = 0; i < pathlen; ++i) {
 		if (compbuffer[i] == '\0')
@@ -563,12 +595,16 @@ int vfs_lookup(vnode_t **result, vnode_t *start, char *path, char *lastcomp, int
 			islast = j == pathlen;
 		}
 
+		arch_e9_puts("component: ");
+		arch_e9_puts(component);
+
 		if (islast && (flags & VFS_LOOKUP_PARENT)) {
 			__assert(lastcomp);
 			strcpy(lastcomp, component);
 			break;
 		}
 
+
 		if (strcmp(component, "..") == 0) {
 			// if the tree root, skip to next component
 			vnode_t *root = NULL;
@@ -581,6 +617,7 @@ int vfs_lookup(vnode_t **result, vnode_t *start, char *path, char *lastcomp, int
 			// if the root of a mounted fs, go to
 			if (current->flags & V_FLAGS_ROOT) {
 				vnode_t *n = lowestnodeinmp(current);
+				arch_e9_puts("lock3\n");
 				if (n != current) {
 					VOP_HOLD(n);
 					VOP_UNLOCK(current);
@@ -588,6 +625,7 @@ int vfs_lookup(vnode_t **result, vnode_t *start, char *path, char *lastcomp, int
 					current = n;
 					VOP_LOCK(current);
 				}
+				arch_e9_puts("lock3ok\n");
 			}
 		}
 
@@ -596,7 +634,9 @@ int vfs_lookup(vnode_t **result, vnode_t *start, char *path, char *lastcomp, int
 		if (error)
 			break;
 
+		arch_e9_puts("lookup\n");
 		error = VOP_LOOKUP(current, component, &next, getcred());
+		arch_e9_puts("lookupok\n");
 		if (error)
 			break;
 
@@ -614,11 +654,13 @@ int vfs_lookup(vnode_t **result, vnode_t *start, char *path, char *lastcomp, int
 		}
 
 		if (r != next) {
+			arch_e9_puts("lock4\n");
 			VOP_HOLD(r);
 			VOP_UNLOCK(next);
 			VOP_RELEASE(next);
 			next = r;
 			VOP_LOCK(next);
+			arch_e9_puts("lock4ok\n");
 		}
 
 		// dereference symlink
@@ -652,7 +694,9 @@ int vfs_lookup(vnode_t **result, vnode_t *start, char *path, char *lastcomp, int
 			}
 
 			// TODO possible deadlock when input vnode is locked and the vfs_lookup would lock it
+			arch_e9_puts("it1\n");
 			error = vfs_lookup(&derefresult, derefstart, linkderef, NULL, pass);
+			arch_e9_puts("it1ok\n");
 
 			if (*linkderef == '/')
 				VOP_RELEASE(derefstart);

kernel-src/include/kernel/vfs.h

@@ -126,7 +126,7 @@ typedef struct vops_t {
 	int (*setattr)(vnode_t *node, vattr_t *attr, int attrs, cred_t *cred);
 	int (*poll)(vnode_t *node, struct polldata *, int events);
 	int (*access)(vnode_t *node, mode_t mode, cred_t *cred);
-	int (*unlink)(vnode_t *node, char *name, cred_t *cred);
+	int (*unlink)(vnode_t *node, vnode_t *child, char *name, cred_t *cred);
 	int (*link)(vnode_t *node, vnode_t *dir, char *name, cred_t *cred);
 	int (*symlink)(vnode_t *parent, char *name, vattr_t *attr, char *path, cred_t *cred);
 	int (*readlink)(vnode_t *parent, char **link, cred_t *cred);
@@ -180,7 +180,7 @@ typedef struct vops_t {
 #define VOP_GETATTR(v, a, c) (v)->ops->getattr(v, a, c)
 #define VOP_SETATTR(v, a, w, c) (v)->ops->setattr(v, a, w, c)
 #define VOP_ACCESS(v, m, c) (v)->ops->access(v, m, c)
-#define VOP_UNLINK(v, n, c) (v)->ops->unlink(v, n, c)
+#define VOP_UNLINK(v, ch, n, c) (v)->ops->unlink(v, ch, n, c)
 #define VOP_LINK(v, d, n, c) (v)->ops->link(v, d, n, c)
 #define VOP_SYMLINK(v, n, a, p, c) (v)->ops->symlink(v, n, a, p, c)
 #define VOP_READLINK(v, l, c) (v)->ops->readlink(v, l, c)