| Message ID | 20241026064048.370062-1-heinrich.schuchardt@canonical.com |
|---|---|
| Headers | show |
| Series | fs: ext4: implement opendir, readdir, closedir | expand |
On Sat, 26 Oct 2024 08:40:43 +0200, Heinrich Schuchardt wrote: > With this series opendir, readdir, closedir are implemented for ext4. > These functions are needed for the UEFI sub-system to interact with > the ext4 file system. > > To reduce code growth the functions are reused to implement the ls > command for ext4. > > [...] Applied to local tree/v2-tidy-test-dir, thanks!
Hi Tom On Sun, Nov 3, 2024 at 12:26 AM Tom Rini <trini@konsulko.com> wrote: > > On Sat, 26 Oct 2024 08:40:43 +0200, Heinrich Schuchardt wrote: > > > With this series opendir, readdir, closedir are implemented for ext4. > > These functions are needed for the UEFI sub-system to interact with > > the ext4 file system. > > > > To reduce code growth the functions are reused to implement the ls > > command for ext4. > > > > [...] > > Applied to local tree/v2-tidy-test-dir, thanks! > Am I sleeping? int ext4fs_opendir(const char *dirname, struct fs_dir_stream **dirsp) { struct ext4_dir_stream *dirs = NULL; struct ext2fs_node *dir = NULL; int ret; *dirsp = NULL; dirs = calloc(1, sizeof(struct ext4_dir_stream)); if (!dirs) return -ENOMEM; dirs->dirname = strdup(dirname); if (!dirs->dirname) { free(dirs); return -ENOMEM; } ret = ext4fs_find_file(dirname, &ext4fs_root->diropen, &dir, FILETYPE_DIRECTORY); if (ret == 1) { ret = 0; *dirsp = (struct fs_dir_stream *)dirs; } else { ret = -ENOENT; free(dirs->dirname); free(dirs); } if (dir) ext4fs_free_node(dir, &ext4fs_root->diropen); return ret; } Should not be like this? > -- > Tom > >
On Sun, Nov 03, 2024 at 12:36:38AM +0100, Michael Nazzareno Trimarchi wrote: > Hi Tom > > On Sun, Nov 3, 2024 at 12:26 AM Tom Rini <trini@konsulko.com> wrote: > > > > On Sat, 26 Oct 2024 08:40:43 +0200, Heinrich Schuchardt wrote: > > > > > With this series opendir, readdir, closedir are implemented for ext4. > > > These functions are needed for the UEFI sub-system to interact with > > > the ext4 file system. > > > > > > To reduce code growth the functions are reused to implement the ls > > > command for ext4. > > > > > > [...] > > > > Applied to local tree/v2-tidy-test-dir, thanks! > > > Am I sleeping? ... I always forget b4 compares with checked out branch, that should have been master. > int ext4fs_opendir(const char *dirname, struct fs_dir_stream **dirsp) { > struct ext4_dir_stream *dirs = NULL; > struct ext2fs_node *dir = NULL; > int ret; > > *dirsp = NULL; > > dirs = calloc(1, sizeof(struct ext4_dir_stream)); > if (!dirs) > return -ENOMEM; > > dirs->dirname = strdup(dirname); > if (!dirs->dirname) { > free(dirs); > return -ENOMEM; > } > > ret = ext4fs_find_file(dirname, &ext4fs_root->diropen, &dir, > FILETYPE_DIRECTORY); > > if (ret == 1) { > ret = 0; > *dirsp = (struct fs_dir_stream *)dirs; > } else { > ret = -ENOENT; > free(dirs->dirname); > free(dirs); > } > > if (dir) > ext4fs_free_node(dir, &ext4fs_root->diropen); > > return ret; > } > > Should not be like this? Please elaborate?
Hi Il dom 3 nov 2024, 00:53 Tom Rini <trini@konsulko.com> ha scritto: > On Sun, Nov 03, 2024 at 12:36:38AM +0100, Michael Nazzareno Trimarchi > wrote: > > Hi Tom > > > > On Sun, Nov 3, 2024 at 12:26 AM Tom Rini <trini@konsulko.com> wrote: > > > > > > On Sat, 26 Oct 2024 08:40:43 +0200, Heinrich Schuchardt wrote: > > > > > > > With this series opendir, readdir, closedir are implemented for ext4. > > > > These functions are needed for the UEFI sub-system to interact with > > > > the ext4 file system. > > > > > > > > To reduce code growth the functions are reused to implement the ls > > > > command for ext4. > > > > > > > > [...] > > > > > > Applied to local tree/v2-tidy-test-dir, thanks! > > > > > Am I sleeping? > > ... I always forget b4 compares with checked out branch, that should > have been master. > > > int ext4fs_opendir(const char *dirname, struct fs_dir_stream **dirsp) { > > struct ext4_dir_stream *dirs = NULL; > > struct ext2fs_node *dir = NULL; > > int ret; > > > > *dirsp = NULL; > > > > dirs = calloc(1, sizeof(struct ext4_dir_stream)); > > if (!dirs) > > return -ENOMEM; > > > > dirs->dirname = strdup(dirname); > > if (!dirs->dirname) { > > free(dirs); > > return -ENOMEM; > > } > > > > ret = ext4fs_find_file(dirname, &ext4fs_root->diropen, &dir, > > FILETYPE_DIRECTORY); > > > > if (ret == 1) { > > ret = 0; > > *dirsp = (struct fs_dir_stream *)dirs; > > } else { > > ret = -ENOENT; > > free(dirs->dirname); > > free(dirs); > I have add in this path the free of dirs, because according to the code I can see from this email is not reference anymore and anyway according to what I have commented already moving allocation of dirs later make the code a bit simpler. Michael > } > > > > if (dir) > > ext4fs_free_node(dir, &ext4fs_root->diropen); > > > > return ret; > > } > > > > Should not be like this? > > Please elaborate? > > -- > Tom >
Hi Tom On Sun, Nov 3, 2024 at 9:21 AM Michael Nazzareno Trimarchi <michael@amarulasolutions.com> wrote: > > Hi > > Il dom 3 nov 2024, 00:53 Tom Rini <trini@konsulko.com> ha scritto: >> >> On Sun, Nov 03, 2024 at 12:36:38AM +0100, Michael Nazzareno Trimarchi wrote: >> > Hi Tom >> > >> > On Sun, Nov 3, 2024 at 12:26 AM Tom Rini <trini@konsulko.com> wrote: >> > > >> > > On Sat, 26 Oct 2024 08:40:43 +0200, Heinrich Schuchardt wrote: >> > > >> > > > With this series opendir, readdir, closedir are implemented for ext4. >> > > > These functions are needed for the UEFI sub-system to interact with >> > > > the ext4 file system. >> > > > >> > > > To reduce code growth the functions are reused to implement the ls >> > > > command for ext4. >> > > > >> > > > [...] >> > > >> > > Applied to local tree/v2-tidy-test-dir, thanks! >> > > >> > Am I sleeping? >> >> ... I always forget b4 compares with checked out branch, that should >> have been master. >> >> > int ext4fs_opendir(const char *dirname, struct fs_dir_stream **dirsp) { >> > struct ext4_dir_stream *dirs = NULL; >> > struct ext2fs_node *dir = NULL; >> > int ret; >> > >> > *dirsp = NULL; >> > >> > dirs = calloc(1, sizeof(struct ext4_dir_stream)); >> > if (!dirs) >> > return -ENOMEM; >> > >> > dirs->dirname = strdup(dirname); >> > if (!dirs->dirname) { >> > free(dirs); >> > return -ENOMEM; >> > } >> > >> > ret = ext4fs_find_file(dirname, &ext4fs_root->diropen, &dir, >> > FILETYPE_DIRECTORY); >> > >> > if (ret == 1) { >> > ret = 0; >> > *dirsp = (struct fs_dir_stream *)dirs; >> > } else { >> > ret = -ENOENT; >> > free(dirs->dirname); >> > free(dirs); > > > I have add in this path the free of dirs, because according to the code I can see from this email is not reference anymore and anyway according to what I have commented already moving allocation of dirs later make the code a bit simpler. > There are few leaks in this function. I have fixed, can you take a look again on what you merge? Michael > Michael > > >> > } >> > >> > if (dir) >> > ext4fs_free_node(dir, &ext4fs_root->diropen); >> > >> > return ret; >> > } >> > >> > Should not be like this? >> >> Please elaborate? >> >> -- >> Tom
On 11/3/24 00:36, Michael Nazzareno Trimarchi wrote: > Hi Tom > > On Sun, Nov 3, 2024 at 12:26 AM Tom Rini <trini@konsulko.com> wrote: >> >> On Sat, 26 Oct 2024 08:40:43 +0200, Heinrich Schuchardt wrote: >> >>> With this series opendir, readdir, closedir are implemented for ext4. >>> These functions are needed for the UEFI sub-system to interact with >>> the ext4 file system. >>> >>> To reduce code growth the functions are reused to implement the ls >>> command for ext4. >>> >>> [...] >> >> Applied to local tree/v2-tidy-test-dir, thanks! >> > Am I sleeping? > > int ext4fs_opendir(const char *dirname, struct fs_dir_stream **dirsp) { > struct ext4_dir_stream *dirs = NULL; Thank you for your review. We should not set a value that we do not use. > struct ext2fs_node *dir = NULL; > int ret; > > *dirsp = NULL; > > dirs = calloc(1, sizeof(struct ext4_dir_stream)); > if (!dirs) > return -ENOMEM; > > dirs->dirname = strdup(dirname); > if (!dirs->dirname) { Yes this line in my code is incorrect. > free(dirs); > return -ENOMEM; > } > > ret = ext4fs_find_file(dirname, &ext4fs_root->diropen, &dir, > FILETYPE_DIRECTORY); > > if (ret == 1) { > ret = 0; > *dirsp = (struct fs_dir_stream *)dirs; > } else { > ret = -ENOENT; > free(dirs->dirname); > free(dirs); Yes, we should avoid a memory leak here. I will send a patch. Best regards Heinrich > } > > if (dir) > ext4fs_free_node(dir, &ext4fs_root->diropen); > > return ret; > } > > Should not be like this? > >> -- >> Tom >> >> > >
Hi On Thu, Nov 7, 2024 at 10:27 AM Heinrich Schuchardt <xypron.glpk@gmx.de> wrote: > > On 11/3/24 00:36, Michael Nazzareno Trimarchi wrote: > > Hi Tom > > > > On Sun, Nov 3, 2024 at 12:26 AM Tom Rini <trini@konsulko.com> wrote: > >> > >> On Sat, 26 Oct 2024 08:40:43 +0200, Heinrich Schuchardt wrote: > >> > >>> With this series opendir, readdir, closedir are implemented for ext4. > >>> These functions are needed for the UEFI sub-system to interact with > >>> the ext4 file system. > >>> > >>> To reduce code growth the functions are reused to implement the ls > >>> command for ext4. > >>> > >>> [...] > >> > >> Applied to local tree/v2-tidy-test-dir, thanks! > >> > > Am I sleeping? > > > > int ext4fs_opendir(const char *dirname, struct fs_dir_stream **dirsp) { > > struct ext4_dir_stream *dirs = NULL; > > Thank you for your review. > > We should not set a value that we do not use. > > > struct ext2fs_node *dir = NULL; > > int ret; > > > > *dirsp = NULL; > > > > dirs = calloc(1, sizeof(struct ext4_dir_stream)); > > if (!dirs) > > return -ENOMEM; > > > > dirs->dirname = strdup(dirname); > > if (!dirs->dirname) { > > Yes this line in my code is incorrect. > We need to use valgrind in unit test or we need some code analys to automate this process. We are always looking for memory leaks in uboot in exit path or we need to have alloction funciton that automatic free the memory when we go in error path Michael > > free(dirs); > > return -ENOMEM; > > } > > > > ret = ext4fs_find_file(dirname, &ext4fs_root->diropen, &dir, > > FILETYPE_DIRECTORY); > > > > if (ret == 1) { > > ret = 0; > > *dirsp = (struct fs_dir_stream *)dirs; > > } else { > > ret = -ENOENT; > > free(dirs->dirname); > > free(dirs); > > Yes, we should avoid a memory leak here. > > I will send a patch. > > Best regards > > Heinrich > > > } > > > > if (dir) > > ext4fs_free_node(dir, &ext4fs_root->diropen); > > > > return ret; > > } > > > > Should not be like this? > > > >> -- > >> Tom > >> > >> > > > > >
On 11/7/24 10:31, Michael Nazzareno Trimarchi wrote: > Hi > > On Thu, Nov 7, 2024 at 10:27 AM Heinrich Schuchardt <xypron.glpk@gmx.de> wrote: >> >> On 11/3/24 00:36, Michael Nazzareno Trimarchi wrote: >>> Hi Tom >>> >>> On Sun, Nov 3, 2024 at 12:26 AM Tom Rini <trini@konsulko.com> wrote: >>>> >>>> On Sat, 26 Oct 2024 08:40:43 +0200, Heinrich Schuchardt wrote: >>>> >>>>> With this series opendir, readdir, closedir are implemented for ext4. >>>>> These functions are needed for the UEFI sub-system to interact with >>>>> the ext4 file system. >>>>> >>>>> To reduce code growth the functions are reused to implement the ls >>>>> command for ext4. >>>>> >>>>> [...] >>>> >>>> Applied to local tree/v2-tidy-test-dir, thanks! >>>> >>> Am I sleeping? >>> >>> int ext4fs_opendir(const char *dirname, struct fs_dir_stream **dirsp) { >>> struct ext4_dir_stream *dirs = NULL; >> >> Thank you for your review. >> >> We should not set a value that we do not use. >> >>> struct ext2fs_node *dir = NULL; >>> int ret; >>> >>> *dirsp = NULL; >>> >>> dirs = calloc(1, sizeof(struct ext4_dir_stream)); >>> if (!dirs) >>> return -ENOMEM; >>> >>> dirs->dirname = strdup(dirname); >>> if (!dirs->dirname) { >> >> Yes this line in my code is incorrect. >> > > We need to use valgrind in unit test or we need some code analys to > automate this process. > We are always looking for memory leaks in uboot in exit path or we > need to have alloction funciton that > automatic free the memory when we go in error path Coverity probably would have reported this. But I think we only run it on tags. This article describes how Valgrind could be used: https://developers.redhat.com/articles/2022/03/23/use-valgrind-memcheck-custom-memory-manager Both malloc and LMB would be candidates. Best regards Heinrich > > Michael > >>> free(dirs); >>> return -ENOMEM; >>> } >>> >>> ret = ext4fs_find_file(dirname, &ext4fs_root->diropen, &dir, >>> FILETYPE_DIRECTORY); >>> >>> if (ret == 1) { >>> ret = 0; >>> *dirsp = (struct fs_dir_stream *)dirs; >>> } else { >>> ret = -ENOENT; >>> free(dirs->dirname); >>> free(dirs); >> >> Yes, we should avoid a memory leak here. >> >> I will send a patch. >> >> Best regards >> >> Heinrich >> >>> } >>> >>> if (dir) >>> ext4fs_free_node(dir, &ext4fs_root->diropen); >>> >>> return ret; >>> } >>> >>> Should not be like this? >>> >>>> -- >>>> Tom >>>> >>>> >>> >>> >> > >