Using bit shifts for VXFS file modes

Using bit shifts for VXFS file modes

[Amy Parker]

Hello filesystem developers!

I was scouting through the FreeVXFS code, when I came across this in
fs/freevxfs/vxfs.h:

enum vxfs_mode {
        VXFS_ISUID = 0x00000800, /* setuid */
        VXFS_ISGID = 0x00000400, /* setgid */
        VXFS_ISVTX = 0x00000200, /* sticky bit */
        VXFS_IREAD = 0x00000100, /* read */
        VXFS_IWRITE = 0x00000080, /* write */
        VXFS_IEXEC = 0x00000040, /* exec */

Especially in an expanded form like this, these are ugly to read, and
a pain to work with.

An example of potentially a better method, from fs/dax.c:

#define DAX_SHIFT (4)
#define DAX_LOCKED (1UL << 0)
#define DAX_PMD (1UL << 1)
#define DAX_ZERO_PAGE (1UL << 2)
#define DAX_EMPTY (1UL << 3)

Pardon the space condensation - my email client is not functioning properly.

Anyways, I believe using bit shifts to represent different file modes
would be a much better idea - no runtime penalty as they get
calculated into constants at compile time, and significantly easier
for the average user to read.

Any thoughts on this?

Best regards,
Amy Parker
(she/her/hers)

Re: Using bit shifts for VXFS file modes

[Chaitanya Kulkarni]

On 2/1/21 15:50, Amy Parker wrote:
> Especially in an expanded form like this, these are ugly to read, and
> a pain to work with.
>
> An example of potentially a better method, from fs/dax.c:
>
> #define DAX_SHIFT (4)
> #define DAX_LOCKED (1UL << 0)
> #define DAX_PMD (1UL << 1)
> #define DAX_ZERO_PAGE (1UL << 2)
> #define DAX_EMPTY (1UL << 3)
I think this is much better to read with proper alignment.

Re: Using bit shifts for VXFS file modes

[Randy Dunlap]

On 2/1/21 3:49 PM, Amy Parker wrote:
> Hello filesystem developers!
> 
> I was scouting through the FreeVXFS code, when I came across this in
> fs/freevxfs/vxfs.h:
> 
> enum vxfs_mode {
>         VXFS_ISUID = 0x00000800, /* setuid */
>         VXFS_ISGID = 0x00000400, /* setgid */
>         VXFS_ISVTX = 0x00000200, /* sticky bit */
>         VXFS_IREAD = 0x00000100, /* read */
>         VXFS_IWRITE = 0x00000080, /* write */
>         VXFS_IEXEC = 0x00000040, /* exec */
> 
> Especially in an expanded form like this, these are ugly to read, and
> a pain to work with.
> 
> An example of potentially a better method, from fs/dax.c:
> 
> #define DAX_SHIFT (4)
> #define DAX_LOCKED (1UL << 0)
> #define DAX_PMD (1UL << 1)
> #define DAX_ZERO_PAGE (1UL << 2)
> #define DAX_EMPTY (1UL << 3)
> 
> Pardon the space condensation - my email client is not functioning properly.

That's the gmail web interface, right?
I believe that you can use a real email client to talk to
smtp.gmail.com and it won't mangle spaces in your emails.

> Anyways, I believe using bit shifts to represent different file modes
> would be a much better idea - no runtime penalty as they get
> calculated into constants at compile time, and significantly easier
> for the average user to read.
> 
> Any thoughts on this?

It's all just opinions. :)

I find the hex number list easier to read .. and the values are
right there in front of you when debugging, instead of having to
determine what 1 << 9 is.

cheers.
-- 
~Randy

Re: Using bit shifts for VXFS file modes

[Amy Parker]

On Mon, Feb 1, 2021 at 3:57 PM Chaitanya Kulkarni
<Chaitanya.Kulkarni@wdc.com> wrote:
>
> On 2/1/21 15:50, Amy Parker wrote:
> > Especially in an expanded form like this, these are ugly to read, and
> > a pain to work with.
> >
> > An example of potentially a better method, from fs/dax.c:
> >
> > #define DAX_SHIFT (4)
> > #define DAX_LOCKED (1UL << 0)
> > #define DAX_PMD (1UL << 1)
> > #define DAX_ZERO_PAGE (1UL << 2)
> > #define DAX_EMPTY (1UL << 3)
> I think this is much better to read with proper alignment.

Yep - on the actual file it is.

Re: Using bit shifts for VXFS file modes

[Amy Parker]

On Mon, Feb 1, 2021 at 4:21 PM Randy Dunlap <rdunlap@infradead.org> wrote:
>
> On 2/1/21 3:49 PM, Amy Parker wrote:
> > Hello filesystem developers!
> >
> > I was scouting through the FreeVXFS code, when I came across this in
> > fs/freevxfs/vxfs.h:
> >
> > enum vxfs_mode {
> >         VXFS_ISUID = 0x00000800, /* setuid */
> >         VXFS_ISGID = 0x00000400, /* setgid */
> >         VXFS_ISVTX = 0x00000200, /* sticky bit */
> >         VXFS_IREAD = 0x00000100, /* read */
> >         VXFS_IWRITE = 0x00000080, /* write */
> >         VXFS_IEXEC = 0x00000040, /* exec */
> >
> > Especially in an expanded form like this, these are ugly to read, and
> > a pain to work with.
> >
> > An example of potentially a better method, from fs/dax.c:
> >
> > #define DAX_SHIFT (4)
> > #define DAX_LOCKED (1UL << 0)
> > #define DAX_PMD (1UL << 1)
> > #define DAX_ZERO_PAGE (1UL << 2)
> > #define DAX_EMPTY (1UL << 3)
> >
> > Pardon the space condensation - my email client is not functioning properly.
>
> That's the gmail web interface, right?

Sadly for now yes. Currently migrating to my own local email system -
just waiting on a domain purchase.

> I believe that you can use a real email client to talk to
> smtp.gmail.com and it won't mangle spaces in your emails.

I've tried - smtp.gmail.com either requires:
- OAuth2: doesn't work for most things
- App passwords: not used by any email client dev with a touch of respect

>
> > Anyways, I believe using bit shifts to represent different file modes
> > would be a much better idea - no runtime penalty as they get
> > calculated into constants at compile time, and significantly easier
> > for the average user to read.
> >
> > Any thoughts on this?
>
> It's all just opinions. :)

Sure, it's just opinions, but this is about making stuff easier to
work with for the average kernel monkey.

>
> I find the hex number list easier to read .. and the values are

Fair, wonder what the average person would think? When trying to help
others understand hex I usually show representations in bitshift form
so they can get the place value nature.

> right there in front of you when debugging, instead of having to
> determine what 1 << 9 is.

Maybe, but it takes barely more time to find the ninth digit of the
hex expression when debugging. Plus, since you're counting the places
in - you're a lot less likely to accidentally select the wrong one
when you're debugging if you're just eyeballing the values.

Perhaps adding a docstring above this with maps of the bitshifts to
their raw hex equivalents set as current could be a good idea?

>
> cheers.
> --
> ~Randy
>

Best regards,
Amy Parker
(she/her/hers)

Re: Using bit shifts for VXFS file modes

[Theodore Ts'o]

On Mon, Feb 01, 2021 at 03:49:20PM -0800, Amy Parker wrote:
> Hello filesystem developers!
> 
> I was scouting through the FreeVXFS code, when I came across this in
> fs/freevxfs/vxfs.h:
> 
> enum vxfs_mode {
>         VXFS_ISUID = 0x00000800, /* setuid */
>         VXFS_ISGID = 0x00000400, /* setgid */
>         VXFS_ISVTX = 0x00000200, /* sticky bit */
>         VXFS_IREAD = 0x00000100, /* read */
>         VXFS_IWRITE = 0x00000080, /* write */
>         VXFS_IEXEC = 0x00000040, /* exec */

The main reason why some developers prefer to using enum is because it
allows the compiler to do type checking.  Also some people prefer
using hex digits because it becomes easier for people who are looking
at hex dumps.  So for example:

typedef enum {
        EXT4_IGET_NORMAL =      0,
        EXT4_IGET_SPECIAL =     0x0001, /* OK to iget a system inode */
        EXT4_IGET_HANDLE =      0x0002  /* Inode # is from a handle */
} ext4_iget_flags;

> Anyways, I believe using bit shifts to represent different file modes
> would be a much better idea - no runtime penalty as they get
> calculated into constants at compile time, and significantly easier
> for the average user to read.

That's a matter of personal preference; and I'll note that it's not a
matter of what is better for average users, but rather the average
file system developer.  Some people find octal easier, because that
was what Digital Equipment Corporation (DEC) systems tended to use,
and early Unix was developed on PDP-11.  So that's why octal gets used
in the man page for chmod, e.g.:

#define S_IRUSR 00400
#define S_IWUSR 00200
#define S_IXUSR 00100

#define S_IRGRP 00040
#define S_IWGRP 00020
#define S_IXGRP 00010

Personally, *I* find this easier to read than

#define S_IRGRP (1U << 5)
#define S_IWGRP (1U << 4)
#define S_IXGRP (1U << 3)

But perhaps that's because I can convert between octal and binary in
my sleep (having learned how to toggle in disk bootstraps into the
front console of a PDP-8i[1] when I was in grade school).

[1] https://www.vintagecomputer.net/digital/pdp8i/Digital_PDP8i_a.JPG

> Any thoughts on this?

I don't think there's a right answer here.  In some cases, hex will be
better; in some cases, octal (especially as far as Unix permissions is
concerned); and in other cases, perhaps using bit shifts is more important.

A lot depends on how you plan can use it, and your past experiewnce.
Maybe you can take left shift numbers and be able to translate that to
hex when looking at kernel oops messages; I can't, but I can take hex
definiions and can take something like 0xA453 and map that to what
flags are set that are defined using hex constants.

Cheers,

						- Ted

Re: Using bit shifts for VXFS file modes

[Darrick J. Wong]

On Mon, Feb 01, 2021 at 03:49:20PM -0800, Amy Parker wrote:
> Hello filesystem developers!
> 
> I was scouting through the FreeVXFS code, when I came across this in
> fs/freevxfs/vxfs.h:
> 
> enum vxfs_mode {
>         VXFS_ISUID = 0x00000800, /* setuid */
>         VXFS_ISGID = 0x00000400, /* setgid */
>         VXFS_ISVTX = 0x00000200, /* sticky bit */
>         VXFS_IREAD = 0x00000100, /* read */
>         VXFS_IWRITE = 0x00000080, /* write */
>         VXFS_IEXEC = 0x00000040, /* exec */
> 
> Especially in an expanded form like this, these are ugly to read, and
> a pain to work with.

I would personally just change those to use the constants in
include/uapi/linux/stat.h.  They're userspace ABI and I don't think
anyone's going to come up with a good reason to change the numbering
after nearly 50 years.

That said, on the general principle of "anything you touch you get to
QA" I would leave it alone.

--D

> 
> An example of potentially a better method, from fs/dax.c:
> 
> #define DAX_SHIFT (4)
> #define DAX_LOCKED (1UL << 0)
> #define DAX_PMD (1UL << 1)
> #define DAX_ZERO_PAGE (1UL << 2)
> #define DAX_EMPTY (1UL << 3)
> 
> Pardon the space condensation - my email client is not functioning properly.
> 
> Anyways, I believe using bit shifts to represent different file modes
> would be a much better idea - no runtime penalty as they get
> calculated into constants at compile time, and significantly easier
> for the average user to read.
> 
> Any thoughts on this?
> 
> Best regards,
> Amy Parker
> (she/her/hers)

Re: Using bit shifts for VXFS file modes

[Amy Parker]

On Mon, Feb 1, 2021 at 5:48 PM Darrick J. Wong <djwong@kernel.org> wrote:
>
> On Mon, Feb 01, 2021 at 03:49:20PM -0800, Amy Parker wrote:
> > Hello filesystem developers!
> >
> > I was scouting through the FreeVXFS code, when I came across this in
> > fs/freevxfs/vxfs.h:
> >
> > enum vxfs_mode {
> >         VXFS_ISUID = 0x00000800, /* setuid */
> >         VXFS_ISGID = 0x00000400, /* setgid */
> >         VXFS_ISVTX = 0x00000200, /* sticky bit */
> >         VXFS_IREAD = 0x00000100, /* read */
> >         VXFS_IWRITE = 0x00000080, /* write */
> >         VXFS_IEXEC = 0x00000040, /* exec */
> >
> > Especially in an expanded form like this, these are ugly to read, and
> > a pain to work with.
>
> I would personally just change those to use the constants in
> include/uapi/linux/stat.h.  They're userspace ABI and I don't think
> anyone's going to come up with a good reason to change the numbering
> after nearly 50 years.

This is a great idea. Didn't think of that, thank you.

>
> That said, on the general principle of "anything you touch you get to
> QA" I would leave it alone.
>

Yeah, I guess that's there too.

Best regards,
Amy Parker
(she/her/hers)