meterfs: Implement mount, getAttr, setAttr, stat and readdir by agkaminski · Pull Request #119 · phoenix-rtos/phoenix-rtos-filesystems

agkaminski · 2024-01-11T12:53:58Z

JIRA: RTOS-182

Description

Change is intended as non-breaking, old API should be still functional

Motivation and Context

Allow meterfs to be mounted in a directory and make 'ls' work

Types of changes

Bug fix (non-breaking change which fixes an issue)
New feature (non-breaking change which adds functionality)
Breaking change (fix or feature that would cause existing functionality to change)

How Has This Been Tested?

Already covered by automatic testing.
New test added: (add PR link here).
Tested by hand on: (stm32l4x6).

Checklist:

My change requires a change to the documentation.
I have updated the documentation accordingly.
I have added tests to cover my changes.
All new and existing linter checks and tests passed.
My changes generate no new compilation warnings for any of the targets.

Special treatment

This PR needs additional PRs to work (list the PRs, preferably in merge-order).
I will merge this PR by myself when appropriate.

JIRA: RTOS-182

nalajcie · 2024-01-11T13:17:14Z

readdir ❤️

kemonats · 2024-01-11T18:39:22Z

meterfs/meterfs.c

-		if (name[i] == '/')
-			return -ENOENT;
+		char *end = strchrnul(name + len, '/');
+		const size_t size = end - name + len;


Probably should be:

Suggested change

const size_t size = end - name + len;

const size_t size = end - (name + len);

kemonats · 2024-01-11T18:39:37Z

meterfs/meterfs.c

+	}
+
+	/* Hack-ish - handle virtual "." and ".." */
+	if (offs == 0) { /* "."" */


Suggested change

if (offs == 0) { /* "."" */

if (offs == 0) { /* "." */

same in line 1476

kemonats · 2024-01-11T18:39:46Z

meterfs/meterfs.c

+		dent->d_reclen = len;
+		dent->d_namlen = len;
+		dent->d_type = DT_REG;
+		strncpy(dent->d_name, f->header.name, len);


it should be faster

Suggested change

strncpy(dent->d_name, f->header.name, len);

memcpy(dent->d_name, f->header.name, len);

kemonats · 2024-01-11T18:39:59Z

meterfs/meterfs.c

+	if (parent == NULL) {
+		return -ENOMEM;
+	}
+	memcpy(parent, ctx->mountpoint, mntlen + 1);


a little faster

Suggested change

memcpy(parent, ctx->mountpoint, mntlen + 1);

memcpy(parent, ctx->mountpoint, mntlen);

kemonats · 2024-01-12T09:51:16Z

meterfs/meterfs.c

 int meterfs_init(meterfs_ctx_t *ctx)
 {
-	int err;
+	ctx->mountpoint = NULL;


Move it after checking the condition ctx == NULL

anglov

Not reviewied code, I just playing with it a little with adapted flashsrv for DEND project.
My findings below:
1.meterfs: Filesystem check done. Found 23 files.,but ls show 24 files. One file is doubled
2. echo "abc" > /meterfs_mount_point/mfs_file causes that psh destroys file in unexpected way.

agkaminski · 2025-04-09T11:44:03Z

/gemini

gemini-code-assist · 2025-04-09T11:44:08Z

It looks like you ran the /gemini command without specifying which action you wanted to perform. If you intended to trigger a code review, the command is /gemini review. For a list of available commands, use /gemini help.

agkaminski · 2025-04-09T11:44:21Z

/gemini review

gemini-code-assist

Code Review

The pull request introduces several new features to the meterfs, including mount, getAttr, setAttr, stat, and readdir functionalities. The changes seem well-structured and address the stated goal of enabling meterfs to be mounted in a directory and making 'ls' work. However, there are a few areas that could benefit from further attention.

Summary of Findings

Error Handling in meterfs_mount: The meterfs_mount function allocates memory for parent and mountPoint. If realloc fails, the original ctx->mountpoint is leaked. This should be addressed to prevent memory leaks.
Inconsistent ID Handling: The code mixes id_t and oid_t types, especially in the meterfs_lookup functions. This could lead to confusion and potential errors. Standardizing on oid_t and removing the deprecated meterfs_lookup would improve clarity.
Magic Number in meterfs_readdir: The meterfs_readdir function uses the magic number 3 to adjust the offset. This could be made more explicit with a named constant.
TODO comment in meterfs_stat: The meterfs_stat function has a TODO comment regarding the filesystem ID. This should be addressed to ensure proper filesystem identification.

Merge Readiness

The pull request introduces significant new functionality and is a step in the right direction. However, there are several issues that need to be addressed before merging, particularly the memory leak in meterfs_mount and the inconsistent ID handling. I am unable to directly approve this pull request, and recommend that it not be merged until the critical and high severity issues are addressed, and that others review and approve this code before merging.

gemini-code-assist · 2025-04-09T11:44:39Z

meterfs/meterfs.c

+	int ret = lookup(parent, &ctx->parent, NULL);
+	free(parent);
+	if (ret < 0) {
+		return -ENOENT;
+	}
+
+	void *mountPoint = realloc(ctx->mountpoint, mntlen + 1);
+	if (mountPoint == NULL) {
+		return -ENOMEM;
+	}
+	ctx->mountpoint = mountPoint;


If realloc fails, the original ctx->mountpoint is leaked. This should be addressed to prevent memory leaks. Also, lookup is undeclared, so it should be defined or replaced with a standard library function.

int ret = lookup(parent, &ctx->parent, NULL); free(parent); if (ret < 0) { free(ctx->mountpoint); // Free previously allocated mountpoint return -ENOENT; } void *mountPoint = realloc(ctx->mountpoint, mntlen + 1); if (mountPoint == NULL) { free(ctx->mountpoint); // Free previously allocated mountpoint return -ENOMEM; } ctx->mountpoint = mountPoint;

gemini-code-assist · 2025-04-09T11:44:39Z

meterfs/meterfs.c

+				*res = ctx->parent;
+				return len - 1;


The return value len - 1 seems unusual. Should this be len + size instead, to be consistent with the other successful return?

Suggested change

*res = ctx->parent;

return len - 1;

*res = ctx->parent;

return len + size;

gemini-code-assist · 2025-04-09T11:44:39Z

meterfs/meterfs.c

+/* Backward compatibility, remove after adapting all flashsrvs */
+int meterfs_lookup(const char *name, id_t *res, meterfs_ctx_t *ctx)
+{
+	oid_t oid;
+	int ret = meterfs_lookupOid(name, &oid, ctx);
+	*res = oid.id;
+	return ret;
 }


Since meterfs_lookup is deprecated, consider removing it entirely to avoid confusion and simplify the API. If it must be retained for backward compatibility, add a prominent comment indicating its deprecated status and suggesting the use of meterfs_lookupOid instead.

Suggested change

/* Backward compatibility, remove after adapting all flashsrvs */

int meterfs_lookup(const char *name, id_t *res, meterfs_ctx_t *ctx)

{

oid_t oid;

int ret = meterfs_lookupOid(name, &oid, ctx);

*res = oid.id;

return ret;

}

/* Deprecated: Use meterfs_lookupOid instead */

int meterfs_lookup(const char *name, id_t *res, meterfs_ctx_t *ctx)

{

oid_t oid;

int ret = meterfs_lookupOid(name, &oid, ctx);

*res = oid.id;

return ret;

}

gemini-code-assist · 2025-04-09T11:44:39Z

meterfs/meterfs.c

+				/* TODO - remove redundant custom enums */
+				*attr = (id == ROOTDIR_ID) ? otDir : otFile;


The comment "TODO - remove redundant custom enums" indicates that the custom enums otDir and otFile should be removed. Consider doing this as part of this PR, or creating a separate issue to track this work.

Suggested change

/* TODO - remove redundant custom enums */

*attr = (id == ROOTDIR_ID) ? otDir : otFile;

/* Handle fs rootdir, otherwise regular file */

*attr = (id == ROOTDIR_ID) ? S_IFDIR : S_IFREG;

meterfs: Implement mount, getAttr, setAttr, stat and readdir

b9ede5a

JIRA: RTOS-182

kemonats reviewed Jan 11, 2024

View reviewed changes

kemonats reviewed Jan 12, 2024

View reviewed changes

anglov requested changes Jan 23, 2024

View reviewed changes

gemini-code-assist bot suggested changes Apr 9, 2025

View reviewed changes

	const size_t size = end - name + len;
	const size_t size = end - (name + len);

	strncpy(dent->d_name, f->header.name, len);
	memcpy(dent->d_name, f->header.name, len);

	memcpy(parent, ctx->mountpoint, mntlen + 1);
	memcpy(parent, ctx->mountpoint, mntlen);

		/* TODO - remove redundant custom enums */
		*attr = (id == ROOTDIR_ID) ? otDir : otFile;

Conversation

agkaminski commented Jan 11, 2024

Description

Motivation and Context

Types of changes

How Has This Been Tested?

Checklist:

Special treatment

Uh oh!

nalajcie commented Jan 11, 2024

Uh oh!

kemonats Jan 11, 2024

Choose a reason for hiding this comment

Uh oh!

kemonats Jan 11, 2024

Choose a reason for hiding this comment

Uh oh!

kemonats Jan 11, 2024

Choose a reason for hiding this comment

Uh oh!

kemonats Jan 11, 2024

Choose a reason for hiding this comment

Uh oh!

kemonats Jan 12, 2024

Choose a reason for hiding this comment

Uh oh!

anglov left a comment

Choose a reason for hiding this comment

Uh oh!

agkaminski commented Apr 9, 2025

Uh oh!

gemini-code-assist bot commented Apr 9, 2025

Uh oh!

agkaminski commented Apr 9, 2025

Uh oh!

gemini-code-assist bot left a comment

Choose a reason for hiding this comment

Code Review

Summary of Findings

Merge Readiness

Uh oh!

gemini-code-assist bot Apr 9, 2025

Choose a reason for hiding this comment

Uh oh!

gemini-code-assist bot Apr 9, 2025

Choose a reason for hiding this comment

Uh oh!

gemini-code-assist bot Apr 9, 2025

Choose a reason for hiding this comment

Uh oh!

gemini-code-assist bot Apr 9, 2025

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants