Re: FAT macro

[Saurav Pal]

Hi,

Thanks for the tip! I've created an issue and a PR, now waiting for a
review.

Regards,
Saurav

On Sat, Feb 3, 2024 at 1:57 AM Alan C. Assis  wrote:

> Hi Saurav,
>
> You can follow the steps here:
> https://nuttx.apache.org/docs/latest/contributing/index.html
>
> After running the checkpatch.sh and confirming everything is fine, you can
> create a commit.
>
> Basically your commit should be like this:
>
>
> --
> fs/vfat: Fix typo in the macro DIRSEC_BYTENDX
>
> Some comments explaining what the commit is supposed to do!
>
> Signed-off-by: Saurav Pal 
>
> --
>
> Best Regards,
>
> Alan
>
> On Fri, Feb 2, 2024 at 5:14 PM Saurav Pal  wrote:
>
> > Hi Alan,
> >
> > Yeah I'll add an issue in the repo, and put a PR for fixing this. A quick
> > question...is there any standard on commit names, or should I just use my
> > own like "FIX: VFAT macro Issue#123"?
> >
> > About why side effects have not been seen...like Lwazi explained,
> > macro DIRSEC_BYTENDX(f,
> > i) is always used as DIRSEC_BYTENDX(fs, inode), and so, in its expansion,
> > DIRSEC_NDXMASK(f) should become DIRSEC_NDXMASK(fs), and that weirdly
> > coincides with what the typo is, and so, it weirdly works how it was
> > intended to and does not give any errors.
> >
> > Since it is a macro, even language servers (as far as the ones I know) do
> > not visually show their "unused parameters" warning like in the case
> proper
> > functions.
> >
> > Regards,
> > Saurav
> >
> > On Sat, Feb 3, 2024 at 1:11 AM Alan C. Assis  wrote:
> >
> > > Hi Saurav,
> > >
> > > I think you found a BUG!
> > >
> > > Please report it at https://github.com/apache/nuttx/issues to keep a
> > track
> > > of it and when you we submit a PR it could be closed automatically
> (since
> > > you link it at your PR).
> > >
> > > In fact looking both macros the f to fs mistake becomes clear:
> > >
> > > #define DIRSEC_NDXMASK(f)   (((f)->fs_hwsectorsize - 1) >> 5)
> > >
> > > #define DIRSEC_BYTENDX(f,i) (((i) & DIRSEC_NDXMASK(fs)) << 5)
> > >
> > > I'm curious to know what this mistake could cause to VFAT on NuttX?
> > >
> > > Why haven't we seen any side effects in products using NuttX with VFAT?
> > >
> > > Best Regards,
> > >
> > > Alan
> > >
> > > On Fri, Feb 2, 2024 at 1:55 PM Saurav Pal 
> wrote:
> > >
> > > > Hi Alan,
> > > >
> > > > Thank you for looking at our code base and planning to add
> > Documentation,
> > > > > that is really important!
> > > > >
> > > > > NuttX has a long history but our Documentation is still lagging
> > behind,
> > > > so
> > > > > your work will be very beneficial for our community.
> > > > >
> > > >
> > > > I'll try my best to contribute some documentation for the various
> file
> > > > systems I look into, and I, in turn, hope they are informative, and
> > more
> > > > importantly, correct, as I'm pretty new to the VFS layer of NuttX.
> > > >
> > > >
> > > > > BTW, why do you think it is wrong, could you please share your
> > > thoughts?
> > > > >
> > > >
> > > > The problem according to me is on L264...where it's defined as:
> > > >
> > > > #define DIRSEC_BYTENDX(*f*,i) (((i) & DIRSEC_NDXMASK(*fs*)) << 5)
> > > >
> > > > I can't seem to find anything defined as *fs *in the namespace at
> that
> > > > point (used in the fat_findsfnentry
> > > > <
> > >
> >
> https://github.com/apache/nuttx/blob/master/fs/fat/fs_fat32dirent.c#L1137
> > > > >function),
> > > > and I was wondering if it was a simple typo or a mistake on my part
> as
> > > it's
> > > > code that's quite old, which usually don't have such mistakes due to
> > > > extensive testing.
> > > >
> > > > Best Regards,
> > > > Saurav
> > > >
> > >
> >
>

Re: FAT macro

[Alan C. Assis]

Hi Saurav,

You can follow the steps here:
https://nuttx.apache.org/docs/latest/contributing/index.html

After running the checkpatch.sh and confirming everything is fine, you can
create a commit.

Basically your commit should be like this:

--
fs/vfat: Fix typo in the macro DIRSEC_BYTENDX

Some comments explaining what the commit is supposed to do!

Signed-off-by: Saurav Pal 
--

Best Regards,

Alan

On Fri, Feb 2, 2024 at 5:14 PM Saurav Pal  wrote:

> Hi Alan,
>
> Yeah I'll add an issue in the repo, and put a PR for fixing this. A quick
> question...is there any standard on commit names, or should I just use my
> own like "FIX: VFAT macro Issue#123"?
>
> About why side effects have not been seen...like Lwazi explained,
> macro DIRSEC_BYTENDX(f,
> i) is always used as DIRSEC_BYTENDX(fs, inode), and so, in its expansion,
> DIRSEC_NDXMASK(f) should become DIRSEC_NDXMASK(fs), and that weirdly
> coincides with what the typo is, and so, it weirdly works how it was
> intended to and does not give any errors.
>
> Since it is a macro, even language servers (as far as the ones I know) do
> not visually show their "unused parameters" warning like in the case proper
> functions.
>
> Regards,
> Saurav
>
> On Sat, Feb 3, 2024 at 1:11 AM Alan C. Assis  wrote:
>
> > Hi Saurav,
> >
> > I think you found a BUG!
> >
> > Please report it at https://github.com/apache/nuttx/issues to keep a
> track
> > of it and when you we submit a PR it could be closed automatically (since
> > you link it at your PR).
> >
> > In fact looking both macros the f to fs mistake becomes clear:
> >
> > #define DIRSEC_NDXMASK(f)   (((f)->fs_hwsectorsize - 1) >> 5)
> >
> > #define DIRSEC_BYTENDX(f,i) (((i) & DIRSEC_NDXMASK(fs)) << 5)
> >
> > I'm curious to know what this mistake could cause to VFAT on NuttX?
> >
> > Why haven't we seen any side effects in products using NuttX with VFAT?
> >
> > Best Regards,
> >
> > Alan
> >
> > On Fri, Feb 2, 2024 at 1:55 PM Saurav Pal  wrote:
> >
> > > Hi Alan,
> > >
> > > Thank you for looking at our code base and planning to add
> Documentation,
> > > > that is really important!
> > > >
> > > > NuttX has a long history but our Documentation is still lagging
> behind,
> > > so
> > > > your work will be very beneficial for our community.
> > > >
> > >
> > > I'll try my best to contribute some documentation for the various file
> > > systems I look into, and I, in turn, hope they are informative, and
> more
> > > importantly, correct, as I'm pretty new to the VFS layer of NuttX.
> > >
> > >
> > > > BTW, why do you think it is wrong, could you please share your
> > thoughts?
> > > >
> > >
> > > The problem according to me is on L264...where it's defined as:
> > >
> > > #define DIRSEC_BYTENDX(*f*,i) (((i) & DIRSEC_NDXMASK(*fs*)) << 5)
> > >
> > > I can't seem to find anything defined as *fs *in the namespace at that
> > > point (used in the fat_findsfnentry
> > > <
> >
> https://github.com/apache/nuttx/blob/master/fs/fat/fs_fat32dirent.c#L1137
> > > >function),
> > > and I was wondering if it was a simple typo or a mistake on my part as
> > it's
> > > code that's quite old, which usually don't have such mistakes due to
> > > extensive testing.
> > >
> > > Best Regards,
> > > Saurav
> > >
> >
>

Re: FAT macro

[Saurav Pal]

Hi Alan,

Yeah I'll add an issue in the repo, and put a PR for fixing this. A quick
question...is there any standard on commit names, or should I just use my
own like "FIX: VFAT macro Issue#123"?

About why side effects have not been seen...like Lwazi explained,
macro DIRSEC_BYTENDX(f,
i) is always used as DIRSEC_BYTENDX(fs, inode), and so, in its expansion,
DIRSEC_NDXMASK(f) should become DIRSEC_NDXMASK(fs), and that weirdly
coincides with what the typo is, and so, it weirdly works how it was
intended to and does not give any errors.

Since it is a macro, even language servers (as far as the ones I know) do
not visually show their "unused parameters" warning like in the case proper
functions.

Regards,
Saurav

On Sat, Feb 3, 2024 at 1:11 AM Alan C. Assis  wrote:

> Hi Saurav,
>
> I think you found a BUG!
>
> Please report it at https://github.com/apache/nuttx/issues to keep a track
> of it and when you we submit a PR it could be closed automatically (since
> you link it at your PR).
>
> In fact looking both macros the f to fs mistake becomes clear:
>
> #define DIRSEC_NDXMASK(f)   (((f)->fs_hwsectorsize - 1) >> 5)
>
> #define DIRSEC_BYTENDX(f,i) (((i) & DIRSEC_NDXMASK(fs)) << 5)
>
> I'm curious to know what this mistake could cause to VFAT on NuttX?
>
> Why haven't we seen any side effects in products using NuttX with VFAT?
>
> Best Regards,
>
> Alan
>
> On Fri, Feb 2, 2024 at 1:55 PM Saurav Pal  wrote:
>
> > Hi Alan,
> >
> > Thank you for looking at our code base and planning to add Documentation,
> > > that is really important!
> > >
> > > NuttX has a long history but our Documentation is still lagging behind,
> > so
> > > your work will be very beneficial for our community.
> > >
> >
> > I'll try my best to contribute some documentation for the various file
> > systems I look into, and I, in turn, hope they are informative, and more
> > importantly, correct, as I'm pretty new to the VFS layer of NuttX.
> >
> >
> > > BTW, why do you think it is wrong, could you please share your
> thoughts?
> > >
> >
> > The problem according to me is on L264...where it's defined as:
> >
> > #define DIRSEC_BYTENDX(*f*,i) (((i) & DIRSEC_NDXMASK(*fs*)) << 5)
> >
> > I can't seem to find anything defined as *fs *in the namespace at that
> > point (used in the fat_findsfnentry
> > <
> https://github.com/apache/nuttx/blob/master/fs/fat/fs_fat32dirent.c#L1137
> > >function),
> > and I was wondering if it was a simple typo or a mistake on my part as
> it's
> > code that's quite old, which usually don't have such mistakes due to
> > extensive testing.
> >
> > Best Regards,
> > Saurav
> >
>

Re: FAT macro

[Alan C. Assis]

Hi Saurav,

I think you found a BUG!

Please report it at https://github.com/apache/nuttx/issues to keep a track
of it and when you we submit a PR it could be closed automatically (since
you link it at your PR).

In fact looking both macros the f to fs mistake becomes clear:

#define DIRSEC_NDXMASK(f)   (((f)->fs_hwsectorsize - 1) >> 5)

#define DIRSEC_BYTENDX(f,i) (((i) & DIRSEC_NDXMASK(fs)) << 5)

I'm curious to know what this mistake could cause to VFAT on NuttX?

Why haven't we seen any side effects in products using NuttX with VFAT?

Best Regards,

Alan

On Fri, Feb 2, 2024 at 1:55 PM Saurav Pal  wrote:

> Hi Alan,
>
> Thank you for looking at our code base and planning to add Documentation,
> > that is really important!
> >
> > NuttX has a long history but our Documentation is still lagging behind,
> so
> > your work will be very beneficial for our community.
> >
>
> I'll try my best to contribute some documentation for the various file
> systems I look into, and I, in turn, hope they are informative, and more
> importantly, correct, as I'm pretty new to the VFS layer of NuttX.
>
>
> > BTW, why do you think it is wrong, could you please share your thoughts?
> >
>
> The problem according to me is on L264...where it's defined as:
>
> #define DIRSEC_BYTENDX(*f*,i) (((i) & DIRSEC_NDXMASK(*fs*)) << 5)
>
> I can't seem to find anything defined as *fs *in the namespace at that
> point (used in the fat_findsfnentry
>  >function),
> and I was wondering if it was a simple typo or a mistake on my part as it's
> code that's quite old, which usually don't have such mistakes due to
> extensive testing.
>
> Best Regards,
> Saurav
>

Re: FAT macro

[Saurav Pal]

Hi,

Thanks for the confirmation and an explanation as to why it works even as
it is!

Regards,
Saurav

On Fri, 2 Feb, 2024, 22:35 Lwazi Dube,  wrote:

> It is a typo. Please change to DIRSEC_NDXMASK(f)
> There are no errors because fs is already available in every function that
> "calls" DIRSEC_BYTENDX.
>
> On Fri, 2 Feb 2024 at 11:55, Saurav Pal  wrote:
>
> > Hi Alan,
> >
> > Thank you for looking at our code base and planning to add Documentation,
> > > that is really important!
> > >
> > > NuttX has a long history but our Documentation is still lagging behind,
> > so
> > > your work will be very beneficial for our community.
> > >
> >
> > I'll try my best to contribute some documentation for the various file
> > systems I look into, and I, in turn, hope they are informative, and more
> > importantly, correct, as I'm pretty new to the VFS layer of NuttX.
> >
> >
> > > BTW, why do you think it is wrong, could you please share your
> thoughts?
> > >
> >
> > The problem according to me is on L264...where it's defined as:
> >
> > #define DIRSEC_BYTENDX(*f*,i) (((i) & DIRSEC_NDXMASK(*fs*)) << 5)
> >
> > I can't seem to find anything defined as *fs *in the namespace at that
> > point (used in the fat_findsfnentry
> > <
> https://github.com/apache/nuttx/blob/master/fs/fat/fs_fat32dirent.c#L1137
> > >function),
> > and I was wondering if it was a simple typo or a mistake on my part as
> it's
> > code that's quite old, which usually don't have such mistakes due to
> > extensive testing.
> >
> > Best Regards,
> > Saurav
> >
>

Re: FAT macro

[Lwazi Dube]

It is a typo. Please change to DIRSEC_NDXMASK(f)
There are no errors because fs is already available in every function that
"calls" DIRSEC_BYTENDX.

On Fri, 2 Feb 2024 at 11:55, Saurav Pal  wrote:

> Hi Alan,
>
> Thank you for looking at our code base and planning to add Documentation,
> > that is really important!
> >
> > NuttX has a long history but our Documentation is still lagging behind,
> so
> > your work will be very beneficial for our community.
> >
>
> I'll try my best to contribute some documentation for the various file
> systems I look into, and I, in turn, hope they are informative, and more
> importantly, correct, as I'm pretty new to the VFS layer of NuttX.
>
>
> > BTW, why do you think it is wrong, could you please share your thoughts?
> >
>
> The problem according to me is on L264...where it's defined as:
>
> #define DIRSEC_BYTENDX(*f*,i) (((i) & DIRSEC_NDXMASK(*fs*)) << 5)
>
> I can't seem to find anything defined as *fs *in the namespace at that
> point (used in the fat_findsfnentry
>  >function),
> and I was wondering if it was a simple typo or a mistake on my part as it's
> code that's quite old, which usually don't have such mistakes due to
> extensive testing.
>
> Best Regards,
> Saurav
>

Re: FAT macro

[Saurav Pal]

Hi Alan,

Thank you for looking at our code base and planning to add Documentation,
> that is really important!
>
> NuttX has a long history but our Documentation is still lagging behind, so
> your work will be very beneficial for our community.
>

I'll try my best to contribute some documentation for the various file
systems I look into, and I, in turn, hope they are informative, and more
importantly, correct, as I'm pretty new to the VFS layer of NuttX.


> BTW, why do you think it is wrong, could you please share your thoughts?
>

The problem according to me is on L264...where it's defined as:

#define DIRSEC_BYTENDX(*f*,i) (((i) & DIRSEC_NDXMASK(*fs*)) << 5)

I can't seem to find anything defined as *fs *in the namespace at that
point (used in the fat_findsfnentry
function),
and I was wondering if it was a simple typo or a mistake on my part as it's
code that's quite old, which usually don't have such mistakes due to
extensive testing.

Best Regards,
Saurav

Re: FAT macro

[Alan C. Assis]

Hi Saurav,

Thank you for looking at our code base and planning to add Documentation,
that is really important!

NuttX has a long history but our Documentation is still lagging behind, so
your work will be very beneficial for our community.

I took a look at that macro you asked and seems it was wrote by Greg almost
16 years ago:

08d439fefbd (patacongo  2008-08-02 14:44:25 +  262) #define
DIRSEC_NDXMASK(f)   (((f)->fs_hwsectorsize - 1) >> 5)

BTW, why do you think it is wrong, could you please share your thoughts?

Best Regards,

Alan

On Fri, Feb 2, 2024 at 3:50 AM Saurav Pal  wrote:

> Hi,
>
> I was browsing through the source code of FAT implementation, trying to
> write some documentation, and I came across this macro
> .
>
> Since I'm new to the codebase, I wanted to know if this (especially
> DIRSEC_NDXMASK(fs)) is correct and I'm missing something or if it should
> have been DIRSEC_NDXMASK(f) (and I can raise an issue for it).
>
> Thanks and regards,
> Saurav
>