| Message ID | 20180904074948.18146-5-takahiro.akashi@linaro.org |
|---|---|
| State | Changes Requested |
| Delegated to: | Tom Rini |
| Headers | show |
| Series | subject: fs: fat: extend FAT write operations | expand |
On 04.09.18 09:49, AKASHI Takahiro wrote: > Directory iterator was introduced in major re-work of read operation by > Rob. We want to use it for write operation extensively as well. Please indicate in the commit message that write operations are implemented in a different .c file and so we have to export the respective functions. Alex
On 09/04/2018 11:01 AM, Alexander Graf wrote: > > > On 04.09.18 09:49, AKASHI Takahiro wrote: >> Directory iterator was introduced in major re-work of read operation by >> Rob. We want to use it for write operation extensively as well. > > Please indicate in the commit message that write operations are > implemented in a different .c file and so we have to export the > respective functions. Why? Look at this ugly code: fs/fat/fat_write.c:17:#include "fat.c" Best regards Heinrich > > Alex >
> Am 04.09.2018 um 12:50 schrieb Heinrich Schuchardt <xypron.glpk@gmx.de>: > > > >> On 09/04/2018 11:01 AM, Alexander Graf wrote: >> >> >>> On 04.09.18 09:49, AKASHI Takahiro wrote: >>> Directory iterator was introduced in major re-work of read operation by >>> Rob. We want to use it for write operation extensively as well. >> >> Please indicate in the commit message that write operations are >> implemented in a different .c file and so we have to export the >> respective functions. > > Why? Look at this ugly code: > > fs/fat/fat_write.c:17:#include "fat.c" In that case we don't need this patch at all, no? Alex > > Best regards > > Heinrich > >> >> Alex >>
On Tue, Sep 04, 2018 at 12:57:54PM +0200, Alexander Graf wrote: > > > > Am 04.09.2018 um 12:50 schrieb Heinrich Schuchardt <xypron.glpk@gmx.de>: > > > > > > > >> On 09/04/2018 11:01 AM, Alexander Graf wrote: > >> > >> > >>> On 04.09.18 09:49, AKASHI Takahiro wrote: > >>> Directory iterator was introduced in major re-work of read operation by > >>> Rob. We want to use it for write operation extensively as well. > >> > >> Please indicate in the commit message that write operations are > >> implemented in a different .c file and so we have to export the > >> respective functions. > > > > Why? Look at this ugly code: > > > > fs/fat/fat_write.c:17:#include "fat.c" > > In that case we don't need this patch at all, no? Oops, I didn't notice this before. If, however, "include fat.c" makes any sense, theoretically we don't need "depends on FS_FAT" for FS_FAT_WRITE. There seems to be a contradiction between the code and config. I prefer just to remove the line, '#include "fat.c"' from fat_write.c and add more "extern" definitions in fat.h if necessary. Thanks, -Takakahiro AKASHI > Alex > > > > > Best regards > > > > Heinrich > > > >> > >> Alex > >>
On 05.09.18 04:14, AKASHI Takahiro wrote: > On Tue, Sep 04, 2018 at 12:57:54PM +0200, Alexander Graf wrote: >> >> >>> Am 04.09.2018 um 12:50 schrieb Heinrich Schuchardt <xypron.glpk@gmx.de>: >>> >>> >>> >>>> On 09/04/2018 11:01 AM, Alexander Graf wrote: >>>> >>>> >>>>> On 04.09.18 09:49, AKASHI Takahiro wrote: >>>>> Directory iterator was introduced in major re-work of read operation by >>>>> Rob. We want to use it for write operation extensively as well. >>>> >>>> Please indicate in the commit message that write operations are >>>> implemented in a different .c file and so we have to export the >>>> respective functions. >>> >>> Why? Look at this ugly code: >>> >>> fs/fat/fat_write.c:17:#include "fat.c" >> >> In that case we don't need this patch at all, no? > > Oops, I didn't notice this before. > If, however, "include fat.c" makes any sense, theoretically we don't need > "depends on FS_FAT" for FS_FAT_WRITE. > There seems to be a contradiction between the code and config. I'm not sure it's contradictory. Someone just implemented a poor man's LTO by including the 2 files with each other. > I prefer just to remove the line, '#include "fat.c"' from fat_write.c > and add more "extern" definitions in fat.h if necessary. We can worry about refactoring things later down the road. By making functions go external, we lose the compiler's ability to inline functions which may again bloat code which may trip random boards above size limits. For now, I'd suggest you leave the ugly #include "fat.c" in and not move anything into fat.h. That way the file is self-contained and you don't have to worry about exporting just yet. Alex
On Wed, Sep 05, 2018 at 10:16:32AM +0200, Alexander Graf wrote: > > > On 05.09.18 04:14, AKASHI Takahiro wrote: > > On Tue, Sep 04, 2018 at 12:57:54PM +0200, Alexander Graf wrote: > >> > >> > >>> Am 04.09.2018 um 12:50 schrieb Heinrich Schuchardt <xypron.glpk@gmx.de>: > >>> > >>> > >>> > >>>> On 09/04/2018 11:01 AM, Alexander Graf wrote: > >>>> > >>>> > >>>>> On 04.09.18 09:49, AKASHI Takahiro wrote: > >>>>> Directory iterator was introduced in major re-work of read operation by > >>>>> Rob. We want to use it for write operation extensively as well. > >>>> > >>>> Please indicate in the commit message that write operations are > >>>> implemented in a different .c file and so we have to export the > >>>> respective functions. > >>> > >>> Why? Look at this ugly code: > >>> > >>> fs/fat/fat_write.c:17:#include "fat.c" > >> > >> In that case we don't need this patch at all, no? > > > > Oops, I didn't notice this before. > > If, however, "include fat.c" makes any sense, theoretically we don't need > > "depends on FS_FAT" for FS_FAT_WRITE. > > There seems to be a contradiction between the code and config. > > I'm not sure it's contradictory. Someone just implemented a poor man's > LTO by including the 2 files with each other. Well, LTO has long existed since gcc4.8 ... (and improvement would be quite small, I guess.) > > I prefer just to remove the line, '#include "fat.c"' from fat_write.c > > and add more "extern" definitions in fat.h if necessary. > > We can worry about refactoring things later down the road. By making > functions go external, we lose the compiler's ability to inline > functions which may again bloat code which may trip random boards above > size limits. > > For now, I'd suggest you leave the ugly #include "fat.c" in and not move > anything into fat.h. That way the file is self-contained and you don't > have to worry about exporting just yet. Although I won't tolerate such an ugly code, I will follow your suggestion mainly because lots of existing defconfigs enable FAT_WRITE without FS_FAT. Thanks, -Takahiro AKASHI > > Alex
diff --git a/fs/fat/fat.c b/fs/fat/fat.c index eaea9300fd7f..0574af0c0011 100644 --- a/fs/fat/fat.c +++ b/fs/fat/fat.c @@ -638,25 +638,6 @@ static int get_fs_info(fsdata *mydata) * For more complete example, see fat_itr_resolve() */ -typedef struct { - fsdata *fsdata; /* filesystem parameters */ - unsigned clust; /* current cluster */ - int last_cluster; /* set once we've read last cluster */ - int is_root; /* is iterator at root directory */ - int remaining; /* remaining dent's in current cluster */ - - /* current iterator position values: */ - dir_entry *dent; /* current directory entry */ - char l_name[VFAT_MAXLEN_BYTES]; /* long (vfat) name */ - char s_name[14]; /* short 8.3 name */ - char *name; /* l_name if there is one, else s_name */ - - /* storage for current cluster in memory: */ - u8 block[MAX_CLUSTSIZE] __aligned(ARCH_DMA_MINALIGN); -} fat_itr; - -static int fat_itr_isdir(fat_itr *itr); - /** * fat_itr_root() - initialize an iterator to start at the root * directory @@ -665,7 +646,7 @@ static int fat_itr_isdir(fat_itr *itr); * @fsdata: filesystem data for the partition * @return 0 on success, else -errno */ -static int fat_itr_root(fat_itr *itr, fsdata *fsdata) +int fat_itr_root(fat_itr *itr, fsdata *fsdata) { if (get_fs_info(fsdata)) return -ENXIO; @@ -697,7 +678,7 @@ static int fat_itr_root(fat_itr *itr, fsdata *fsdata) * @parent: the iterator pointing at a directory entry in the * parent directory of the directory to iterate */ -static void fat_itr_child(fat_itr *itr, fat_itr *parent) +void fat_itr_child(fat_itr *itr, fat_itr *parent) { fsdata *mydata = parent->fsdata; /* for silly macros */ unsigned clustnum = START(parent->dent); @@ -717,7 +698,7 @@ static void fat_itr_child(fat_itr *itr, fat_itr *parent) itr->last_cluster = 0; } -static void *next_cluster(fat_itr *itr) +void *next_cluster(fat_itr *itr) { fsdata *mydata = itr->fsdata; /* for silly macros */ int ret; @@ -838,7 +819,7 @@ static dir_entry *extract_vfat_name(fat_itr *itr) * @return boolean, 1 if success or 0 if no more entries in the * current directory */ -static int fat_itr_next(fat_itr *itr) +int fat_itr_next(fat_itr *itr) { dir_entry *dent; @@ -883,19 +864,11 @@ static int fat_itr_next(fat_itr *itr) * @itr: the iterator * @return true if cursor is at a directory */ -static int fat_itr_isdir(fat_itr *itr) +int fat_itr_isdir(fat_itr *itr) { return !!(itr->dent->attr & ATTR_DIR); } -/* - * Helpers: - */ - -#define TYPE_FILE 0x1 -#define TYPE_DIR 0x2 -#define TYPE_ANY (TYPE_FILE | TYPE_DIR) - /** * fat_itr_resolve() - traverse directory structure to resolve the * requested path. @@ -911,7 +884,7 @@ static int fat_itr_isdir(fat_itr *itr) * @type: bitmask of allowable file types * @return 0 on success or -errno */ -static int fat_itr_resolve(fat_itr *itr, const char *path, unsigned type) +int fat_itr_resolve(fat_itr *itr, const char *path, unsigned type) { const char *next; diff --git a/include/fat.h b/include/fat.h index 127e6622a9b0..d86eb5a11576 100644 --- a/include/fat.h +++ b/include/fat.h @@ -189,6 +189,38 @@ static inline u32 sect_to_clust(fsdata *fsdata, int sect) return (sect - fsdata->data_begin) / fsdata->clust_size; } +/* + * Directory iterator + */ + +#define TYPE_FILE 0x1 +#define TYPE_DIR 0x2 +#define TYPE_ANY (TYPE_FILE | TYPE_DIR) + +typedef struct { + fsdata *fsdata; /* filesystem parameters */ + unsigned clust; /* current cluster */ + int last_cluster; /* set once we've read last cluster */ + int is_root; /* is iterator at root directory */ + int remaining; /* remaining dent's in current cluster */ + + /* current iterator position values: */ + dir_entry *dent; /* current directory entry */ + char l_name[VFAT_MAXLEN_BYTES]; /* long (vfat) name */ + char s_name[14]; /* short 8.3 name */ + char *name; /* l_name if there is one, else s_name */ + + /* storage for current cluster in memory: */ + u8 block[MAX_CLUSTSIZE] __aligned(ARCH_DMA_MINALIGN); +} fat_itr; + +int fat_itr_root(fat_itr *itr, fsdata *fsdata); +void fat_itr_child(fat_itr *itr, fat_itr *parent); +void *next_cluster(fat_itr *itr); +int fat_itr_next(fat_itr *itr); +int fat_itr_isdir(fat_itr *itr); +int fat_itr_resolve(fat_itr *itr, const char *path, unsigned type); + int file_fat_detectfs(void); int fat_exists(const char *filename); int fat_size(const char *filename, loff_t *size);
Directory iterator was introduced in major re-work of read operation by Rob. We want to use it for write operation extensively as well. This patch makes relevant functions, as well as iterator definition, visible outside of fat.c. Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org> --- fs/fat/fat.c | 39 ++++++--------------------------------- include/fat.h | 32 ++++++++++++++++++++++++++++++++ 2 files changed, 38 insertions(+), 33 deletions(-)