Discussion:
[developer] Behavior of tail -f wrt truncation
Bryan Cantrill
2012-07-29 05:38:03 UTC
Permalink
All,

We have a bit of a thorny problem with tail -f. Currently, our tail -f
(and, it must be said, Solaris 11's) does not pick up a truncation of the
file. That is, if you're doing a tail -f of /tmp/foo, and /tmp/foo gets
truncated and subsequently written to, your tail won't print any output
until the size of /tmp/foo exceeds the size of the seek offset at the time
of truncation (and even then, you'll be missing the new contents of the
file before that point). This seems busted -- and it's not the way that
either BSD tail or GNU tail behave.

Now, we have a couple of options. GNU tail -f takes the obvious (but also
obviously flawed) approach of fstat()'ing the file every period of time
and comparing the st_size to the old st_size; if the st_size is less than
the old st_size, it (somewhat uselessly, in my opinion) prints out a
message ("tail: /tmp/foo: file truncated") and then (somewhat brokenly, in
my opinion) moves its seek offset to the new end of the file. (That is,
whatever was written to the file between truncation and the fstat() is
never printed.) Subsequent appends to the file, however, will (correctly)
result in output. Note that this approach leads to some very odd
behavior: if you're doing a tail -f of a file that has 10 bytes, and that
file is recreated (opened O_TRUNC) and then has 100 bytes written to it
immediately, you'll see only the last 90 bytes.

BSD (including OS X) tail -f takes the much better approach of using
kevent in such a way that it can determine when the file has been
truncated, adjusting its seek pointer appropriately. (It can only do this
when using the USE_PORT action -- which is why ours is busted.) This
means that BSD tail -f essentially always does the Right Thing: it will
catch file truncation and print the new contents, and if you truncate the
file and overwrite it with a size larger than the old size, it will
correctly print all of the new contents.

In terms of options for us: I think our tail -f behavior as it is
currently is pretty clearly busted, but we could easily implement the GNU
behavior (albeit without the error message and the additional semantic
oddity of not printing the new contents). However, I think that this
behavior still leaves a lot to be desired in that it gets so confused when
the size after truncation-and-write exceeds the size before truncation.

The second option is to take the BSD approach. This would take us back to
using event ports (which we ripped out as part of illumos#535), and we
would want to be mindful of the issues raised in illumos#535. In
particular, we would want to use PORT_SOURCE_FILE, not PORT_SOURCE_FD.
Even still, our existing functionality for PORT_SOURCE_FILE isn't _quite_
enough to get us there: PORT_SOURCE_FILE as it is today will tell us that
a file has been modified, but not that it's been truncated. However, it's
a very simple (and, in my opinion, reasonable) change to add a FILE_TRUNC
event (or similar) to the PORT_SOURCE_FILE events -- which would allow our
tail -f (and not to mention, any other PORT_SOURCE_FILE event port
consumer) to exactly match the BSD behavior. I think that this is my
preferred approach, but it's obviously a tad more involved.

Thoughts on any/all of this?

- Bryan


-------------------------------------------
illumos-developer
Archives: https://www.listbox.com/member/archive/182179/=now
RSS Feed: https://www.listbox.com/member/archive/rss/182179/21175072-c42b328b
Modify Your Subscription: https://www.listbox.com/member/?member_id=21175072&id_secret=21175072-605db409
Powered by Listbox: http://www.listbox.com
Joshua M. Clulow
2012-07-29 07:10:12 UTC
Permalink
Post by Bryan Cantrill
The second option is to take the BSD approach. This would take us back to
using event ports (which we ripped out as part of illumos#535), and we
would want to be mindful of the issues raised in illumos#535. In
particular, we would want to use PORT_SOURCE_FILE, not PORT_SOURCE_FD.
Even still, our existing functionality for PORT_SOURCE_FILE isn't _quite_
enough to get us there: PORT_SOURCE_FILE as it is today will tell us that
a file has been modified, but not that it's been truncated. However, it's
a very simple (and, in my opinion, reasonable) change to add a FILE_TRUNC
event (or similar) to the PORT_SOURCE_FILE events -- which would allow our
tail -f (and not to mention, any other PORT_SOURCE_FILE event port
consumer) to exactly match the BSD behavior. I think that this is my
preferred approach, but it's obviously a tad more involved.
So I just played with all three of: our, the GNU and the OS X tail.
The OS X tail was the only one of the three that didn't do anything
undesirable or surprising. If we can have that level of correctness
in our implementation with mostly existing infrastructure then I
certainly think it's worth the effort.

It would be good to amend the tail(1) man page to reflect that we
appear to have working -F support. I suspect that the use of
PORT_SOURCE_FILE events (viz. FILE_DELETE/FILE_RENAME*) may also
improve this mechanism.

I'm probably too busy in August to actually implement this myself but
I'll certainly try and assist with review/testing.
--
Joshua M. Clulow
UNIX Admin/Developer
http://blog.sysmgr.org


-------------------------------------------
illumos-developer
Archives: https://www.listbox.com/member/archive/182179/=now
RSS Feed: https://www.listbox.com/member/archive/rss/182179/21175072-c42b328b
Modify Your Subscription: https://www.listbox.com/member/?member_id=21175072&id_secret=21175072-605db409
Powered by Listbox: http://www.listbox.com
Garrett D'Amore
2012-07-29 09:35:21 UTC
Permalink
Post by Joshua M. Clulow
Post by Bryan Cantrill
The second option is to take the BSD approach. This would take us back to
using event ports (which we ripped out as part of illumos#535), and we
would want to be mindful of the issues raised in illumos#535. In
particular, we would want to use PORT_SOURCE_FILE, not PORT_SOURCE_FD.
Even still, our existing functionality for PORT_SOURCE_FILE isn't _quite_
enough to get us there: PORT_SOURCE_FILE as it is today will tell us that
a file has been modified, but not that it's been truncated. However, it's
a very simple (and, in my opinion, reasonable) change to add a FILE_TRUNC
event (or similar) to the PORT_SOURCE_FILE events -- which would allow our
tail -f (and not to mention, any other PORT_SOURCE_FILE event port
consumer) to exactly match the BSD behavior. I think that this is my
preferred approach, but it's obviously a tad more involved.
So I just played with all three of: our, the GNU and the OS X tail.
The OS X tail was the only one of the three that didn't do anything
undesirable or surprising. If we can have that level of correctness
in our implementation with mostly existing infrastructure then I
certainly think it's worth the effort.
It would be good to amend the tail(1) man page to reflect that we
appear to have working -F support. I suspect that the use of
PORT_SOURCE_FILE events (viz. FILE_DELETE/FILE_RENAME*) may also
improve this mechanism.
I'm probably too busy in August to actually implement this myself but
I'll certainly try and assist with review/testing.
As one of the parties involved with our current tail, I do agree that Bryan's suggested approach seems sound.

I also wonder if POSIX has anything to say here (though I doubt it) -- this is a fairly strange (IMO) edge case.

Out of curiosity, how did you happen to notice this in the first place? I've never seen tail -f used in a circumstance where truncation was normal. (With log files, which get *rotated*, the old log file is moved out of the way, and subsequently compressed. I think this is done in a way that the tail program will wind up keeping a reference to the old uncompressed file, and hence will see it just as a file with no changes occurring. )

Given the history with 535, we want to be extra careful to test this to make sure we get it "right".

- Garrett



-------------------------------------------
illumos-developer
Archives: https://www.listbox.com/member/archive/182179/=now
RSS Feed: https://www.listbox.com/member/archive/rss/182179/21175072-c42b328b
Modify Your Subscription: https://www.listbox.com/member/?member_id=21175072&id_secret=21175072-605db409
Powered by Listbox: http://www.listbox.com
Gary Mills
2012-07-29 21:27:45 UTC
Permalink
Post by Garrett D'Amore
Out of curiosity, how did you happen to notice this in the first
place? I've never seen tail -f used in a circumstance where
truncation was normal. (With log files, which get *rotated*, the
old log file is moved out of the way, and subsequently compressed.
I think this is done in a way that the tail program will wind up
keeping a reference to the old uncompressed file, and hence will see
it just as a file with no changes occurring. )
That's the only circumstance in which I've noticed a `tail -f'
failure. Is there any reasonable way to make this work? I suppose it
would have to reopen the file by name.
--
-Gary Mills- -refurb- -Winnipeg, Manitoba, Canada-


-------------------------------------------
illumos-developer
Archives: https://www.listbox.com/member/archive/182179/=now
RSS Feed: https://www.listbox.com/member/archive/rss/182179/21175072-c42b328b
Modify Your Subscription: https://www.listbox.com/member/?member_id=21175072&id_secret=21175072-605db409
Powered by Listbox: http://www.listbox.com
Jim Klimov
2012-07-29 22:03:04 UTC
Permalink
Post by Gary Mills
Post by Garrett D'Amore
Out of curiosity, how did you happen to notice this in the first
place? I've never seen tail -f used in a circumstance where
truncation was normal. (With log files, which get *rotated*, the
old log file is moved out of the way, and subsequently compressed.
I think this is done in a way that the tail program will wind up
keeping a reference to the old uncompressed file, and hence will see
it just as a file with no changes occurring. )
That's the only circumstance in which I've noticed a `tail -f'
failure. Is there any reasonable way to make this work? I suppose it
would have to reopen the file by name.
That is, it would have to poll (or receive events) to check:
* if the inode corresponding to the filename remains the same
(a-la "gtail --follow"),
* if the file size is shorter than current seek location.

Kind of so?

//Jim


-------------------------------------------
illumos-developer
Archives: https://www.listbox.com/member/archive/182179/=now
RSS Feed: https://www.listbox.com/member/archive/rss/182179/21175072-c42b328b
Modify Your Subscription: https://www.listbox.com/member/?member_id=21175072&id_secret=21175072-605db409
Powered by Listbox: http://www.listbox.com
Joshua M. Clulow
2012-07-30 02:52:17 UTC
Permalink
Post by Jim Klimov
Post by Gary Mills
That's the only circumstance in which I've noticed a `tail -f'
failure. Is there any reasonable way to make this work? I suppose it
would have to reopen the file by name.
* if the inode corresponding to the filename remains the same
(a-la "gtail --follow"),
* if the file size is shorter than current seek location.
As I said earlier, we already appear to have working -F (follow?)
support that handles this case as a result of the original tail port
in #244. See:

http://src.illumos.org/source/xref/illumos-gate/usr/src/cmd/tail/tail.c#102

It's just not (yet) documented in the manpage.

Bryan, I noticed you just pushed a workaround for 6456558 (port_get
does not return with ETIME when timeout = 0) in node. Should we be
fixing /that/ bug prior to this one? Either way, the original event
ports code ripped out in #535 *did* make use of a timeout (for -F
support) with port_get() so whoever does this work should probably be
aware of that behaviour also.
--
Joshua M. Clulow
UNIX Admin/Developer
http://blog.sysmgr.org


-------------------------------------------
illumos-developer
Archives: https://www.listbox.com/member/archive/182179/=now
RSS Feed: https://www.listbox.com/member/archive/rss/182179/21175072-c42b328b
Modify Your Subscription: https://www.listbox.com/member/?member_id=21175072&id_secret=21175072-605db409
Powered by Listbox: http://www.listbox.com
Joshua M. Clulow
2012-07-30 02:54:31 UTC
Permalink
Post by Joshua M. Clulow
Bryan, I noticed you just pushed a workaround for 6456558 (port_get
does not return with ETIME when timeout = 0) in node. Should we be
fixing /that/ bug prior to this one? Either way, the original event
ports code ripped out in #535 *did* make use of a timeout (for -F
support) with port_get() so whoever does this work should probably be
aware of that behaviour also.
Sigh, I retract the question -- it wasn't a zeroed timeout. I need more coffee.
--
Joshua M. Clulow
UNIX Admin/Developer
http://blog.sysmgr.org


-------------------------------------------
illumos-developer
Archives: https://www.listbox.com/member/archive/182179/=now
RSS Feed: https://www.listbox.com/member/archive/rss/182179/21175072-c42b328b
Modify Your Subscription: https://www.listbox.com/member/?member_id=21175072&id_secret=21175072-605db409
Powered by Listbox: http://www.listbox.com
Bryan Cantrill
2012-07-30 03:17:29 UTC
Permalink
Post by Joshua M. Clulow
Post by Joshua M. Clulow
Bryan, I noticed you just pushed a workaround for 6456558 (port_get
does not return with ETIME when timeout = 0) in node. Should we be
fixing /that/ bug prior to this one? Either way, the original event
ports code ripped out in #535 *did* make use of a timeout (for -F
support) with port_get() so whoever does this work should probably be
aware of that behaviour also.
Sigh, I retract the question -- it wasn't a zeroed timeout. I need more coffee.
That's true, but we definitely need to fix 6456558, and it's on my
queue -- it's really unconscionable that this bug hasn't been fixed,
because the failure mode for port_get() in particular is terrible.
(Namely, it returns success without filling in the port_event_t
because there is no event.) But (and as you observed), there isn't
actually a dependency between these two bugs...

- Bryan


-------------------------------------------
illumos-developer
Archives: https://www.listbox.com/member/archive/182179/=now
RSS Feed: https://www.listbox.com/member/archive/rss/182179/21175072-c42b328b
Modify Your Subscription: https://www.listbox.com/member/?member_id=21175072&id_secret=21175072-605db409
Powered by Listbox: http://www.listbox.com
Nicholas George
2012-07-30 05:03:19 UTC
Permalink
I'm pretty sure -F shows the issue that he's talking about (assuming I
understood it correctly).

From one console window (tail run first):
***@chise:~$ tail -F moo.txt
tail: cannot open `moo.txt' for reading: No such file or directory
tail: `moo.txt' has appeared; following end of new file
Moo

ooo


Then from a second console window (this all occurs after I start tail):
***@chise:~$ echo "Moo" > moo.txt
***@chise:~$ echo "Mooo" > moo.txt
***@chise:~$ echo "Mooooooo" > moo.txt

This is definitely not what I would have expected :-D

I can't check a linux system right now to see if it does the same thing.

Nick
Post by Joshua M. Clulow
Post by Jim Klimov
Post by Gary Mills
That's the only circumstance in which I've noticed a `tail -f'
failure. Is there any reasonable way to make this work? I suppose it
would have to reopen the file by name.
* if the inode corresponding to the filename remains the same
(a-la "gtail --follow"),
* if the file size is shorter than current seek location.
As I said earlier, we already appear to have working -F (follow?)
support that handles this case as a result of the original tail port
http://src.illumos.org/source/xref/illumos-gate/usr/src/cmd/tail/tail.c#102
It's just not (yet) documented in the manpage.
Bryan, I noticed you just pushed a workaround for 6456558 (port_get
does not return with ETIME when timeout = 0) in node. Should we be
fixing /that/ bug prior to this one? Either way, the original event
ports code ripped out in #535 *did* make use of a timeout (for -F
support) with port_get() so whoever does this work should probably be
aware of that behaviour also.
-------------------------------------------
illumos-developer
Archives: https://www.listbox.com/member/archive/182179/=now
RSS Feed: https://www.listbox.com/member/archive/rss/182179/21175072-c42b328b
Modify Your Subscription: https://www.listbox.com/member/?member_id=21175072&id_secret=21175072-605db409
Powered by Listbox: http://www.listbox.com
Bryan Cantrill
2012-07-30 03:17:19 UTC
Permalink
Post by Garrett D'Amore
Post by Joshua M. Clulow
Post by Bryan Cantrill
The second option is to take the BSD approach. This would take us back to
using event ports (which we ripped out as part of illumos#535), and we
would want to be mindful of the issues raised in illumos#535. In
particular, we would want to use PORT_SOURCE_FILE, not PORT_SOURCE_FD.
Even still, our existing functionality for PORT_SOURCE_FILE isn't _quite_
enough to get us there: PORT_SOURCE_FILE as it is today will tell us that
a file has been modified, but not that it's been truncated. However, it's
a very simple (and, in my opinion, reasonable) change to add a FILE_TRUNC
event (or similar) to the PORT_SOURCE_FILE events -- which would allow our
tail -f (and not to mention, any other PORT_SOURCE_FILE event port
consumer) to exactly match the BSD behavior. I think that this is my
preferred approach, but it's obviously a tad more involved.
So I just played with all three of: our, the GNU and the OS X tail.
The OS X tail was the only one of the three that didn't do anything
undesirable or surprising. If we can have that level of correctness
in our implementation with mostly existing infrastructure then I
certainly think it's worth the effort.
It would be good to amend the tail(1) man page to reflect that we
appear to have working -F support. I suspect that the use of
PORT_SOURCE_FILE events (viz. FILE_DELETE/FILE_RENAME*) may also
improve this mechanism.
I'm probably too busy in August to actually implement this myself but
I'll certainly try and assist with review/testing.
As one of the parties involved with our current tail, I do agree that Bryan's suggested approach seems sound.
Excellent -- will proceed accordingly.
Post by Garrett D'Amore
I also wonder if POSIX has anything to say here (though I doubt it) -- this is a fairly strange (IMO) edge case.
I should have mentioned that POSIX (not surprisingly) says nothing on
the subject of file truncation, dedicating many more words to
discussing the rationale for having -f be ignored if the input is a
pipe. (POSIX does, however, explicitly encourage using superior
mechanisms to sleeping on new data: "The -f option has been
implemented as a loop that sleeps for 1 second and copies any bytes
that are available. This is sufficient, but if more efficient methods
of determining when new data are available are developed,
implementations are encouraged to use them.")
Post by Garrett D'Amore
Out of curiosity, how did you happen to notice this in the first place? I've never seen tail -f used in a circumstance where truncation was normal. (With log files, which get *rotated*, the old log file is moved out of the way, and subsequently compressed. I think this is done in a way that the tail program will wind up keeping a reference to the old uncompressed file, and hence will see it just as a file with no changes occurring. )
We ran into this with software that is monitoring a log file that
wasn't rotated but rather periodically truncated. (For the rotation
case, we're probably better off using -F anyway.)
Post by Garrett D'Amore
Given the history with 535, we want to be extra careful to test this to make sure we get it "right".
Yes, you bet!

- Bryan


-------------------------------------------
illumos-developer
Archives: https://www.listbox.com/member/archive/182179/=now
RSS Feed: https://www.listbox.com/member/archive/rss/182179/21175072-c42b328b
Modify Your Subscription: https://www.listbox.com/member/?member_id=21175072&id_secret=21175072-605db409
Powered by Listbox: http://www.listbox.com
Bryan Cantrill
2012-08-04 00:11:36 UTC
Permalink
Post by Bryan Cantrill
Post by Garrett D'Amore
Post by Joshua M. Clulow
Post by Bryan Cantrill
The second option is to take the BSD approach. This would take us back to
using event ports (which we ripped out as part of illumos#535), and we
would want to be mindful of the issues raised in illumos#535. In
particular, we would want to use PORT_SOURCE_FILE, not PORT_SOURCE_FD.
Even still, our existing functionality for PORT_SOURCE_FILE isn't _quite_
enough to get us there: PORT_SOURCE_FILE as it is today will tell us that
a file has been modified, but not that it's been truncated. However, it's
a very simple (and, in my opinion, reasonable) change to add a FILE_TRUNC
event (or similar) to the PORT_SOURCE_FILE events -- which would allow our
tail -f (and not to mention, any other PORT_SOURCE_FILE event port
consumer) to exactly match the BSD behavior. I think that this is my
preferred approach, but it's obviously a tad more involved.
So I just played with all three of: our, the GNU and the OS X tail.
The OS X tail was the only one of the three that didn't do anything
undesirable or surprising. If we can have that level of correctness
in our implementation with mostly existing infrastructure then I
certainly think it's worth the effort.
It would be good to amend the tail(1) man page to reflect that we
appear to have working -F support. I suspect that the use of
PORT_SOURCE_FILE events (viz. FILE_DELETE/FILE_RENAME*) may also
improve this mechanism.
I'm probably too busy in August to actually implement this myself but
I'll certainly try and assist with review/testing.
As one of the parties involved with our current tail, I do agree that Bryan's suggested approach seems sound.
Excellent -- will proceed accordingly.
I just wanted to follow up that I've done this work and pushed it to SmartOS:

https://github.com/joyent/illumos-joyent/commit/12ef7bd6596eedcaafd544ec675d6b72dfd15998

(I'm happy to get a webrev on cr.illumos.org if people would prefer
it.) Note that this work was marginally more complicated than I'd
hoped due to bugs in tmpfs and NFS (they didn't properly emit the
create event), but the diffs are still pretty modest. Also, note that
I've fleshed out the tests quite a bit, and verified that our behavior
now matches that of BSD (I modified the test to run successfully on
BSD). Please let me know if you have any comments, with my thanks in
advance!

Thanks,
Bryan


-------------------------------------------
illumos-developer
Archives: https://www.listbox.com/member/archive/182179/=now
RSS Feed: https://www.listbox.com/member/archive/rss/182179/21175072-c42b328b
Modify Your Subscription: https://www.listbox.com/member/?member_id=21175072&id_secret=21175072-605db409
Powered by Listbox: http://www.listbox.com
Joshua M. Clulow
2012-08-04 04:28:28 UTC
Permalink
Post by Bryan Cantrill
https://github.com/joyent/illumos-joyent/commit/12ef7bd6596eedcaafd544ec675d6b72dfd15998
(I'm happy to get a webrev on cr.illumos.org if people would prefer
it.)
A webrev would be neat if it's not too much trouble, but I've
started looking anyway. Thus far, I have:

tailtests.sh:
25-28:
Looks like this should use $actual and $output, not $a and $o.

forward.c:
290-313:
This means correct functioning of tail(1) now depends on /proc,
right? Are there valid reasons not to have /proc available
(e.g. a limited chroot environment) and if so, should this
dependency be documented in the man page?

Could we perhaps extend portfs, etc, to provide something like
PORT_SOURCE_FILE_AT where we use f_vnode from a file descriptor,
rather than looking it up by path?
--
Joshua M. Clulow
UNIX Admin/Developer
http://blog.sysmgr.org


-------------------------------------------
illumos-developer
Archives: https://www.listbox.com/member/archive/182179/=now
RSS Feed: https://www.listbox.com/member/archive/rss/182179/21175072-c42b328b
Modify Your Subscription: https://www.listbox.com/member/?member_id=21175072&id_secret=21175072-605db409
Powered by Listbox: http://www.listbox.com
Garrett D'Amore
2012-08-04 08:45:25 UTC
Permalink
Post by Joshua M. Clulow
Post by Bryan Cantrill
https://github.com/joyent/illumos-joyent/commit/12ef7bd6596eedcaafd544ec675d6b72dfd15998
(I'm happy to get a webrev on cr.illumos.org if people would prefer
it.)
A webrev would be neat if it's not too much trouble, but I've
Looks like this should use $actual and $output, not $a and $o.
This means correct functioning of tail(1) now depends on /proc,
right? Are there valid reasons not to have /proc available
(e.g. a limited chroot environment) and if so, should this
dependency be documented in the man page?
I've not looked at the code… but I definitely don't like making tail depend on /proc. It breaks chroot, and mazy break some zones if they don't arrange for /proc in the non-global zone (not sure what else would break in such an environment, since usually /proc *is* available in NGZs.) However, if the new functionality depends on it, and we failed back to older functionality in its absence, that would be completely reasonable.

- Garrett
Post by Joshua M. Clulow
Could we perhaps extend portfs, etc, to provide something like
PORT_SOURCE_FILE_AT where we use f_vnode from a file descriptor,
rather than looking it up by path?
--
Joshua M. Clulow
UNIX Admin/Developer
http://blog.sysmgr.org
-------------------------------------------
illumos-developer
Archives: https://www.listbox.com/member/archive/182179/=now
RSS Feed: https://www.listbox.com/member/archive/rss/182179/21239177-c925e33f
Modify Your Subscription: https://www.listbox.com/member/?&
Powered by Listbox: http://www.listbox.com
-------------------------------------------
illumos-developer
Archives: https://www.listbox.com/member/archive/182179/=now
RSS Feed: https://www.listbox.com/member/archive/rss/182179/21175072-c42b328b
Modify Your Subscription: https://www.listbox.com/member/?member_id=21175072&id_secret=21175072-605db409
Powered by Listbox: http://www.listbox.com
Bryan Cantrill
2012-08-04 16:25:00 UTC
Permalink
Post by Garrett D'Amore
Post by Joshua M. Clulow
Post by Bryan Cantrill
https://github.com/joyent/illumos-joyent/commit/12ef7bd6596eedcaafd544ec675d6b72dfd15998
(I'm happy to get a webrev on cr.illumos.org if people would prefer
it.)
A webrev would be neat if it's not too much trouble, but I've
Looks like this should use $actual and $output, not $a and $o.
This means correct functioning of tail(1) now depends on /proc,
right? Are there valid reasons not to have /proc available
(e.g. a limited chroot environment) and if so, should this
dependency be documented in the man page?
I've not looked at the code… but I definitely don't like making tail depend on /proc. It breaks chroot, and mazy break some zones if they don't arrange for /proc in the non-global zone (not sure what else would break in such an environment, since usually /proc *is* available in NGZs.) However, if the new functionality depends on it, and we failed back to older functionality in its absence, that would be completely reasonable.
The notion that a non-global zone could so much as boot without /proc
is demonstrably false: svc.startd and svcadm both have hard
dependencies on /proc -- to say nothing of the process-based tools
that are invoked during zone boot (e.g. pgrep). And it doesn't end
there -- anything that depends on reading the aux vector also has a
hard dependency on /proc, and this includes getexecname(), itself
called by isaexec. The new dependency of "tail -f" on /proc would be
the least of the problems of anyone who didn't have access to it.

I don't object to having it fall back on the old implementation in the
unlikely event that /proc is not available, but let's be clear about
the many, many dependencies on /proc that already exist among common
utilities in the system.

- Bryan


-------------------------------------------
illumos-developer
Archives: https://www.listbox.com/member/archive/182179/=now
RSS Feed: https://www.listbox.com/member/archive/rss/182179/21175072-c42b328b
Modify Your Subscription: https://www.listbox.com/member/?member_id=21175072&id_secret=21175072-605db409
Powered by Listbox: http://www.listbox.com
Garrett D'Amore
2012-08-04 16:47:27 UTC
Permalink
Post by Bryan Cantrill
Post by Garrett D'Amore
Post by Joshua M. Clulow
Post by Bryan Cantrill
https://github.com/joyent/illumos-joyent/commit/12ef7bd6596eedcaafd544ec675d6b72dfd15998
(I'm happy to get a webrev on cr.illumos.org if people would prefer
it.)
A webrev would be neat if it's not too much trouble, but I've
Looks like this should use $actual and $output, not $a and $o.
This means correct functioning of tail(1) now depends on /proc,
right? Are there valid reasons not to have /proc available
(e.g. a limited chroot environment) and if so, should this
dependency be documented in the man page?
I've not looked at the code… but I definitely don't like making tail depend on /proc. It breaks chroot, and mazy break some zones if they don't arrange for /proc in the non-global zone (not sure what else would break in such an environment, since usually /proc *is* available in NGZs.) However, if the new functionality depends on it, and we failed back to older functionality in its absence, that would be completely reasonable.
The notion that a non-global zone could so much as boot without /proc
is demonstrably false: svc.startd and svcadm both have hard
dependencies on /proc -- to say nothing of the process-based tools
that are invoked during zone boot (e.g. pgrep). And it doesn't end
there -- anything that depends on reading the aux vector also has a
hard dependency on /proc, and this includes getexecname(), itself
called by isaexec. The new dependency of "tail -f" on /proc would be
the least of the problems of anyone who didn't have access to it.
I don't object to having it fall back on the old implementation in the
unlikely event that /proc is not available, but let's be clear about
the many, many dependencies on /proc that already exist among common
utilities in the system.
Thanks for the explanation. That actually makes really good sense. I suspected there were other dependencies on /proc.

What about chroot jails though? I realize this is a stretch, but do people create those without /proc? (And expect arbitrary POSIX tools to work in them?) Its been ages since I worried about chroot jails, and mostly the only time I've used them is for anonymous FTP servers, but I'd imagine that I could still build such a jail.

This all sounds somewhat contrived at this point.

So, if its trivial or near trivial, to retain the fallback behavior (and also that doesn't require keeping code around that you were otherwise going to delete), then I'd allow for it. But if it requires anything more than trivial effort on your part, or it requires keeping around a lot of code that would otherwise be deleted, then I'd just scrap the legacy implementation.

- Garrett




-------------------------------------------
illumos-developer
Archives: https://www.listbox.com/member/archive/182179/=now
RSS Feed: https://www.listbox.com/member/archive/rss/182179/21175072-c42b328b
Modify Your Subscription: https://www.listbox.com/member/?member_id=21175072&id_secret=21175072-605db409
Powered by Listbox: http://www.listbox.com
Gordon Ross
2012-08-04 18:33:46 UTC
Permalink
On Sat, Aug 4, 2012 at 12:47 PM, Garrett D'Amore <***@damore.org> wrote:
[...]
Post by Garrett D'Amore
Thanks for the explanation. That actually makes really good sense. I suspected there were other dependencies on /proc.
What about chroot jails though? I realize this is a stretch, but do people create those without /proc? (And expect arbitrary POSIX tools to work in them?) Its been ages since I worried about chroot jails, and mostly the only time I've used them is for anonymous FTP servers, but I'd imagine that I could still build such a jail.
This all sounds somewhat contrived at this point.
So, if its trivial or near trivial, to retain the fallback behavior (and also that doesn't require keeping code around that you were otherwise going to delete), then I'd allow for it. But if it requires anything more than trivial effort on your part, or it requires keeping around a lot of code that would otherwise be deleted, then I'd just scrap the legacy implementation.
- Garrett
We use some things that run in chroot jails. We have no /proc mount
in those jails,
and yes, a few things fail in such an environment. It is unfortunate
how much is
needed to make a full-fledged chroot jail, but as Bryan describes, I
suppose we're
already pretty far down that road (of requiring lots o' stuff like
/proc in a jail).
So I'd say, go ahead and introduce the dependency if the trade-off warrants.
In this case I guess it does, even if architecturally, it's not pretty.

Perhaps file a feature request inviting someone to come up with something
architecturally more attractive than using /proc/fd/N here?
I would not hold up the current integration for this.

BTW, a loopback mount of /proc into the jail works OK, when needed.
It just so happened that nothing we run chroot today needs /proc.
--
Gordon Ross <***@nexenta.com>
Nexenta Systems, Inc. www.nexenta.com
Enterprise class storage for everyone


-------------------------------------------
illumos-developer
Archives: https://www.listbox.com/member/archive/182179/=now
RSS Feed: https://www.listbox.com/member/archive/rss/182179/21175072-c42b328b
Modify Your Subscription: https://www.listbox.com/member/?member_id=21175072&id_secret=21175072-605db409
Powered by Listbox: http://www.listbox.com
Jens Elkner
2012-08-04 19:40:38 UTC
Permalink
On Sat, Aug 04, 2012 at 09:47:27AM -0700, Garrett D'Amore wrote:

1st: IMHO this is a huge improvement for admins and probably developers
as well! So thanx a lot for fixing the ancient/annyoing behavior ... :-)
...
Post by Bryan Cantrill
I don't object to having it fall back on the old implementation in the
unlikely event that /proc is not available, but let's be clear about
the many, many dependencies on /proc that already exist among common
utilities in the system.
...
What about chroot jails though? I realize this is a stretch, but do people create those without /proc? (And expect arbitrary POSIX tools to work in them?)
IMHO applies to people, which use Solaris < 10, other OS or very old
stuff, which shouldn't be used anymore/anyway. People using a modern OS
and need a jail should use a zone - the earth is still spinning ;-)
Its been ages since I worried about chroot jails, and mostly the only time I've used them is for anonymous FTP servers
but I'd imagine that I could still build such a jail.
Yes, but who wants/needs this pain in the ass?

...
So, if its trivial or near trivial, to retain the fallback behavior...
Hmmm, I think, if one is using a strange jail setup, he does/can do the
management stuff from outside the jail - and thus there is no need to
keep legacy stuff/bloat ...

Have fun,
jel.
--
Otto-von-Guericke University http://www.cs.uni-magdeburg.de/
Department of Computer Science Geb. 29 R 027, Universitaetsplatz 2
39106 Magdeburg, Germany Tel: +49 391 67 12768


-------------------------------------------
illumos-developer
Archives: https://www.listbox.com/member/archive/182179/=now
RSS Feed: https://www.listbox.com/member/archive/rss/182179/21175072-c42b328b
Modify Your Subscription: https://www.listbox.com/member/?member_id=21175072&id_secret=21175072-605db409
Powered by Listbox: http://www.listbox.com
DavidHalko
2012-08-04 20:45:47 UTC
Permalink
The use of tail -f is hardly management stuff. It is often used for queuing at the shell level and breaking up data streams written by regular applications into smaller files, to be transferred via other means.

That means of transfer, for inter-organizational communication is basically FTP or other standard file transfer mechanism. When providing this kind of access, chroot is very nice, providing only critical file transfer libraries, in case of a hacking.

The user community decides when this is preferable to zones, no reason to remove capability for well known use cases because it is well established.

In other words, chroot, ftp, tail -f are important glue which are well used in the industry. And yes, they have been used together without proc.

Breaking working code, used as a well documented and well used ipc, to accommodate bad file rotation practice (i.e. truncation instead of moving) is crazy in my mind!

Thanks, Dave

Sent from my iPhone
Post by Jens Elkner
1st: IMHO this is a huge improvement for admins and probably developers
as well! So thanx a lot for fixing the ancient/annyoing behavior ... :-)
...
Post by Bryan Cantrill
I don't object to having it fall back on the old implementation in the
unlikely event that /proc is not available, but let's be clear about
the many, many dependencies on /proc that already exist among common
utilities in the system.
...
What about chroot jails though? I realize this is a stretch, but do people create those without /proc? (And expect arbitrary POSIX tools to work in them?)
IMHO applies to people, which use Solaris < 10, other OS or very old
stuff, which shouldn't be used anymore/anyway. People using a modern OS
and need a jail should use a zone - the earth is still spinning ;-)
Its been ages since I worried about chroot jails, and mostly the only time I've used them is for anonymous FTP servers
but I'd imagine that I could still build such a jail.
Yes, but who wants/needs this pain in the ass?
...
So, if its trivial or near trivial, to retain the fallback behavior...
Hmmm, I think, if one is using a strange jail setup, he does/can do the
management stuff from outside the jail - and thus there is no need to
keep legacy stuff/bloat ...
Have fun,
jel.
--
Otto-von-Guericke University http://www.cs.uni-magdeburg.de/
Department of Computer Science Geb. 29 R 027, Universitaetsplatz 2
39106 Magdeburg, Germany Tel: +49 391 67 12768
-------------------------------------------
illumos-developer
Archives: https://www.listbox.com/member/archive/182179/=now
RSS Feed: https://www.listbox.com/member/archive/rss/182179/21175352-313cabda
Modify Your Subscription: https://www.listbox.com/member/?&
Powered by Listbox: http://www.listbox.com
-------------------------------------------
illumos-developer
Archives: https://www.listbox.com/member/archive/182179/=now
RSS Feed: https://www.listbox.com/member/archive/rss/182179/21175072-c42b328b
Modify Your Subscription: https://www.listbox.com/member/?member_id=21175072&id_secret=21175072-605db409
Powered by Listbox: http://www.listbox.com
Jens Elkner
2012-08-04 23:09:03 UTC
Permalink
On Sat, Aug 04, 2012 at 04:45:47PM -0400, DavidHalko wrote:
Hi David,
Post by DavidHalko
The use of tail -f is hardly management stuff. It is often used for queuing at the shell level and breaking up data streams written by regular applications into smaller files, to be transferred via other means.
Hmmm, I've never seen any ill non-management app, which uses tail in the
background to provide data. So wrt. my experience, this argument has no
practical relevance. But in theory you might be right.
Post by DavidHalko
That means of transfer, for inter-organizational communication is basically FTP or other standard file transfer mechanism. When providing this kind of access, chroot is very nice, providing only critical file transfer libraries, in case of a hacking.
Well, if one has conerns about those things, IMHO he shouldn't even
think about jails! If he doesn't want to catchup with recent
developments, he can keep running his old OS and ancient/ill stuff w/o
preventing cool stuff from making progress ;-)
Post by DavidHalko
The user community decides when this is preferable to zones, no reason to remove capability for well known use cases because it is well established.
In other words, chroot, ftp, tail -f are important glue which are well used in the industry. And yes, they have been used together without proc.
You are talking about the "never change a running system" industry,
right? They tend to not upgrade their OS, so why care about it?
BTW: Many software does/can do a chroot w/o the need to have a
[full blown] jail (i.e. "local" libs, /proc and bla) there, even FTP
servers, sure. But if one [mis]configures them to depend on "jail" stuff,
ill scripts (which use "tools", depending e.g. on /proc) it's really
time to start to think about it ...
Post by DavidHalko
Breaking working code, used as a well documented and well used ipc, to accommodate bad file rotation practice (i.e. truncation instead of moving) is crazy in my mind!
Hmmm, I guess, you miss the point...

Anyway, don't wan't to start a religious war. Just want to point out,
that all the "please keep this old stuff, because in theory strange
things could use it" w/o providing a reasonable example/use case with
practical relevance and preventing progress makes me sick ... ;-)

My 2¢.

Prost,
jel.
Post by DavidHalko
Sent from my iPhone
Sent from a computer
--
Otto-von-Guericke University http://www.cs.uni-magdeburg.de/
Department of Computer Science Geb. 29 R 027, Universitaetsplatz 2
39106 Magdeburg, Germany Tel: +49 391 67 12768


-------------------------------------------
illumos-developer
Archives: https://www.listbox.com/member/archive/182179/=now
RSS Feed: https://www.listbox.com/member/archive/rss/182179/21175072-c42b328b
Modify Your Subscription: https://www.listbox.com/member/?member_id=21175072&id_secret=21175072-605db409
Powered by Listbox: http://www.listbox.com
DavidHalko
2012-08-05 13:41:27 UTC
Permalink
Hi Jel,

Sent from my iPhone
Post by Jens Elkner
Hi David,
Post by DavidHalko
The use of tail -f is hardly management stuff. It is often used for queuing at the shell level and breaking up data streams written by regular applications into smaller files, to be transferred via other means.
Hmmm, I've never seen any ill non-management app, which uses tail in the
background to provide data. So wrt. my experience, this argument has no
practical relevance. But in theory you might be right.
I have produced 3 apps in the past 3 years that uses tail -f as a way of splitting data produced by a log of existing commercial apps.
Post by Jens Elkner
Post by DavidHalko
That means of transfer, for inter-organizational communication is basically FTP or other standard file transfer mechanism. When providing this kind of access, chroot is very nice, providing only critical file transfer libraries, in case of a hacking.
Well, if one has conerns about those things, IMHO he shouldn't even
think about jails! If he doesn't want to catchup with recent
developments, he can keep running his old OS and ancient/ill stuff w/o
preventing cool stuff from making progress ;-)
Legacy apps used for inter-company communication requires dozens of companies to find the funding simultaneously to do menial tasks like moving tickets between ticketing systems or alerts from on-site pollers to ticketing systems.
Post by Jens Elkner
Post by DavidHalko
The user community decides when this is preferable to zones, no reason to remove capability for well known use cases because it is well established.
In other words, chroot, ftp, tail -f are important glue which are well used in the industry. And yes, they have been used together without proc.
You are talking about the "never change a running system" industry,
right?
Telecommunications is an important industry. Network Management is the backbone of every major organization. So is Help Desk / Service Desk - trouble tickets have to flow between partners. Standards in these industry are paramount; vague standards result in behaviors which must be maintained (or the OS drops out of favorability) because the application supporting the business comes first.
Post by Jens Elkner
They tend to not upgrade their OS, so why care about it?
Hardware dies. Hardware requires spare parts. OS versions fall off maintenance. Security concerns require patches. If Illumos will not support legacy app behavior, it will never be selected as a replacement when old POSIX systems come of age.

Linux is the preferred replacement system for new systems, even though some level of glue rewrite is required - there is absolutely no reason to choose Illumos over Linux if both Linux & Illumos do not support legacy POSIX system behaviors. Illumos must make the case that they offer closer to compatibility.
Post by Jens Elkner
BTW: Many software does/can do a chroot w/o the need to have a
[full blown] jail (i.e. "local" libs, /proc and bla) there, even FTP
servers, sure. But if one [mis]configures them to depend on "jail" stuff,
ill scripts (which use "tools", depending e.g. on /proc) it's really
time to start to think about it ...
Post by DavidHalko
Breaking working code, used as a well documented and well used ipc, to accommodate bad file rotation practice (i.e. truncation instead of moving) is crazy in my mind!
Hmmm, I guess, you miss the point...
I guess so. I see no benefit in adding a third behavior to a basic command like tail, depending on whether it runs outside of a zone, without an explicit switch.

App developers should be completely insulated from the fact that an app is in a zone or not - adding another contingency differentiation is just the opposite of where an OS should be going. If people want a different behavior, make an environment variable (BSDCOMPAT=tail:ls:...) or a new flag (-tf to follow truncated files like BSD using proc, note in the man page that the new behavior does not run in a zone.)
Post by Jens Elkner
Anyway, don't wan't to start a religious war. Just want to point out,
that all the "please keep this old stuff, because in theory strange
things could use it" w/o providing a reasonable example/use case with
practical relevance and preventing progress makes me sick ... ;-)
If I did not use tail -f every day in apps I use, to glue commercial apps together where I don't have the source code, I would not have brought it up.

Thanks, Dave
http://netmgt.blogspot.com/
Post by Jens Elkner
My 2¢.
Prost,
jel.
Post by DavidHalko
Sent from my iPhone
Sent from a computer
--
Otto-von-Guericke University http://www.cs.uni-magdeburg.de/
-------------------------------------------
illumos-developer
Archives: https://www.listbox.com/member/archive/182179/=now
RSS Feed: https://www.listbox.com/member/archive/rss/182179/21175072-c42b328b
Modify Your Subscription: https://www.listbox.com/member/?member_id=21175072&id_secret=21175072-605db409
Powered by Listbox: http://www.listbox.com
Garrett D'Amore
2012-08-05 15:05:25 UTC
Permalink
Post by DavidHalko
Linux is the preferred replacement system for new systems, even though some level of glue rewrite is required - there is absolutely no reason to choose Illumos over Linux if both Linux & Illumos do not support legacy POSIX system behaviors. Illumos must make the case that they offer closer to compatibility.
I take serious objection to that. If the only reason we exist is to provide compatibility for a closed or legacy platform, then we have already lost.

But I don't concede defeat at all. In fact, a lot of people are moving *from* Linux to illumos solutions because of the superior capabilities of illumos:

- ZFS (yes there is a linux port, but it is second class)
- DTrace
- mdb/kmdb
- Zones
- KVM (also on Linux, but KVM on Zones providers better isolation)
- Crossbow

And we've not stopped innovating.

So for the record, I view support for legacy apps as a goal, but not the prime directive. The prime directive for illumos is to continue to innovate and *grow* the illumos community.

I always said we wanted ABI compatibility. I have since conceded that in some cases (Sun C++ for example) this might well be impossible. I still think we can avoid arbitrary application breakage.

I am not convinced at all that *this* case represents breakage to any interesting applications -- and any breakage is easily rectified by just loopback mounting /proc into the jail. (Did *any* of the apps you discussed run inside a chroot'ed jail?)

FYI, I raised this compatibility concern initially, and Brian has more than adequately addressed it IMO.

He's also agreed to provide the fallback behavior (although I indicated that I thought this was unnecessary if the work to do that is non-trivial.)

- Garrett




-------------------------------------------
illumos-developer
Archives: https://www.listbox.com/member/archive/182179/=now
RSS Feed: https://www.listbox.com/member/archive/rss/182179/21175072-c42b328b
Modify Your Subscription: https://www.listbox.com/member/?member_id=21175072&id_secret=21175072-605db409
Powered by Listbox: http://www.listbox.com
Andrew M. Hettinger
2012-08-05 19:59:20 UTC
Permalink
Andrew Hettinger
http://Prominic.NET || ***@Prominic.NET
Tel: 866.339.3169 (toll free) -or- +1.217.356.2888 x.110 (int'l)
Fax: 866.372.3356 (toll free) -or- +1.217.356.3356 (int'l)
Post by DavidHalko
Post by DavidHalko
Linux is the preferred replacement system for new systems, even
though some level of glue rewrite is required - there is absolutely
no reason to choose Illumos over Linux if both Linux & Illumos do
not support legacy POSIX system behaviors. Illumos must make the
case that they offer closer to compatibility.
POSIX compliance should be a goal. This behavior is NOT defined by POSIX or
SUS (AFAICT). It looks like we are discussing a corner case, that the
standard does not define. If you can find where it DOES define it, I'll
happily stand corrected. For reference, here is the relevent section of the
standard:

http://pubs.opengroup.org/onlinepubs/007908799/xcu/tail.html
Post by DavidHalko
I take serious objection to that. If the only reason we exist is to
provide compatibility for a closed or legacy platform, then we have
already lost.
But I don't concede defeat at all. In fact, a lot of people are
moving *from* Linux to illumos solutions because of the superior
- ZFS (yes there is a linux port, but it is second class)
- DTrace
- mdb/kmdb
- Zones
- KVM (also on Linux, but KVM on Zones providers better isolation)
- Crossbow
And we've not stopped innovating.
I want to concur with this. I'm not moving legacy apps to Illumos, I'm
running new things on it. I am not alone.
Post by DavidHalko
So for the record, I view support for legacy apps as a goal, but not
the prime directive. The prime directive for illumos is to
continue to innovate and *grow* the illumos community.
There are many objectives, often in conflict. There must be a balance
between them, and as soon as one become the "prime directive," I think, all
is lost.
Post by DavidHalko
I always said we wanted ABI compatibility. I have since conceded
that in some cases (Sun C++ for example) this might well be
impossible.
That's a shame, too, but ultimately out of your/our control.
Post by DavidHalko
I still think we can avoid arbitrary application breakage.
And should.
Post by DavidHalko
I am not convinced at all that *this* case represents breakage to
any interesting applications -- and any breakage is easily rectified
by just loopback mounting /proc into the jail. (Did *any* of the
apps you discussed run inside a chroot'ed jail?)
FYI, I raised this compatibility concern initially, and Brian has
more than adequately addressed it IMO.
He's also agreed to provide the fallback behavior (although I
indicated that I thought this was unnecessary if the work to do that
is non-trivial.)
With fallback behavior, I think this should address any legacy apps. New
apps should be able to do one of:

1) loopback mount /proc
2) use a zone
3) use the legacy fallback behavior

I think that provides quite a number of options to users for what to do.
Post by DavidHalko
- Garrett
-------------------------------------------
illumos-developer
Archives: https://www.listbox.com/member/archive/182179/=now
RSS Feed: https://www.listbox.com/member/archive/rss/182179/21175072-c42b328b
Modify Your Subscription: https://www.listbox.com/member/?member_id=21175072&id_secret=21175072-605db409
Powered by Listbox: http://www.listbox.com
Bill Sommerfeld
2012-08-05 16:22:07 UTC
Permalink
Post by Garrett D'Amore
and any breakage is easily rectified by just loopback mounting /proc into the jail.
loopback-mounting all of /proc into a chroot jail may provide an escape
path out of the jail, or may allow processes in the jail to read
information from outside the chroot that they shouldn't be able to
access. No doubt there are ways to make this safer but it makes
analyzing the configuration harder.

I'd just use a zone, or a chroot inside a zone dedicated to just that
chroot.

It might make sense to provide a "/proc/self"-only mount option for
procfs for use cases like this.

- Bill


-------------------------------------------
illumos-developer
Archives: https://www.listbox.com/member/archive/182179/=now
RSS Feed: https://www.listbox.com/member/archive/rss/182179/21175072-c42b328b
Modify Your Subscription: https://www.listbox.com/member/?member_id=21175072&id_secret=21175072-605db409
Powered by Listbox: http://www.listbox.com
Bryan Cantrill
2012-08-06 16:21:56 UTC
Permalink
For whatever it's worth, I changed the code to gracefully fall back to
the old behavior is /proc is unavailable. So we can definitely feel
welcome -- but not obligated -- to stop talking about when and where
/proc is unavailable. ;)

- Bryan


-------------------------------------------
illumos-developer
Archives: https://www.listbox.com/member/archive/182179/=now
RSS Feed: https://www.listbox.com/member/archive/rss/182179/21175072-c42b328b
Modify Your Subscription: https://www.listbox.com/member/?member_id=21175072&id_secret=21175072-605db409
Powered by Listbox: http://www.listbox.com
Garrett D'Amore
2012-08-06 16:35:24 UTC
Permalink
Post by Bryan Cantrill
For whatever it's worth, I changed the code to gracefully fall back to
the old behavior is /proc is unavailable. So we can definitely feel
welcome -- but not obligated -- to stop talking about when and where
/proc is unavailable. ;)
Thanks, and sorry for opening that particular can of worms.

- Garrett
Post by Bryan Cantrill
- Bryan
-------------------------------------------
illumos-developer
Archives: https://www.listbox.com/member/archive/182179/=now
RSS Feed: https://www.listbox.com/member/archive/rss/182179/21239177-c925e33f
Modify Your Subscription: https://www.listbox.com/member/?&
Powered by Listbox: http://www.listbox.com
-------------------------------------------
illumos-developer
Archives: https://www.listbox.com/member/archive/182179/=now
RSS Feed: https://www.listbox.com/member/archive/rss/182179/21175072-c42b328b
Modify Your Subscription: https://www.listbox.com/member/?member_id=21175072&id_secret=21175072-605db409
Powered by Listbox: http://www.listbox.com
Gordon Ross
2012-08-06 04:31:44 UTC
Permalink
On Fri, Aug 3, 2012 at 8:11 PM, Bryan Cantrill <***@gmail.com> wrote:
[...]
Post by Bryan Cantrill
https://github.com/joyent/illumos-joyent/commit/12ef7bd6596eedcaafd544ec675d6b72dfd15998
(I'm happy to get a webrev on cr.illumos.org if people would prefer
it.) Note that this work was marginally more complicated than I'd
hoped due to bugs in tmpfs and NFS (they didn't properly emit the
create event), but the diffs are still pretty modest. Also, note that
I've fleshed out the tests quite a bit, and verified that our behavior
now matches that of BSD (I modified the test to run successfully on
BSD). Please let me know if you have any comments, with my thanks in
advance!
Thanks,
Bryan
Hi Bryan,

I tried out your changes this evening. Looks pretty good.
I did find and fix a bug with watching multiple files.
Attached is a fix relative to your work, and a new test.

Thanks,
--
Gordon Ross <***@nexenta.com>
Nexenta Systems, Inc. www.nexenta.com
Enterprise class storage for everyone
Joshua M. Clulow
2012-08-06 06:41:28 UTC
Permalink
Post by Gordon Ross
Post by Bryan Cantrill
https://github.com/joyent/illumos-joyent/commit/12ef7bd6596eedcaafd544ec675d6b72dfd15998
Please let me know if you have any comments, with my thanks in
advance!
I tried out your changes this evening. Looks pretty good.
I did find and fix a bug with watching multiple files.
Attached is a fix relative to your work, and a new test.
I also have a couple of fixes for the test script (variable names in
checktest() and to allow for the use of more than one flag) and some
extra language for the man page tail(1):

https://gist.github.com/2e575757015b8160484c


Cheers.
--
Joshua M. Clulow
UNIX Admin/Developer
http://blog.sysmgr.org


-------------------------------------------
illumos-developer
Archives: https://www.listbox.com/member/archive/182179/=now
RSS Feed: https://www.listbox.com/member/archive/rss/182179/21175072-c42b328b
Modify Your Subscription: https://www.listbox.com/member/?member_id=21175072&id_secret=21175072-605db409
Powered by Listbox: http://www.listbox.com
Bryan Cantrill
2012-08-06 06:53:09 UTC
Permalink
Thanks Josh and Gordon -- good finds, both! Will pull both sets of fixes
in, along with my fix to fall back to the old behavior if /proc is not
available and send a new webrev...

- Bryan
Post by Bryan Cantrill
https://github.com/joyent/illumos-joyent/commit/12ef7bd6596eedcaafd544ec675d6b72dfd15998
Post by Gordon Ross
Post by Bryan Cantrill
Please let me know if you have any comments, with my thanks in
advance!
I tried out your changes this evening. Looks pretty good.
I did find and fix a bug with watching multiple files.
Attached is a fix relative to your work, and a new test.
I also have a couple of fixes for the test script (variable names in
checktest() and to allow for the use of more than one flag) and some
https://gist.github.com/2e575757015b8160484c
Cheers.
--
Joshua M. Clulow
UNIX Admin/Developer
http://blog.sysmgr.org
-------------------------------------------
illumos-developer
Archives: https://www.listbox.com/member/archive/182179/=now
https://www.listbox.com/member/archive/rss/182179/21175001-8b3b9e0a
https://www.listbox.com/member/?&
Powered by Listbox: http://www.listbox.com
-------------------------------------------
illumos-developer
Archives: https://www.listbox.com/member/archive/182179/=now
RSS Feed: https://www.listbox.com/member/archive/rss/182179/21175072-c42b328b
Modify Your Subscription: https://www.listbox.com/member/?member_id=21175072&id_secret=21175072-605db409
Powered by Listbox: http://www.listbox.com
Bryan Cantrill
2012-08-06 09:51:29 UTC
Permalink
Thanks Josh and Gordon -- good finds, both! Will pull both sets of fixes in,
along with my fix to fall back to the old behavior if /proc is not available
and send a new webrev...
I've pulled both changes in, albeit with some modifications: (1) the
proposed fix to the multiple file problem had a memory leak (2) I
modified the test for the multiple file problem such that it would
fail without the fix, and (3) I tightened the proposed language in the
tail man page. This new commit is here:

https://github.com/joyent/illumos-joyent/commit/8ce4f9777bbb9bfb1b2c05d3b720b6505bb476ad

The original commit upon which this commit is based is here:

https://github.com/joyent/illumos-joyent/commit/12ef7bd6596eedcaafd544ec675d6b72dfd15998

Tomorrow I'll see if I can bludgeon webrev into generating something
that unifies these two commits into one diff. Thanks again, Gordon
and Josh!

- Bryan


-------------------------------------------
illumos-developer
Archives: https://www.listbox.com/member/archive/182179/=now
RSS Feed: https://www.listbox.com/member/archive/rss/182179/21175072-c42b328b
Modify Your Subscription: https://www.listbox.com/member/?member_id=21175072&id_secret=21175072-605db409
Powered by Listbox: http://www.listbox.com
Gordon Ross
2012-08-06 13:57:52 UTC
Permalink
Post by Bryan Cantrill
Thanks Josh and Gordon -- good finds, both! Will pull both sets of fixes in,
along with my fix to fall back to the old behavior if /proc is not available
and send a new webrev...
I've pulled both changes in, albeit with some modifications: (1) the
proposed fix to the multiple file problem had a memory leak (2) I
Yeah, technically it was a leak. Actually intentional, as it was only
one strdup per file name argument (and I was being lazy).

I see you took out the strdup and are assigning the path name for
every fobj.fo_name = buf, which is on the stack, right? Won't that
mean they'll all point to garbage after the return?

Thanks,
--
Gordon Ross <***@nexenta.com>
Nexenta Systems, Inc. www.nexenta.com
Enterprise class storage for everyone


-------------------------------------------
illumos-developer
Archives: https://www.listbox.com/member/archive/182179/=now
RSS Feed: https://www.listbox.com/member/archive/rss/182179/21175072-c42b328b
Modify Your Subscription: https://www.listbox.com/member/?member_id=21175072&id_secret=21175072-605db409
Powered by Listbox: http://www.listbox.com
Gordon Ross
2012-08-06 14:00:44 UTC
Permalink
Post by Gordon Ross
Post by Bryan Cantrill
Thanks Josh and Gordon -- good finds, both! Will pull both sets of fixes in,
along with my fix to fall back to the old behavior if /proc is not available
and send a new webrev...
I've pulled both changes in, albeit with some modifications: (1) the
proposed fix to the multiple file problem had a memory leak (2) I
Yeah, technically it was a leak. Actually intentional, as it was only
one strdup per file name argument (and I was being lazy).
I see you took out the strdup and are assigning the path name for
every fobj.fo_name = buf, which is on the stack, right? Won't that
mean they'll all point to garbage after the return?
Come to think of it, maybe it would be better to just let the
(fixed size) buf be a member of struct file_info too?
Post by Gordon Ross
--
Nexenta Systems, Inc. www.nexenta.com
Enterprise class storage for everyone
--
Gordon Ross <***@nexenta.com>
Nexenta Systems, Inc. www.nexenta.com
Enterprise class storage for everyone


-------------------------------------------
illumos-developer
Archives: https://www.listbox.com/member/archive/182179/=now
RSS Feed: https://www.listbox.com/member/archive/rss/182179/21175072-c42b328b
Modify Your Subscription: https://www.listbox.com/member/?member_id=21175072&id_secret=21175072-605db409
Powered by Listbox: http://www.listbox.com
Bryan Cantrill
2012-08-06 15:42:27 UTC
Permalink
Post by Gordon Ross
Post by Bryan Cantrill
Thanks Josh and Gordon -- good finds, both! Will pull both sets of fixes in,
along with my fix to fall back to the old behavior if /proc is not available
and send a new webrev...
I've pulled both changes in, albeit with some modifications: (1) the
proposed fix to the multiple file problem had a memory leak (2) I
Yeah, technically it was a leak. Actually intentional, as it was only
one strdup per file name argument (and I was being lazy).
It was actually one strdup() per event per file (associate() is called upon
every event to re-associate the event with the port).
Post by Gordon Ross
I see you took out the strdup and are assigning the path name for
every fobj.fo_name = buf, which is on the stack, right? Won't that
mean they'll all point to garbage after the return?
It doesn't matter. The fo_name is copied in (and the lookup performed)
regardless; it's only the address of the file_obj_t that is persistent
across calls to port_associate().

- Bryan
Post by Gordon Ross
Post by Bryan Cantrill
Thanks Josh and Gordon -- good finds, both! Will pull both sets of
fixes in,
Post by Bryan Cantrill
along with my fix to fall back to the old behavior if /proc is not
available
Post by Bryan Cantrill
and send a new webrev...
I've pulled both changes in, albeit with some modifications: (1) the
proposed fix to the multiple file problem had a memory leak (2) I
Yeah, technically it was a leak. Actually intentional, as it was only
one strdup per file name argument (and I was being lazy).
I see you took out the strdup and are assigning the path name for
every fobj.fo_name = buf, which is on the stack, right? Won't that
mean they'll all point to garbage after the return?
Thanks,
--
Nexenta Systems, Inc. www.nexenta.com
Enterprise class storage for everyone
-------------------------------------------
illumos-developer
Archives: https://www.listbox.com/member/archive/182179/=now
https://www.listbox.com/member/archive/rss/182179/21175001-8b3b9e0a
https://www.listbox.com/member/?&
Powered by Listbox: http://www.listbox.com
-------------------------------------------
illumos-developer
Archives: https://www.listbox.com/member/archive/182179/=now
RSS Feed: https://www.listbox.com/member/archive/rss/182179/21175072-c42b328b
Modify Your Subscription: https://www.listbox.com/member/?member_id=21175072&id_secret=21175072-605db409
Powered by Listbox: http://www.listbox.com
Gordon Ross
2012-08-06 16:25:07 UTC
Permalink
Post by Gordon Ross
Post by Bryan Cantrill
Post by Bryan Cantrill
Thanks Josh and Gordon -- good finds, both! Will pull both sets of
fixes in,
Post by Gordon Ross
Post by Bryan Cantrill
Post by Bryan Cantrill
along with my fix to fall back to the old behavior if /proc is not
available
Post by Gordon Ross
Post by Bryan Cantrill
Post by Bryan Cantrill
and send a new webrev...
I've pulled both changes in, albeit with some modifications: (1) the
proposed fix to the multiple file problem had a memory leak (2) I
Yeah, technically it was a leak. Actually intentional, as it was only
one strdup per file name argument (and I was being lazy).
It was actually one strdup() per event per file (associate() is called
upon every event to re-associate the event with the port).
Oh. Didn't notice that.
Post by Gordon Ross
I see you took out the strdup and are assigning the path name for
every fobj.fo_name = buf, which is on the stack, right? Won't that
mean they'll all point to garbage after the return?
It doesn't matter. The fo_name is copied in (and the lookup performed)
regardless; it's only the address of the file_obj_t that is persistent
across calls to port_associate().
So the garbage has no functional impact, OK. It could still confuse and
mislead someone doing debugging in this code. So, what do you think of
this suggestion?

... maybe it would be better to just let the (fixed size) buf be a
member of struct file_info too?

Thanks,
Gordon
--
Gordon Ross <***@nexenta.com>
Nexenta Systems, Inc. www.nexenta.com
Enterprise class storage for everyone



-------------------------------------------
illumos-developer
Archives: https://www.listbox.com/member/archive/182179/=now
RSS Feed: https://www.listbox.com/member/archive/rss/182179/21175072-c42b328b
Modify Your Subscription: https://www.listbox.com/member/?member_id=21175072&id_secret=21175072-605db409
Powered by Listbox: http://www.listbox.com
Bryan Cantrill
2012-08-06 16:47:02 UTC
Permalink
Post by Gordon Ross
Post by Gordon Ross
Post by Bryan Cantrill
Post by Bryan Cantrill
Thanks Josh and Gordon -- good finds, both! Will pull both sets of
fixes in,
Post by Gordon Ross
Post by Bryan Cantrill
Post by Bryan Cantrill
along with my fix to fall back to the old behavior if /proc is not
available
Post by Gordon Ross
Post by Bryan Cantrill
Post by Bryan Cantrill
and send a new webrev...
I've pulled both changes in, albeit with some modifications: (1) the
proposed fix to the multiple file problem had a memory leak (2) I
Yeah, technically it was a leak. Actually intentional, as it was only
one strdup per file name argument (and I was being lazy).
It was actually one strdup() per event per file (associate() is called
upon every event to re-associate the event with the port).
Oh. Didn't notice that.
Post by Gordon Ross
I see you took out the strdup and are assigning the path name for
every fobj.fo_name = buf, which is on the stack, right? Won't that
mean they'll all point to garbage after the return?
It doesn't matter. The fo_name is copied in (and the lookup performed)
regardless; it's only the address of the file_obj_t that is persistent
across calls to port_associate().
So the garbage has no functional impact, OK. It could still confuse and
mislead someone doing debugging in this code. So, what do you think of
this suggestion?
... maybe it would be better to just let the (fixed size) buf be a
member of struct file_info too?
I really don't think that there's a reason to do that. Indeed, to me
that's the kind of code that's actually more confusing rather than less, as
it leaves the question "why did they do that?!" (And I think the comment
that I have there adequately explains why the file_obj_t is in the
file_info -- I don't think that more is required.)

- Bryan



-------------------------------------------
illumos-developer
Archives: https://www.listbox.com/member/archive/182179/=now
RSS Feed: https://www.listbox.com/member/archive/rss/182179/21175072-c42b328b
Modify Your Subscription: https://www.listbox.com/member/?member_id=21175072&id_secret=21175072-605db409
Powered by Listbox: http://www.listbox.com

Darren Reed
2012-08-06 11:10:20 UTC
Permalink
Post by Bryan Cantrill
...
https://github.com/joyent/illumos-joyent/commit/12ef7bd6596eedcaafd544ec675d6b72dfd15998
(I'm happy to get a webrev on cr.illumos.org if people would prefer
it.) Note that this work was marginally more complicated than I'd
hoped due to bugs in tmpfs and NFS (they didn't properly emit the
create event), but the diffs are still pretty modest.
In usr/src/uts/common/fs/nfs/nfs3_vnops.c, vnevent_create() is moved from
line 2309-2312 to 2305-2310 where it is now called regardless of error
however in usr/src/uts/common/fs/nfs/nfs_vnops.c, the addition of the code
at 2037-2044 ensures that vnevent_create() only happens when an error
does not occur. On the face of it, I'm struggling to accept that the code in
nfs3_vnops.c is right. Is there something going on here that is not obvious?

If so, it might be helpful to add a short comment about why the return
value of nfs3setattr is ignored. Or should the call to nfs3setattr() have
its return value ignroed by casting it to void?

And to answer your other question, full webrevs are beneficial if only
because "frames" makes it a lot easier to properly review changes like
this set.

Darren




-------------------------------------------
illumos-developer
Archives: https://www.listbox.com/member/archive/182179/=now
RSS Feed: https://www.listbox.com/member/archive/rss/182179/21175072-c42b328b
Modify Your Subscription: https://www.listbox.com/member/?member_id=21175072&id_secret=21175072-605db409
Powered by Listbox: http://www.listbox.com
Darren Reed
2012-08-01 12:41:20 UTC
Permalink
Post by Bryan Cantrill
...
The second option is to take the BSD approach. This would take us back to
using event ports (which we ripped out as part of illumos#535), and we
would want to be mindful of the issues raised in illumos#535. In
particular, we would want to use PORT_SOURCE_FILE, not PORT_SOURCE_FD.
What are the perceived benefits of using PORT_SOURCE_FILE
over PORT_SOURCE_FD here? Or is this port an architectural
perception of it being the file that gets truncated and
not the fd so therefore it should be an event associated
with a file? (The question is so that I can understand the
choice, not because I want to argue one way or another.)
Post by Bryan Cantrill
Even still, our existing functionality for PORT_SOURCE_FILE isn't _quite_
enough to get us there: PORT_SOURCE_FILE as it is today will tell us that
a file has been modified, but not that it's been truncated. However, it's
a very simple (and, in my opinion, reasonable) change to add a FILE_TRUNC
event (or similar) to the PORT_SOURCE_FILE events -- which would allow our
tail -f (and not to mention, any other PORT_SOURCE_FILE event port
consumer) to exactly match the BSD behavior. I think that this is my
preferred approach, but it's obviously a tad more involved.
Thoughts on any/all of this?
Unless there is a specific reason not to, I would like to
suggest at least consideration of also adding a FILE_TRUNC
event for PORT_SOURCE_FD. In this way, a program that has
been passed an fd to the file can become aware of the event
as well - it may have no idea of which file has actually
been opened and for which the fd is associated with and
would thus be unable to use PORT_SOURCE_FILE to receive
the event.

Darren



-------------------------------------------
illumos-developer
Archives: https://www.listbox.com/member/archive/182179/=now
RSS Feed: https://www.listbox.com/member/archive/rss/182179/21175072-c42b328b
Modify Your Subscription: https://www.listbox.com/member/?member_id=21175072&id_secret=21175072-605db409
Powered by Listbox: http://www.listbox.com
Loading...