diff mbox series

[v2,01/23] fs: fat: guard the content of include/fat.h

Message ID 20180904074948.18146-2-takahiro.akashi@linaro.org
State Superseded
Headers show
Series subject: fs: fat: extend FAT write operations | expand

Commit Message

AKASHI Takahiro Sept. 4, 2018, 7:49 a.m. UTC
The whole content of include/fat.h is private to FAT implementation
and then should be guarded with CONFIG_FS_FAT.

Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org>
---
 include/fat.h | 3 +++
 1 file changed, 3 insertions(+)

Comments

Alexander Graf Sept. 4, 2018, 8:52 a.m. UTC | #1
On 04.09.18 09:49, AKASHI Takahiro wrote:
> The whole content of include/fat.h is private to FAT implementation
> and then should be guarded with CONFIG_FS_FAT.
> 
> Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org>
> ---
>  include/fat.h | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/include/fat.h b/include/fat.h
> index 09e142368585..c02839dcb040 100644
> --- a/include/fat.h
> +++ b/include/fat.h
> @@ -9,6 +9,8 @@
>  #ifndef _FAT_H_
>  #define _FAT_H_
>  
> +#ifdef CONFIG_FS_FAT

Isn't this missing an include of at least common.h to actually propagate
the config define?

Also, you want to use #if CONFIG_IS_ENABLED(...) here to guard against
SPL builds too.

However, I don't fully grasp why you need this patch. Please describe in
your commit message what the incentive is for creating it. What errors
are you trying to protect against?


Alex

> +
>  #include <asm/byteorder.h>
>  #include <fs.h>
>  
> @@ -202,4 +204,5 @@ int fat_opendir(const char *filename, struct fs_dir_stream **dirsp);
>  int fat_readdir(struct fs_dir_stream *dirs, struct fs_dirent **dentp);
>  void fat_closedir(struct fs_dir_stream *dirs);
>  void fat_close(void);
> +#endif /* CONFIG_FS_FAT */
>  #endif /* _FAT_H_ */
>
Heinrich Schuchardt Sept. 4, 2018, 10:46 a.m. UTC | #2
On 09/04/2018 10:52 AM, Alexander Graf wrote:
> 
> 
> On 04.09.18 09:49, AKASHI Takahiro wrote:
>> The whole content of include/fat.h is private to FAT implementation
>> and then should be guarded with CONFIG_FS_FAT.

Hello Takahiro,

doesn't this imply that building common/spl/spl_sata.c without FAT will
fail for CONFIG_SPL_SATA_SUPPORT=y (e.g. cm_t54_defconfig with FAT
disabled).

Did you run Travis CI on your patch series?

>>
>> Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org>
>> ---
>>  include/fat.h | 3 +++
>>  1 file changed, 3 insertions(+)
>>
>> diff --git a/include/fat.h b/include/fat.h
>> index 09e142368585..c02839dcb040 100644
>> --- a/include/fat.h
>> +++ b/include/fat.h
>> @@ -9,6 +9,8 @@
>>  #ifndef _FAT_H_
>>  #define _FAT_H_
>>  
>> +#ifdef CONFIG_FS_FAT
> 
> Isn't this missing an include of at least common.h to actually propagate
> the config define?
> 
> Also, you want to use #if CONFIG_IS_ENABLED(...) here to guard against
> SPL builds too.

Probably not:
common/spl/spl_fat.c:14:#include <fat.h>

Best regards

Heinrich Schuchardt

> 
> However, I don't fully grasp why you need this patch. Please describe in
> your commit message what the incentive is for creating it. What errors
> are you trying to protect against?
> 
> 
> Alex
> 
>> +
>>  #include <asm/byteorder.h>
>>  #include <fs.h>
>>  
>> @@ -202,4 +204,5 @@ int fat_opendir(const char *filename, struct fs_dir_stream **dirsp);
>>  int fat_readdir(struct fs_dir_stream *dirs, struct fs_dirent **dentp);
>>  void fat_closedir(struct fs_dir_stream *dirs);
>>  void fat_close(void);
>> +#endif /* CONFIG_FS_FAT */
>>  #endif /* _FAT_H_ */
>>
>
AKASHI Takahiro Sept. 5, 2018, 1:14 a.m. UTC | #3
On Tue, Sep 04, 2018 at 10:52:20AM +0200, Alexander Graf wrote:
> 
> 
> On 04.09.18 09:49, AKASHI Takahiro wrote:
> > The whole content of include/fat.h is private to FAT implementation
> > and then should be guarded with CONFIG_FS_FAT.
> > 
> > Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org>
> > ---
> >  include/fat.h | 3 +++
> >  1 file changed, 3 insertions(+)
> > 
> > diff --git a/include/fat.h b/include/fat.h
> > index 09e142368585..c02839dcb040 100644
> > --- a/include/fat.h
> > +++ b/include/fat.h
> > @@ -9,6 +9,8 @@
> >  #ifndef _FAT_H_
> >  #define _FAT_H_
> >  
> > +#ifdef CONFIG_FS_FAT
> 
> Isn't this missing an include of at least common.h to actually propagate
> the config define?
> 
> Also, you want to use #if CONFIG_IS_ENABLED(...) here to guard against
> SPL builds too.

Let's discuss in a reply to Heinrich's comment.

> However, I don't fully grasp why you need this patch. Please describe in
> your commit message what the incentive is for creating it. What errors
> are you trying to protect against?

Sure; See
https://lists.denx.de/pipermail/u-boot/2018-August/338054.html

In short, one macro definition in this file will break without CONFI_FS_FAT_*.

Thanks,
-Takahiro AKASHI

> 
> Alex
> 
> > +
> >  #include <asm/byteorder.h>
> >  #include <fs.h>
> >  
> > @@ -202,4 +204,5 @@ int fat_opendir(const char *filename, struct fs_dir_stream **dirsp);
> >  int fat_readdir(struct fs_dir_stream *dirs, struct fs_dirent **dentp);
> >  void fat_closedir(struct fs_dir_stream *dirs);
> >  void fat_close(void);
> > +#endif /* CONFIG_FS_FAT */
> >  #endif /* _FAT_H_ */
> >
AKASHI Takahiro Sept. 5, 2018, 1:54 a.m. UTC | #4
Hi Heinrich, Alex,

On Tue, Sep 04, 2018 at 12:46:58PM +0200, Heinrich Schuchardt wrote:
> 
> 
> On 09/04/2018 10:52 AM, Alexander Graf wrote:
> > 
> > 
> > On 04.09.18 09:49, AKASHI Takahiro wrote:
> >> The whole content of include/fat.h is private to FAT implementation
> >> and then should be guarded with CONFIG_FS_FAT.
> 
> Hello Takahiro,
> 
> doesn't this imply that building common/spl/spl_sata.c without FAT will
> fail for CONFIG_SPL_SATA_SUPPORT=y (e.g. cm_t54_defconfig with FAT
> disabled).

Yes, you're right, but this is a matter of cm_t54_defconfig.
spl_sta.c uses spl_load_image_fat[_os]() which are implemented
in spl_fat.c which replies on CONFIG_SPL_FAT_SUPPORT, which
will in turn select CONFIG_FS_FAT.
The problem is that cm_t54_defconfig doesn't enable
CONFIG_SPL_FAT_SUPPORT.

To fix this issue, we should add "depends on SPL_FAT_SUPPORT"
to SPL_SATA_SUPPORT (and SPL_USB_SUPPORT) as well.

> Did you run Travis CI on your patch series?

No. Is everybody required to run Travis CI for u-boot
before any submission?

> >>
> >> Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org>
> >> ---
> >>  include/fat.h | 3 +++
> >>  1 file changed, 3 insertions(+)
> >>
> >> diff --git a/include/fat.h b/include/fat.h
> >> index 09e142368585..c02839dcb040 100644
> >> --- a/include/fat.h
> >> +++ b/include/fat.h
> >> @@ -9,6 +9,8 @@
> >>  #ifndef _FAT_H_
> >>  #define _FAT_H_
> >>  
> >> +#ifdef CONFIG_FS_FAT
> > 
> > Isn't this missing an include of at least common.h to actually propagate
> > the config define?
> > 
> > Also, you want to use #if CONFIG_IS_ENABLED(...) here to guard against
> > SPL builds too.
> 
> Probably not:

As I'm saying, CONFIG_SPL_FAT_SUPPORT selects CONFIG_FS_FAT
and so either "ifdef CONFIG_FS_FAT" or "if  CONFIG_IS_ENABLED(FS_FAT)" will
turn into True although CONFIG_SPL_FS_FAT actually doesn't exist.

Therefore, I don't think we need a change Alex suggested.

Thanks,
-Takahiro AKASHI

> common/spl/spl_fat.c:14:#include <fat.h>
> 
> Best regards
> 
> Heinrich Schuchardt
> 
> > 
> > However, I don't fully grasp why you need this patch. Please describe in
> > your commit message what the incentive is for creating it. What errors
> > are you trying to protect against?
> > 
> > 
> > Alex
> > 
> >> +
> >>  #include <asm/byteorder.h>
> >>  #include <fs.h>
> >>  
> >> @@ -202,4 +204,5 @@ int fat_opendir(const char *filename, struct fs_dir_stream **dirsp);
> >>  int fat_readdir(struct fs_dir_stream *dirs, struct fs_dirent **dentp);
> >>  void fat_closedir(struct fs_dir_stream *dirs);
> >>  void fat_close(void);
> >> +#endif /* CONFIG_FS_FAT */
> >>  #endif /* _FAT_H_ */
> >>
> >
Heinrich Schuchardt Sept. 5, 2018, 5:53 a.m. UTC | #5
On 09/05/2018 03:54 AM, AKASHI Takahiro wrote:
>> Did you run Travis CI on your patch series?
> No. Is everybody required to run Travis CI for u-boot
> before any submission?
> 

The custodian (Alex) will run Travis CI tests and if they fail you don't
get your patch series merged.

So it is advisable for the submitter of the patch series to already run
the same tests, especially for large changes.

This will require that you have a repository on Github and authorize
Travis CI to access it. Then every time you push to the repository the
Travis test suite will be run.

As this takes several hours I have decided to use a separate repository
for Travis testing only.

Best regards

Heinrich
diff mbox series

Patch

diff --git a/include/fat.h b/include/fat.h
index 09e142368585..c02839dcb040 100644
--- a/include/fat.h
+++ b/include/fat.h
@@ -9,6 +9,8 @@ 
 #ifndef _FAT_H_
 #define _FAT_H_
 
+#ifdef CONFIG_FS_FAT
+
 #include <asm/byteorder.h>
 #include <fs.h>
 
@@ -202,4 +204,5 @@  int fat_opendir(const char *filename, struct fs_dir_stream **dirsp);
 int fat_readdir(struct fs_dir_stream *dirs, struct fs_dirent **dentp);
 void fat_closedir(struct fs_dir_stream *dirs);
 void fat_close(void);
+#endif /* CONFIG_FS_FAT */
 #endif /* _FAT_H_ */