Revert "Add support fchstat and chstat function for littlefs" #14479

yamt · 2024-10-23T09:24:55Z

Reverts #13964

because:

it seems to introduce some regressions. i saw fstat failing with ENOENT on files on an image generated with https://github.com/jrast/littlefs-python.
we should reduce the amount of the local patches. not the opposite. if you want to extend littlefs, please upstream it first.
it's controversial if it's a good idea to waste on-disk structures for attributes like file modes and uid/gid, which nuttx doesn't really support.

This reverts commit 69f3774.

This reverts commit da5839c.

This reverts commit 8f6ea88.

This reverts commit c577929.

This reverts commit 0ab637f.

This reverts commit f70de1c.

This reverts commit 44992c8.

nuttxpr · 2024-10-23T09:31:11Z

[Experimental Bot, please feedback here]

Based on the provided description, this PR does not meet the NuttX requirements.

Missing Information:

Summary: The PR only states it reverts a previous change. It lacks information on the necessity of the revert, affected code, and how the revert addresses the issue.
Impact: Reverting a change can have significant impact. The PR needs to address all impact points, specifying which areas are affected by the revert and how.
Testing: While mentioning "Testing logs before change" and "Testing logs after change", the PR provides no actual logs. Concrete evidence of testing both before and after the revert is crucial.

Recommendation:

The PR needs significant revisions to meet NuttX requirements. The author should provide:

Detailed Summary: Explain why Add support fchstat and chstat function for littlefs #13964 needs reverting, what functionalities are impacted, and how the revert addresses the original PR's issues.
Thorough Impact Analysis: Analyze and describe the revert's impact on all mentioned aspects (user, build, hardware, documentation, security, compatibility).
Concrete Testing Evidence: Include actual testing logs from before and after the revert to demonstrate the change's effectiveness.

xiaoxiang781216 · 2024-10-24T02:35:56Z

can we add an option to make the new feature selectable?

yamt · 2024-10-24T03:03:49Z

can we add an option to make the new feature selectable?

it's possible. but it's better to remove it for now, IMO.
adding more and more local patches like this here makes the use of different versions of littlefs impossible.

xiaoxiang781216 · 2024-11-01T01:22:14Z

fs/littlefs/lfs_vfs.c

@@ -1572,15 +1571,6 @@ static int littlefs_stat(FAR struct inode *mountpt, FAR const char *relpath,
  buf->st_blocks       = (buf->st_size + buf->st_blksize - 1) /
                         buf->st_blksize;

-  if (info.type == LFS_TYPE_REG)


need keep this patch

xiaoxiang781216 · 2024-11-01T01:23:16Z

fs/littlefs/lfs_vfs.c

@@ -1575,7 +1556,6 @@ static int littlefs_stat(FAR struct inode *mountpt, FAR const char *relpath,

      ret = 0;
      memset(&attr, 0, sizeof(attr));
-      attr.at_mode = S_IRWXG | S_IRWXU | S_IRWXO;


need keep this change

fs/littlefs/lfs_vfs.c

yamt · 2024-11-01T03:45:35Z

@xiaoxiang781216
i'm not sure which changes you are talking about. can you explain a bit?

anyway, i guess the original PR should not have contained unrelated changes together.
i suspect it's simpler to revert the PR as a whole and ask the author to re-submit those parts separately.

xiaoxiang781216 · 2024-11-01T04:14:51Z

@xiaoxiang781216 i'm not sure which changes you are talking about. can you explain a bit?

anyway, i guess the original PR should not have contained unrelated changes together. i suspect it's simpler to revert the PR as a whole and ask the author to re-submit those parts separately.

Ok, let's me revert.

yamt · 2024-11-13T08:56:51Z

@xiaoxiang781216 i'm not sure which changes you are talking about. can you explain a bit?
anyway, i guess the original PR should not have contained unrelated changes together. i suspect it's simpler to revert the PR as a whole and ask the author to re-submit those parts separately.

Ok, let's me revert.

what's the status of this?

xiaoxiang781216 · 2024-11-13T11:25:46Z

I will send in this weak.

xiaoxiang781216 · 2024-11-16T18:14:11Z

@xiaoxiang781216 i'm not sure which changes you are talking about. can you explain a bit?
anyway, i guess the original PR should not have contained unrelated changes together. i suspect it's simpler to revert the PR as a whole and ask the author to re-submit those parts separately.

Ok, let's me revert.

@yamt After comparing carefully all nuttx and littlefs patch, I think the change is good in general. The follow is my answer to your concern.

Reverts #13964

because:

it seems to introduce some regressions. i saw fstat failing with ENOENT on files on an image generated with https://github.com/jrast/littlefs-python.

Could you give a repro step? So, we can provide a fix.

we should reduce the amount of the local patches. not the opposite. if you want to extend littlefs, please upstream it first.

It upstream here: littlefs-project/littlefs#1045.
littlefs already provide lfs_setattr and lfs_getattr which work on file path, the new functions (lfs_file_getattr and lfs_file_setattr) which work on file handle is a natural extension. Let's wait the author feedback.

it's controversial if it's a good idea to waste on-disk structures for attributes like file modes and uid/gid, which nuttx doesn't really support.

uid/gid is useful after:
#8924
#10119
#10176
apache/nuttx-apps#1691
This is the first real filesystem supported uid/gid/mode on NuttX, so it's better to keep it to demonstrate this capability.

BTW, the attribute contains the date/time information which is very useful for most people.

yamt · 2024-11-18T02:11:54Z

@xiaoxiang781216 i'm not sure which changes you are talking about. can you explain a bit?
anyway, i guess the original PR should not have contained unrelated changes together. i suspect it's simpler to revert the PR as a whole and ask the author to re-submit those parts separately.

Ok, let's me revert.

@yamt After comparing carefully all nuttx and littlefs patch, I think the change is good in general. The follow is my answer to your concern.

Reverts #13964
because:

it seems to introduce some regressions. i saw fstat failing with ENOENT on files on an image generated with https://github.com/jrast/littlefs-python.

Could you give a repro step? So, we can provide a fix.

you can reproduce it with the instructions in https://nuttx.apache.org/docs/latest/platforms/xtensa/esp32s3/boards/esp32s3-devkit/index.html#toywasm.
(with an update #14832)

we should reduce the amount of the local patches. not the opposite. if you want to extend littlefs, please upstream it first.

It upstream here: littlefs-project/littlefs#1045. littlefs already provide lfs_setattr and lfs_getattr which work on file path, the new functions (lfs_file_getattr and lfs_file_setattr) which work on file handle is a natural extension. Let's wait the author feedback.

it's controversial if it's a good idea to waste on-disk structures for attributes like file modes and uid/gid, which nuttx doesn't really support.

uid/gid is useful after: #8924 #10119 #10176 apache/nuttx-apps#1691 This is the first real filesystem supported uid/gid/mode on NuttX, so it's better to keep it to demonstrate this capability.

BTW, the attribute contains the date/time information which is very useful for most people.

i don't object much if:

the patch is accepted by the upstream. (it's important for me to be able to use latest version of littlefs.)
and it's optional. (i don't want to use my flash space for these attributes.)

xiaoxiang781216 · 2024-11-18T02:18:10Z

@xiaoxiang781216 i'm not sure which changes you are talking about. can you explain a bit?
anyway, i guess the original PR should not have contained unrelated changes together. i suspect it's simpler to revert the PR as a whole and ask the author to re-submit those parts separately.

Ok, let's me revert.

@yamt After comparing carefully all nuttx and littlefs patch, I think the change is good in general. The follow is my answer to your concern.

Reverts #13964
because:

it seems to introduce some regressions. i saw fstat failing with ENOENT on files on an image generated with https://github.com/jrast/littlefs-python.

Could you give a repro step? So, we can provide a fix.

you can reproduce it with the instructions in https://nuttx.apache.org/docs/latest/platforms/xtensa/esp32s3/boards/esp32s3-devkit/index.html#toywasm. (with an update #14832)

@crafcat7 please find the root cause why the format fail with attribute patch.

we should reduce the amount of the local patches. not the opposite. if you want to extend littlefs, please upstream it first.

It upstream here: littlefs-project/littlefs#1045. littlefs already provide lfs_setattr and lfs_getattr which work on file path, the new functions (lfs_file_getattr and lfs_file_setattr) which work on file handle is a natural extension. Let's wait the author feedback.

it's controversial if it's a good idea to waste on-disk structures for attributes like file modes and uid/gid, which nuttx doesn't really support.

uid/gid is useful after: #8924 #10119 #10176 apache/nuttx-apps#1691 This is the first real filesystem supported uid/gid/mode on NuttX, so it's better to keep it to demonstrate this capability.
BTW, the attribute contains the date/time information which is very useful for most people.

i don't object much if:

the patch is accepted by the upstream. (it's important for me to be able to use latest version of littlefs.)

the patch already upstream, let's wait the feedback.

and it's optional. (i don't want to use my flash space for these attributes.)

Ok, @crafcat7 please add an option to skip the attribute operation.

crafcat7 · 2024-11-18T07:22:45Z

@xiaoxiang781216 i'm not sure which changes you are talking about. can you explain a bit?
anyway, i guess the original PR should not have contained unrelated changes together. i suspect it's simpler to revert the PR as a whole and ask the author to re-submit those parts separately.

Ok, let's me revert.

@yamt After comparing carefully all nuttx and littlefs patch, I think the change is good in general. The follow is my answer to your concern.

Reverts #13964
because:

it seems to introduce some regressions. i saw fstat failing with ENOENT on files on an image generated with https://github.com/jrast/littlefs-python.

Could you give a repro step? So, we can provide a fix.

you can reproduce it with the instructions in https://nuttx.apache.org/docs/latest/platforms/xtensa/esp32s3/boards/esp32s3-devkit/index.html#toywasm. (with an update #14832)

@crafcat7 please find the root cause why the format fail with attribute patch.

The reason for the fstat failure should be caused during mkfs.
Because the get / set_attr function is introduced in the littlefs currently used in NuttX, we need to write the attributes of the original data into the actual mkfs through lfs_setattr.
Based on https://github.com/whitecatboard/Lua-RTOS-ESP32/blob/master/components/mklfs/src/mklfs.c
It can be implemented as follows

static void create_file(char *src) {
    char *path;
    int ret;

    path = strchr(src, '/');
    if (path) {
        fprintf(stdout, "%s\r\n", path);

        // Open source file
        FILE *srcf = fopen(src,"rb");
        if (!srcf) {
            fprintf(stderr,"can't open source file %s: errno=%d (%s)\r\n", src, errno, strerror(errno));
            exit(1);
        }

        // Get source file fd
        int fd = fileno(srcf);

        // Get file metadata
        struct stat filestat;
        if (fstat(fd, &filestat) == -1) {
            fprintf(stderr,"can't get source file %s: errno=%d (%s)\r\n", src, errno, strerror(errno));
            fclose(srcf);
            exit(1);
        }

        // Open destination file
        lfs_file_t dstf;
        if ((ret = lfs_file_open(&lfs, &dstf, path, LFS_O_WRONLY | LFS_O_CREAT)) < 0) {
            fprintf(stderr,"can't open destination file %s: error=%d\r\n", path, ret);
            exit(1);
        }

		char c = fgetc(srcf);
		while (!feof(srcf)) {
			ret = lfs_file_write(&lfs, &dstf, &c, 1);
			if (ret < 0) {
				fprintf(stderr,"can't write to destination file %s: error=%d\r\n", path, ret);
				exit(1);
			}
			c = fgetc(srcf);
		}

    // Attributes of written data
    struct lfs_stat attr;
    memset(&attr, 0, sizeof(attr));

    attr.at_atim = filestat.st_atime;
    attr.at_mtim = filestat.st_mtime;
    attr.at_ctim = filestat.st_ctime;
    attr.at_gid  = filestat.st_gid;
    attr.at_uid  = filestat.st_uid;
    attr.at_mode = filestat.st_mode;
    attr.at_size = filestat.st_size;

    ret = lfs_setattr(&lfs, path, 0, &attr, sizeof(attr));
    if (ret < 0) {
      fprintf(stderr,"can't set attr to destination file %s: error=%d\r\n", path, ret);
      exit(1);
    }

    // Close destination file
		ret = lfs_file_close(&lfs, &dstf);
		if (ret < 0) {
			fprintf(stderr,"can't close destination file %s: error=%d\r\n", path, ret);
			exit(1);
		}

        // Close source file
        fclose(srcf);
    }
}

And I will submit a compatibility change that is when lfs_file_getattr returns LFS_ERR_NOENT, just return 0 (ignore it), so there will be no problem. The file size is obtained through lfs_file_size, so the file size is not affected

we should reduce the amount of the local patches. not the opposite. if you want to extend littlefs, please upstream it first.

It upstream here: littlefs-project/littlefs#1045. littlefs already provide lfs_setattr and lfs_getattr which work on file path, the new functions (lfs_file_getattr and lfs_file_setattr) which work on file handle is a natural extension. Let's wait the author feedback.

it's controversial if it's a good idea to waste on-disk structures for attributes like file modes and uid/gid, which nuttx doesn't really support.

uid/gid is useful after: #8924 #10119 #10176 apache/nuttx-apps#1691 This is the first real filesystem supported uid/gid/mode on NuttX, so it's better to keep it to demonstrate this capability.
BTW, the attribute contains the date/time information which is very useful for most people.

i don't object much if:

the patch is accepted by the upstream. (it's important for me to be able to use latest version of littlefs.)

the patch already upstream, let's wait the feedback.

and it's optional. (i don't want to use my flash space for these attributes.)

Ok, @crafcat7 please add an option to skip the attribute operation.

Likewise, I will submit a change later to turn off the property setting.

xiaoxiang781216 · 2024-11-18T07:42:51Z

@crafcat7 let's port https://github.com/whitecatboard/Lua-RTOS-ESP32/blob/master/components/mklfs/src/mklfs.c to apps/fsutils and add a command line option to generate the compatible attr entry we expect.

crafcat7 · 2024-11-19T03:15:25Z

@crafcat7 let's port https://github.com/whitecatboard/Lua-RTOS-ESP32/blob/master/components/mklfs/src/mklfs.c to apps/fsutils and add a command line option to generate the compatible attr entry we expect.

I will then submit a PR under apps to complete it

crafcat7 · 2024-11-19T03:16:09Z

@xiaoxiang781216 i'm not sure which changes you are talking about. can you explain a bit?
anyway, i guess the original PR should not have contained unrelated changes together. i suspect it's simpler to revert the PR as a whole and ask the author to re-submit those parts separately.

Ok, let's me revert.

@yamt After comparing carefully all nuttx and littlefs patch, I think the change is good in general. The follow is my answer to your concern.

Reverts #13964
because:

it seems to introduce some regressions. i saw fstat failing with ENOENT on files on an image generated with https://github.com/jrast/littlefs-python.

Could you give a repro step? So, we can provide a fix.

you can reproduce it with the instructions in https://nuttx.apache.org/docs/latest/platforms/xtensa/esp32s3/boards/esp32s3-devkit/index.html#toywasm. (with an update #14832)

@crafcat7 please find the root cause why the format fail with attribute patch.

The reason for the fstat failure should be caused during mkfs. Because the get / set_attr function is introduced in the littlefs currently used in NuttX, we need to write the attributes of the original data into the actual mkfs through lfs_setattr. Based on https://github.com/whitecatboard/Lua-RTOS-ESP32/blob/master/components/mklfs/src/mklfs.c It can be implemented as follows
static void create_file(char *src) {
    char *path;
    int ret;

    path = strchr(src, '/');
    if (path) {
        fprintf(stdout, "%s\r\n", path);

        // Open source file
        FILE *srcf = fopen(src,"rb");
        if (!srcf) {
            fprintf(stderr,"can't open source file %s: errno=%d (%s)\r\n", src, errno, strerror(errno));
            exit(1);
        }

        // Get source file fd
        int fd = fileno(srcf);

        // Get file metadata
        struct stat filestat;
        if (fstat(fd, &filestat) == -1) {
            fprintf(stderr,"can't get source file %s: errno=%d (%s)\r\n", src, errno, strerror(errno));
            fclose(srcf);
            exit(1);
        }

        // Open destination file
        lfs_file_t dstf;
        if ((ret = lfs_file_open(&lfs, &dstf, path, LFS_O_WRONLY | LFS_O_CREAT)) < 0) {
            fprintf(stderr,"can't open destination file %s: error=%d\r\n", path, ret);
            exit(1);
        }

		char c = fgetc(srcf);
		while (!feof(srcf)) {
			ret = lfs_file_write(&lfs, &dstf, &c, 1);
			if (ret < 0) {
				fprintf(stderr,"can't write to destination file %s: error=%d\r\n", path, ret);
				exit(1);
			}
			c = fgetc(srcf);
		}

    // Attributes of written data
    struct lfs_stat attr;
    memset(&attr, 0, sizeof(attr));

    attr.at_atim = filestat.st_atime;
    attr.at_mtim = filestat.st_mtime;
    attr.at_ctim = filestat.st_ctime;
    attr.at_gid  = filestat.st_gid;
    attr.at_uid  = filestat.st_uid;
    attr.at_mode = filestat.st_mode;
    attr.at_size = filestat.st_size;

    ret = lfs_setattr(&lfs, path, 0, &attr, sizeof(attr));
    if (ret < 0) {
      fprintf(stderr,"can't set attr to destination file %s: error=%d\r\n", path, ret);
      exit(1);
    }

    // Close destination file
		ret = lfs_file_close(&lfs, &dstf);
		if (ret < 0) {
			fprintf(stderr,"can't close destination file %s: error=%d\r\n", path, ret);
			exit(1);
		}

        // Close source file
        fclose(srcf);
    }
}
And I will submit a compatibility change that is when lfs_file_getattr returns LFS_ERR_NOENT, just return 0 (ignore it), so there will be no problem. The file size is obtained through lfs_file_size, so the file size is not affected

we should reduce the amount of the local patches. not the opposite. if you want to extend littlefs, please upstream it first.

It upstream here: littlefs-project/littlefs#1045. littlefs already provide lfs_setattr and lfs_getattr which work on file path, the new functions (lfs_file_getattr and lfs_file_setattr) which work on file handle is a natural extension. Let's wait the author feedback.

it's controversial if it's a good idea to waste on-disk structures for attributes like file modes and uid/gid, which nuttx doesn't really support.

uid/gid is useful after: #8924 #10119 #10176 apache/nuttx-apps#1691 This is the first real filesystem supported uid/gid/mode on NuttX, so it's better to keep it to demonstrate this capability.
BTW, the attribute contains the date/time information which is very useful for most people.

i don't object much if:

the patch is accepted by the upstream. (it's important for me to be able to use latest version of littlefs.)

the patch already upstream, let's wait the feedback.

and it's optional. (i don't want to use my flash space for these attributes.)

Ok, @crafcat7 please add an option to skip the attribute operation.

Likewise, I will submit a change later to turn off the property setting.

I submitted this compatibility fix at #14844

xiaoxiang781216 · 2024-11-19T14:40:41Z

@yamt could you abandon this pr since #14844 get merged.

yamt · 2024-11-20T07:22:53Z

@yamt could you abandon this pr since #14844 get merged.

as your PR (littlefs-project/littlefs#1045) doesn't seem being accepted by the upstream, i guess we should still merge this revert.

yamt added 7 commits October 23, 2024 18:24

Revert "littlefs:remove the '/' in the end of relpath in mkdir"

0d30be4

This reverts commit 69f3774.

Revert "littlefs: fix warnings"

8f9efd5

This reverts commit da5839c.

Revert "fs/littlefs: revert fstat and use lfs_file_attr function"

72ed32b

This reverts commit 8f6ea88.

Revert "littlefs/stat: return OK when stat success"

74be149

This reverts commit c577929.

Revert "fs/littlefs: Add attr to dirA"

d43247d

This reverts commit 0ab637f.

Revert "fs/littlefs: Fix the bug of missing file types"

dc81889

This reverts commit f70de1c.

Revert "fs/littlefs: Implement fchstat and chstat of littlefs"

708181e

This reverts commit 44992c8.

github-actions bot added Area: File System File System issues Size: M The size of the change in this PR is medium labels Oct 23, 2024

yamt marked this pull request as ready for review October 23, 2024 09:32

acassis requested a review from pkarashchenko October 25, 2024 16:20

xiaoxiang781216 reviewed Nov 1, 2024

View reviewed changes

yamt mentioned this pull request Nov 1, 2024

file_readv_compat/file_writev_compat: Fix partial success handling #14597

Merged

crafcat7 mentioned this pull request Nov 19, 2024

Support enabling/disabling attr attribute in littlefs for nuttx #14844

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Revert "Add support fchstat and chstat function for littlefs" #14479

Revert "Add support fchstat and chstat function for littlefs" #14479

yamt commented Oct 23, 2024 •

edited

Loading

nuttxpr commented Oct 23, 2024

xiaoxiang781216 commented Oct 24, 2024

yamt commented Oct 24, 2024

xiaoxiang781216 Nov 1, 2024

xiaoxiang781216 Nov 1, 2024

yamt commented Nov 1, 2024

xiaoxiang781216 commented Nov 1, 2024

yamt commented Nov 13, 2024

xiaoxiang781216 commented Nov 13, 2024

xiaoxiang781216 commented Nov 16, 2024

yamt commented Nov 18, 2024

xiaoxiang781216 commented Nov 18, 2024

crafcat7 commented Nov 18, 2024 •

edited

Loading

xiaoxiang781216 commented Nov 18, 2024 •

edited

Loading

crafcat7 commented Nov 19, 2024

crafcat7 commented Nov 19, 2024

xiaoxiang781216 commented Nov 19, 2024

yamt commented Nov 20, 2024

Revert "Add support fchstat and chstat function for littlefs" #14479

Are you sure you want to change the base?

Revert "Add support fchstat and chstat function for littlefs" #14479

Conversation

yamt commented Oct 23, 2024 • edited Loading

nuttxpr commented Oct 23, 2024

xiaoxiang781216 commented Oct 24, 2024

yamt commented Oct 24, 2024

xiaoxiang781216 Nov 1, 2024

Choose a reason for hiding this comment

xiaoxiang781216 Nov 1, 2024

Choose a reason for hiding this comment

yamt commented Nov 1, 2024

xiaoxiang781216 commented Nov 1, 2024

yamt commented Nov 13, 2024

xiaoxiang781216 commented Nov 13, 2024

xiaoxiang781216 commented Nov 16, 2024

yamt commented Nov 18, 2024

xiaoxiang781216 commented Nov 18, 2024

crafcat7 commented Nov 18, 2024 • edited Loading

xiaoxiang781216 commented Nov 18, 2024 • edited Loading

crafcat7 commented Nov 19, 2024

crafcat7 commented Nov 19, 2024

xiaoxiang781216 commented Nov 19, 2024

yamt commented Nov 20, 2024

yamt commented Oct 23, 2024 •

edited

Loading

crafcat7 commented Nov 18, 2024 •

edited

Loading

xiaoxiang781216 commented Nov 18, 2024 •

edited

Loading