Move meta file usage to a file backed hashtable by corbinlc · Pull Request #11 · CypherpunkArmory/proot

corbinlc · 2019-01-28T19:06:54Z

Going to do much more testing now, but wanted to get this out, so we could get a code review going

MatthewTighe · 2019-01-28T19:29:35Z

src/extension/fake_id0/helper_functions.c

+
+		if (err != NULL) {
+			read_len = 0;
+			fprintf(stderr, "Read fail.\n");


Seems like you might want a more verbose error message here.

MatthewTighe · 2019-01-28T19:39:09Z

src/extension/fake_id0/helper_functions.c

+		lcl_mode = hash_read_value->mode;
+		*owner = hash_read_value->owner;
+		*group = hash_read_value->group;
+	} else { 


Is it possible that the meta file is deleted in this branch with the call to unlink but the process dies before a call that writes it to the hash?

that is true, should do a put here

+1 for matt

MatthewTighe · 2019-01-28T19:40:27Z

src/extension/fake_id0/helper_functions.c

@@ -341,11 +394,11 @@ int read_meta_file(char path[PATH_MAX], mode_t *mode, uid_t *owner, gid_t *group
 int write_meta_file(char path[PATH_MAX], mode_t mode, uid_t owner, gid_t group,


I think you should rename this function.

MatthewTighe · 2019-01-28T19:41:41Z

src/extension/fake_id0/helper_functions.c

+
+	if ((status == 0) && (addr > 0)) {
+        	ht_value = malloc(sizeof(diskhash_struct_t));
+        	ht_value->mode = dtoo(mode);


If we aren't saving the mode in a human-readable file anymore, maybe we should just abandon the otod and dtoo conversions?

not going to do this just yet, need to convince myself that everyone expects it one way. i know we we check permissions we want it in the octal format. everywhere else maybe decimal format

MatthewTighe · 2019-01-28T19:43:59Z

src/extension/fake_id0/helper_functions.c

+
+	options = leveldb_options_create();
+	leveldb_options_set_create_if_missing(options, 1);
+	db = leveldb_open(options, "/tmp/testExtract/testdb", &err);


I think test terminology should be changed.

MatthewTighe · 2019-01-28T19:45:10Z

src/extension/fake_id0/helper_functions.c

+        	ht_value->group = group;
+		leveldb_put(db, woptions, (char *)&addr, sizeof(ino_t), (char *)ht_value, sizeof(diskhash_struct_t), &err);
+		if (err != NULL) {
+			fprintf(stderr, "Write fail.\n");


Think you may end up wanting a more verbose error message here as well.

MatthewTighe · 2019-01-28T20:28:58Z

src/extension/fake_id0/mk.c

+	if(status == 1)
+		return 0;
+
+	/* If the file already existed get iyt*/


Typo in this comment could be misread as 'it' instead of 'out'.

MatthewTighe · 2019-01-28T20:36:05Z

src/extension/fake_id0/helper_functions.c

-		*group = config->egid;
-		*mode = otod(755);
-		return 0;
+	int status;


I think you might have an easier time removing metafile functionality during a later refactor if you extracted the leveldb read into its own routine, and did metafile reading fallback if that routine fails. At the very least, I think it would be useful to have clearer semantics concerning what read_meta_file does. It's called in a lot of places, but "reading a meta file" isn't really the intent of this function anymore.

MatthewTighe · 2019-01-28T20:37:53Z

src/extension/fake_id0/open.c

 	return 0;
 }

+/** Handles open, openat, and creat syscalls. Creates meta files to match the


This comment seems a little disingenuous, and it comes down again to the naming convention of writing/reading "meta files". It seems like that's not really what we're trying to accomplish anymore.

MatthewTighe · 2019-01-28T20:39:44Z

src/extension/fake_id0/fake_id0.c

 		Config *config;
 		int uid, gid;

+#ifdef USERLAND


Do we use this definition everywhere we make calls to read_ and write_ meta files now? Otherwise, what happens when access to the hash is attempted?

corbinlc · 2019-01-30T22:15:46Z

src/extension/fake_id0/helper_functions.c

-		return status;

-	perms = get_permissions(meta_path, config, 0);
+	perms = get_permissions(path, config, 0);


This should be shorten_path not path. $^%&!

…ll open_exit_end routine

…ing or writing meta info

Unrud · 2019-04-20T17:12:38Z

Wouldn't it be easier to store the metadata in extended attributes? Otherwise, the file system and the database must kept in sync, when files or whole folders are renamed.

corbinlc · 2019-05-13T23:03:18Z

@Unrud that is a really great idea! I stopped looking at this because I found that there was still too much of a performance impact of keeping track of the emulated file ownership and such. There is a reasonable impact to intercepting system calls that happen often like open (which sets the initial ownership and permissions of a new file). This idea though, might make it worthwhile to revisit this.

corbinlc · 2019-05-13T23:03:54Z

Really brilliant idea if it works though.

move meta file usage to a filebacked hashmap

2c6eb66

MatthewTighe reviewed Jan 28, 2019

View reviewed changes

corbinlc added 3 commits January 28, 2019 14:14

make it more portable and print errors the proot way

2f029e4

small fix

35ed6c6

change init flow to only happen after bindings are resolved

1c532c3

corbinlc commented Jan 30, 2019

View reviewed changes

corbinlc added 7 commits January 30, 2019 14:21

use shorten_path, not path when checking directory permissions and ca…

75a97de

…ll open_exit_end routine

add unlink functionality for meta db

3df19a9

cleaning up some things based on review, more to come later

34b5d0b

everything i am changing for now related to leveldb

3ebacf1

remove directory checks to further improve performance

c60a7df

handle migration correctly and store mode in decimal

8ea1532

use lstat instead of stat to not follow symbolic links down when read…

998dd31

…ing or writing meta info

Gorlof approved these changes Apr 12, 2025

View reviewed changes

		@@ -341,11 +394,11 @@ int read_meta_file(char path[PATH_MAX], mode_t mode, uid_t owner, gid_t *group
		int write_meta_file(char path[PATH_MAX], mode_t mode, uid_t owner, gid_t group,

Conversation

corbinlc commented Jan 28, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Unrud commented Apr 20, 2019

Uh oh!

corbinlc commented May 13, 2019

Uh oh!

corbinlc commented May 13, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants