[Toybox] stat: Some cleanup

[Felix Janda]

Hello,

attached are two patches cleaning up stat a bit. The first changes from
4 to 2 space indentation, the second is more interesting.

First of all it extends stat to correctly handle format strings.
(Previously it considered only the second char of the format string.)
This made it possible to remove duplication between print_stat_format()
and stat_main(). The option processing now uses the toybox infrastructure
better and by sorting the options in an appropriate way stat_main()
could be simplified greatly. get_access_str() is now no longer broken.
Apart from this there are various other small changes.

The functionality of get_access_str() is also implemented in the ls toy
(likely much better). Should something like it be in the library?
Actually since stat and ls are both frontends to the stat system call
more is duplicated.

I also wonder whether toybox will ever have SELinux support... My local
man page does even not document the -Z option to stat.

Felix
-------------- next part --------------
A non-text attachment was scrubbed...
Name: stat-reindent.patch
Type: text/x-patch
Size: 18002 bytes
Desc: not available
URL: <http://lists.landley.net/pipermail/toybox-landley.net/attachments/20130420/42012297/attachment-0002.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: stat-cleanup.patch
Type: text/x-patch
Size: 13564 bytes
Desc: not available
URL: <http://lists.landley.net/pipermail/toybox-landley.net/attachments/20130420/42012297/attachment-0003.bin>

[Rob Landley]

Post by Felix Janda
Hello,
attached are two patches cleaning up stat a bit. The first changes from
4 to 2 space indentation, the second is more interesting.
First of all it extends stat to correctly handle format strings.
(Previously it considered only the second char of the format string.)
This made it possible to remove duplication between
print_stat_format()
and stat_main(). The option processing now uses the toybox
infrastructure
better and by sorting the options in an appropriate way stat_main()
could be simplified greatly. get_access_str() is now no longer broken.
Apart from this there are various other small changes.

Cool, and applied.

Post by Felix Janda
The functionality of get_access_str() is also implemented in the ls toy
(likely much better). Should something like it be in the library?

Once code can be shared by a second command, putting it in lib makes
sense.

Post by Felix Janda
Actually since stat and ls are both frontends to the stat system call
more is duplicated.

If you can come up with a clean way to share the code, go for it.

I note that I have a pending todo item in ls: ls -l /dev/null doesn't
show the major/minor numbers. (Oops. Needs another output mode for
device nodes. If I did one, it's not triggering right...)

Post by Felix Janda
I also wonder whether toybox will ever have SELinux support... My local
man page does even not document the -Z option to stat.

I'm leaning against it. If you saw my toybox talk at ELC (it's on
youtube) I talked about security approaches and containers vs selinux.

Post by Felix Janda
Felix

Rob

[Felix Janda]

Post by Rob Landley

[snip]
The functionality of get_access_str() is also implemented in the ls toy
(likely much better). Should something like it be in the library?

Once code can be shared by a second command, putting it in lib makes
sense.

Actually since stat and ls are both frontends to the stat system call
more is duplicated.

If you can come up with a clean way to share the code, go for it.

Ok, actually I'd only put something like get_access_str() of stat into
the lib.

Post by Rob Landley
I note that I have a pending todo item in ls: ls -l /dev/null doesn't
show the major/minor numbers. (Oops. Needs another output mode for
device nodes. If I did one, it's not triggering right...)

Ok.

Post by Rob Landley

I also wonder whether toybox will ever have SELinux support... My local
man page does even not document the -Z option to stat.

I'm leaning against it. If you saw my toybox talk at ELC (it's on
youtube) I talked about security approaches and containers vs selinux.

I anticipated that (and read your outline and skimmed the video). So it
would be ok to remove the SELinux parts from the command (which anyway
only say that they are not implemented)?

Removing these the only thing that "Needs to be implemented" is "%T" for
file systems i.e. file system type in human readable form. So to
implement this one would need to find names for the magic numbers in
linux/magic.h.

(Interesting:

#define XENFS_SUPER_MAGIC 0xabba1974

But fortunately no deadbeef.)

Should "%T" be implemented?

Apart from this I think everything using only the information from
stat/statfs should be implemented (like %t %T for devices). Then there
are %w and %W, which regard file birth time, and for which one would
need an xstat system call... (

https://lwn.net/Articles/397442/

says something about file birth times and links to an article on xstat.)

Felix

[Felix Janda]

Post by Rob Landley

[snip]
The functionality of get_access_str() is also implemented in the ls toy
(likely much better). Should something like it be in the library?

Once code can be shared by a second command, putting it in lib makes
sense.

Actually since stat and ls are both frontends to the stat system call
more is duplicated.

If you can come up with a clean way to share the code, go for it.

Ok, actually I'd only put something like get_access_str() of stat into
the lib.

Post by Rob Landley
I note that I have a pending todo item in ls: ls -l /dev/null doesn't
show the major/minor numbers. (Oops. Needs another output mode for
device nodes. If I did one, it's not triggering right...)

Ok.

Post by Rob Landley

I also wonder whether toybox will ever have SELinux support... My local
man page does even not document the -Z option to stat.

I'm leaning against it. If you saw my toybox talk at ELC (it's on
youtube) I talked about security approaches and containers vs selinux.

I anticipated that (and read your outline and skimmed the video). So it
would be ok to remove the SELinux parts from the command (which anyway
only say that they are not implemented)?

Removing these the only thing that "Needs to be implemented" is "%T" for
file systems i.e. file system type in human readable form. So to
implement this one would need to find names for the magic numbers in
linux/magic.h.

(Interesting:

#define XENFS_SUPER_MAGIC 0xabba1974

But fortunately no deadbeef.)

Should "%T" be implemented?

Apart from this I think everything using only the information from
stat/statfs should be implemented (like %t %T for devices). Then there
are %w and %W, which regard file birth time, and for which one would
need an xstat system call... (

https://lwn.net/Articles/397442/

says something about file birth times and links to an article on xstat.)

Felix

[Rob Landley]

Post by Felix Janda

Post by Rob Landley
If you can come up with a clean way to share the code, go for it.

Ok, actually I'd only put something like get_access_str() of stat into
the lib.

Cool. Although a quick glance at the one in stat implies it doesn't
handle the fun combinations of "suid and sgid bits without
corresponding executable bit when applied to a directory so it's the
sticky bit" that I had to make work in ls.

stat /dev/null
stat /tmp

So if you actually need that behavior you'll need the blob of ls
starting around line 340...

Post by Felix Janda

Post by Rob Landley
I'm leaning against it. If you saw my toybox talk at ELC (it's on
youtube) I talked about security approaches and containers vs

selinux.
I anticipated that (and read your outline and skimmed the video). So
it
would be ok to remove the SELinux parts from the command (which anyway
only say that they are not implemented)?

Yes.

Post by Felix Janda
Should "%T" be implemented?

I'd wait for somebody to complain about its absence.

Rob

[Felix Janda]

Post by Rob Landley

Post by Felix Janda

Post by Rob Landley
If you can come up with a clean way to share the code, go for it.

Ok, actually I'd only put something like get_access_str() of stat into
the lib.

Cool. Although a quick glance at the one in stat implies it doesn't
handle the fun combinations of "suid and sgid bits without
corresponding executable bit when applied to a directory so it's the
sticky bit" that I had to make work in ls.
stat /dev/null
stat /tmp
So if you actually need that behavior you'll need the blob of ls
starting around line 340...

The only thing get_access_str() printed apart from the permissions was
whether it's a directory or not. But I'm very certain that it should be
identical to ls. Thanks for the pointer to the relevant part of ls,
attached is a patch moving it into a library function.

Post by Rob Landley

Post by Felix Janda

Post by Rob Landley
I'm leaning against it. If you saw my toybox talk at ELC (it's on
youtube) I talked about security approaches and containers vs

selinux.
I anticipated that (and read your outline and skimmed the video). So it
would be ok to remove the SELinux parts from the command (which anyway
only say that they are not implemented)?

Yes.

Post by Felix Janda
Should "%T" be implemented?

I'd wait for somebody to complain about its absence.

I like that approach. Attached is another patch removing unimplemented
options/formats and doing a bit of cleanup (mainly of the help text,
which has some resemblance with the GNU manual page).

Felix
-------------- next part --------------
A non-text attachment was scrubbed...
Name: format-mode.patch
Type: text/x-patch
Size: 3963 bytes
Desc: not available
URL: <http://lists.landley.net/pipermail/toybox-landley.net/attachments/20130422/973af648/attachment.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: stat-help.patch
Type: text/x-patch
Size: 9163 bytes
Desc: not available
URL: <http://lists.landley.net/pipermail/toybox-landley.net/attachments/20130422/973af648/attachment-0001.bin>

[Rob Landley]

Post by Felix Janda

Post by Rob Landley

Post by Felix Janda
Should "%T" be implemented?

I'd wait for somebody to complain about its absence.

I like that approach. Attached is another patch removing unimplemented
options/formats and doing a bit of cleanup (mainly of the help text,
which has some resemblance with the GNU manual page).

I've lost the plot somewhere, neither of these patches apply to my
current tree. Last two commit to that file I have (from you) are:

changeset: 872:793972c94560
user: Felix Janda <felix.janda at posteo.de>
date: Sat Apr 20 00:18:17 2013 +0200
files: toys/pending/stat.c
description:
stat cleanup


changeset: 871:8aa07b575cd6
user: Felix Janda <felix.janda at posteo.de>
date: Fri Apr 19 21:29:36 2013 +0200
files: toys/pending/stat.c
description:
stat: Reindent from 4 to 2 spaces

When I "hg import -f" the attached files it's a bunch of rejected
hunks. What did I miss?

Rob

[Felix Janda]

Post by Rob Landley
I've lost the plot somewhere, neither of these patches apply to my
changeset: 872:793972c94560
user: Felix Janda <felix.janda at posteo.de>
date: Sat Apr 20 00:18:17 2013 +0200
files: toys/pending/stat.c
stat cleanup
changeset: 871:8aa07b575cd6
user: Felix Janda <felix.janda at posteo.de>
date: Fri Apr 19 21:29:36 2013 +0200
files: toys/pending/stat.c
stat: Reindent from 4 to 2 spaces
When I "hg import -f" the attached files it's a bunch of rejected
hunks. What did I miss?

I don't know. I just freshly hg cloned toybox, ran

hg import format-mode.patch
hg import stat-help.patch

and it seemed to work.

Felix

[Felix Janda]

Post by Rob Landley
I've lost the plot somewhere, neither of these patches apply to my
changeset: 872:793972c94560
user: Felix Janda <felix.janda at posteo.de>
date: Sat Apr 20 00:18:17 2013 +0200
files: toys/pending/stat.c
stat cleanup
changeset: 871:8aa07b575cd6
user: Felix Janda <felix.janda at posteo.de>
date: Fri Apr 19 21:29:36 2013 +0200
files: toys/pending/stat.c
stat: Reindent from 4 to 2 spaces
When I "hg import -f" the attached files it's a bunch of rejected
hunks. What did I miss?

I don't know. I just freshly hg cloned toybox, ran

hg import format-mode.patch
hg import stat-help.patch

and it seemed to work.

Felix

[Rob Landley]

Post by Felix Janda

Post by Rob Landley

Post by Felix Janda
Should "%T" be implemented?

I'd wait for somebody to complain about its absence.

I like that approach. Attached is another patch removing unimplemented
options/formats and doing a bit of cleanup (mainly of the help text,
which has some resemblance with the GNU manual page).

I've lost the plot somewhere, neither of these patches apply to my
current tree. Last two commit to that file I have (from you) are:

changeset: 872:793972c94560
user: Felix Janda <felix.janda at posteo.de>
date: Sat Apr 20 00:18:17 2013 +0200
files: toys/pending/stat.c
description:
stat cleanup


changeset: 871:8aa07b575cd6
user: Felix Janda <felix.janda at posteo.de>
date: Fri Apr 19 21:29:36 2013 +0200
files: toys/pending/stat.c
description:
stat: Reindent from 4 to 2 spaces

When I "hg import -f" the attached files it's a bunch of rejected
hunks. What did I miss?

Rob

[Felix Janda]

Post by Rob Landley

Post by Felix Janda

Post by Rob Landley
If you can come up with a clean way to share the code, go for it.

Ok, actually I'd only put something like get_access_str() of stat into
the lib.

Cool. Although a quick glance at the one in stat implies it doesn't
handle the fun combinations of "suid and sgid bits without
corresponding executable bit when applied to a directory so it's the
sticky bit" that I had to make work in ls.
stat /dev/null
stat /tmp
So if you actually need that behavior you'll need the blob of ls
starting around line 340...

The only thing get_access_str() printed apart from the permissions was
whether it's a directory or not. But I'm very certain that it should be
identical to ls. Thanks for the pointer to the relevant part of ls,
attached is a patch moving it into a library function.

Post by Rob Landley

Post by Felix Janda

Post by Rob Landley
I'm leaning against it. If you saw my toybox talk at ELC (it's on
youtube) I talked about security approaches and containers vs

selinux.
I anticipated that (and read your outline and skimmed the video). So it
would be ok to remove the SELinux parts from the command (which anyway
only say that they are not implemented)?

Yes.

Post by Felix Janda
Should "%T" be implemented?

I'd wait for somebody to complain about its absence.

I like that approach. Attached is another patch removing unimplemented
options/formats and doing a bit of cleanup (mainly of the help text,
which has some resemblance with the GNU manual page).

Felix
-------------- next part --------------
A non-text attachment was scrubbed...
Name: format-mode.patch
Type: text/x-patch
Size: 3963 bytes
Desc: not available
URL: <http://lists.landley.net/pipermail/toybox-landley.net/attachments/20130422/973af648/attachment-0004.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: stat-help.patch
Type: text/x-patch
Size: 9163 bytes
Desc: not available
URL: <http://lists.landley.net/pipermail/toybox-landley.net/attachments/20130422/973af648/attachment-0005.bin>

[Rob Landley]

Post by Felix Janda

Post by Rob Landley
I've lost the plot somewhere, neither of these patches apply to my
changeset: 872:793972c94560
user: Felix Janda <felix.janda at posteo.de>
date: Sat Apr 20 00:18:17 2013 +0200
files: toys/pending/stat.c
stat cleanup
changeset: 871:8aa07b575cd6
user: Felix Janda <felix.janda at posteo.de>
date: Fri Apr 19 21:29:36 2013 +0200
files: toys/pending/stat.c
stat: Reindent from 4 to 2 spaces
When I "hg import -f" the attached files it's a bunch of rejected
hunks. What did I miss?

I don't know. I just freshly hg cloned toybox, ran
hg import format-mode.patch
hg import stat-help.patch
and it seemed to work.

Found it, problem at my end.

Rob

[Felix Janda]

Hello,

attached are two patches cleaning up stat a bit. The first changes from
4 to 2 space indentation, the second is more interesting.

First of all it extends stat to correctly handle format strings.
(Previously it considered only the second char of the format string.)
This made it possible to remove duplication between print_stat_format()
and stat_main(). The option processing now uses the toybox infrastructure
better and by sorting the options in an appropriate way stat_main()
could be simplified greatly. get_access_str() is now no longer broken.
Apart from this there are various other small changes.

The functionality of get_access_str() is also implemented in the ls toy
(likely much better). Should something like it be in the library?
Actually since stat and ls are both frontends to the stat system call
more is duplicated.

I also wonder whether toybox will ever have SELinux support... My local
man page does even not document the -Z option to stat.

Felix
-------------- next part --------------
A non-text attachment was scrubbed...
Name: stat-reindent.patch
Type: text/x-patch
Size: 18002 bytes
Desc: not available
URL: <http://lists.landley.net/pipermail/toybox-landley.net/attachments/20130420/42012297/attachment-0004.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: stat-cleanup.patch
Type: text/x-patch
Size: 13564 bytes
Desc: not available
URL: <http://lists.landley.net/pipermail/toybox-landley.net/attachments/20130420/42012297/attachment-0005.bin>

[Rob Landley]

Post by Felix Janda
Hello,
attached are two patches cleaning up stat a bit. The first changes from
4 to 2 space indentation, the second is more interesting.
First of all it extends stat to correctly handle format strings.
(Previously it considered only the second char of the format string.)
This made it possible to remove duplication between
print_stat_format()
and stat_main(). The option processing now uses the toybox
infrastructure
better and by sorting the options in an appropriate way stat_main()
could be simplified greatly. get_access_str() is now no longer broken.
Apart from this there are various other small changes.

Cool, and applied.

Post by Felix Janda
The functionality of get_access_str() is also implemented in the ls toy
(likely much better). Should something like it be in the library?

Once code can be shared by a second command, putting it in lib makes
sense.

Post by Felix Janda
Actually since stat and ls are both frontends to the stat system call
more is duplicated.

If you can come up with a clean way to share the code, go for it.

I note that I have a pending todo item in ls: ls -l /dev/null doesn't
show the major/minor numbers. (Oops. Needs another output mode for
device nodes. If I did one, it's not triggering right...)

Post by Felix Janda
I also wonder whether toybox will ever have SELinux support... My local
man page does even not document the -Z option to stat.

I'm leaning against it. If you saw my toybox talk at ELC (it's on
youtube) I talked about security approaches and containers vs selinux.

Post by Felix Janda
Felix

Rob

[Rob Landley]

Post by Felix Janda

Post by Rob Landley
If you can come up with a clean way to share the code, go for it.

Ok, actually I'd only put something like get_access_str() of stat into
the lib.

Cool. Although a quick glance at the one in stat implies it doesn't
handle the fun combinations of "suid and sgid bits without
corresponding executable bit when applied to a directory so it's the
sticky bit" that I had to make work in ls.

stat /dev/null
stat /tmp

So if you actually need that behavior you'll need the blob of ls
starting around line 340...

Post by Felix Janda

Post by Rob Landley
I'm leaning against it. If you saw my toybox talk at ELC (it's on
youtube) I talked about security approaches and containers vs

selinux.
I anticipated that (and read your outline and skimmed the video). So
it
would be ok to remove the SELinux parts from the command (which anyway
only say that they are not implemented)?

Yes.

Post by Felix Janda
Should "%T" be implemented?

I'd wait for somebody to complain about its absence.

Rob

[Rob Landley]

Post by Felix Janda

Post by Rob Landley
I've lost the plot somewhere, neither of these patches apply to my
changeset: 872:793972c94560
user: Felix Janda <felix.janda at posteo.de>
date: Sat Apr 20 00:18:17 2013 +0200
files: toys/pending/stat.c
stat cleanup
changeset: 871:8aa07b575cd6
user: Felix Janda <felix.janda at posteo.de>
date: Fri Apr 19 21:29:36 2013 +0200
files: toys/pending/stat.c
stat: Reindent from 4 to 2 spaces
When I "hg import -f" the attached files it's a bunch of rejected
hunks. What did I miss?

I don't know. I just freshly hg cloned toybox, ran
hg import format-mode.patch
hg import stat-help.patch
and it seemed to work.

Found it, problem at my end.

Rob