Re: New procfs implementation

[Jeremie Koenig]

Hi,

On Wed, Sep 01, 2010 at 01:06:32AM +0200, Samuel Thibault wrote:
> >     { "anonymous-owner", 'a', "USER", 0,
> >     "Make USER the owner of files related to processes without one.  "
> >     "Be aware that USER will be granted access to the environment and "
> >     "other sensitive information about the processes in question.  "
> >     "(default: use uid 0)" },
> 
> Which use do you envision?

You may want to add an entry to /etc/passwd (say, "noone"), used only to
distinguish the anonymous processes from those owned by root, though as
the comment suggests you would have to be careful not to use it for
anything else.

> > procfs.c
> 
> is caching data really useful for anything but directories?
> I'm afraid of all the various re-reading patterns that tools may have.

Caching data seemed the only way to ensure that you get consistent
results if you read a file in multiple runs. As for re-reading, seeking
to 0 invalidates the cached contents.[1]

Since Linux only supports reading /proc files in one run, there are
probably not many other re-reading patterns to worry about, though
arguably this could also mean that we shouldn't worry about supporting
that either.

[1] This is not enough for process files, which build their contents
based on the associated, non-refreshed proc_stat structure, though 'top'
and 'htop' seem to work fine regardless (top definitely keeps
/proc/uptime open).

> procfs.c: About 42, 2 can be a better FIXME guess for now, since that's
> the root inode in ext2fs.

Okay, changed (ebf8ede).

> procfs_make_ino: this is not handling collisions. This can pose problems
> with e.g. tar-ing /proc with hardlink management (yes, I sometimes do
> such thing). I'm afraid it might be better to just assign known major
> numbers to the various content providers (yes, that's not elegant), let
> them handle minor numbers, and combine both. That would help to fill
> d_fileno.

I considered this, but ended up implementing the current solution first,
since it was quicker and kept the interface simple.

Instead of explicitely assigning inode numbers, we could make the
probability of collision negligibly small by extending the inode numbers
to 64 bits (but is there widespread userspace support for this?).

Also, since pid_t is 32-bits wide (at least in theory), dividing a
32-bits inode namespace among processes might prove somewhat tricky and
collision-prone as well.

> netfs_*: shouldn't we check the node type?

Such checks are already done in libnetfs; if some are missing I guess it
would be better to add them there. (As for read()-ing directories, it's
allowed by ext2fs too so I figured it would be okay.)

> rootdir.c: you have #define KERNEL_PID 2 while it was made a translator
> parameter in main.c, is it on purpose?  If so, it's probably worth
> documenting.

It's not really on purpose (it was initially INIT_PID and used for
uptime only). Changed (7f3a812).

Thanks for your comments,
-- 
Jeremie Koenig <jk@jk.fr.eu.org>
http://jk.fr.eu.org