crafcat7 commented on PR #14479: URL: https://github.com/apache/nuttx/pull/14479#issuecomment-2484620894
> > > > > > @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](https://github.com/littlefs-project/littlefs/pull/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](https://github.com/apache/nuttx-apps/pull/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 https://github.com/apache/nuttx/pull/14844 -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
