Page MenuHomeFreeBSD

fix tail -f on stdin
ClosedPublic

Authored by swills on Jun 7 2021, 4:39 AM.
Tags
None
Referenced Files
Unknown Object (File)
Mon, Mar 4, 11:21 AM
Unknown Object (File)
Jan 8 2024, 6:10 PM
Unknown Object (File)
Dec 20 2023, 3:55 AM
Unknown Object (File)
Dec 12 2023, 2:32 AM
Unknown Object (File)
Dec 9 2023, 4:37 PM
Unknown Object (File)
Nov 28 2023, 10:14 PM
Unknown Object (File)
Nov 25 2023, 5:43 AM
Unknown Object (File)
Nov 23 2023, 4:07 PM
Subscribers
None

Details

Diff Detail

Lint
Lint Skipped
Unit
Tests Skipped
Build Status
Buildable 39806
Build 36695: arc lint + arc unit

Event Timeline

swills requested review of this revision.Jun 7 2021, 4:39 AM
swills created this revision.

Can you provide some more context?

Can you provide some more context?

Sure, so tail -f < /tmp/file used to work, but doesn't currently. This fixes it.

Hym its works for me.

# tail -f < pdfork.c 
        printf("%d\n", pid);
        fd2 = dup(fd);
...
tail/tail.c
214

Maybe we can take this code and make a function from it, and below init it with single file, and here with whole argv and argc?

274

Missing error checking.

275

Missing error checking.

swills added inline comments.
tail/tail.c
214

Might be better to do that in a separate review.

Hym its works for me.

# tail -f < pdfork.c 
        printf("%d\n", pid);
        fd2 = dup(fd);
...

What version of FreeBSD are you using there? For me, it doesn't work on 14-CURRENT without these changes.

Hym its works for me.

# tail -f < pdfork.c 
        printf("%d\n", pid);
        fd2 = dup(fd);
...

What version of FreeBSD are you using there? For me, it doesn't work on 14-CURRENT without these changes.

I should clarify, tail works, but tail -f doesn't. That is, tail -f returns the last few lines of the file and exits. With this change, tail -f continues running and also shows any new lines added to the end of the file.

Sorry for taking a while to get to this. I wrote a regression test for this and a few related things: https://reviews.freebsd.org/D31055

usr.bin/tail/tail.c
274 ↗(On Diff #91617)

file doesn't need to be dynamically allocated, it can live on the stack.

In fact, by marking the file_name field const, we can eliminate all of the strdup calls too.

281 ↗(On Diff #91617)

There's no need to call forward() if we're also calling follow(). I think it should basically look like this:

if (rflag) {
 reverse();
} else if (fflag) {
  file.file_name = strdup(fn);
  if (file.file_name == NULL)
    err();
  file.fp = stdin;
  follow()
} else {
  forward();
}

Thanks for the review and writing the test! I've made the change to only call foward() if we're not calling follow().

Changing file_name to const makes sense, but seems like a larger change, could we perhaps do that in a separate commit, just for clarity?

Thanks for the review and writing the test! I've made the change to only call foward() if we're not calling follow().

Changing file_name to const makes sense, but seems like a larger change, could we perhaps do that in a separate commit, just for clarity?

Ok. I still think we should not be malloc()ing file in this change though. Other than that and the inline comment, the change looks good to me.

usr.bin/tail/tail.c
272 ↗(On Diff #91971)

You can reduce the indentation here, else if (fflag) {.

This revision is now accepted and ready to land.Jul 8 2021, 12:50 PM
This revision now requires review to proceed.Jul 8 2021, 1:08 PM

Changing file_name to const makes sense, but seems like a larger change, could we perhaps do that in a separate commit, just for clarity?

Ok. I still think we should not be malloc()ing file in this change though. Other than that and the inline comment, the change looks good to me.

I tried several different ways but can't quite seem to find a way to make this work without the malloc(). It seems that changing file_name to const requires changes elsewhere, and even making a const file_info_t sfile and passing that to follow() runs into issues. I must be missing something.

Changing file_name to const makes sense, but seems like a larger change, could we perhaps do that in a separate commit, just for clarity?

Ok. I still think we should not be malloc()ing file in this change though. Other than that and the inline comment, the change looks good to me.

I tried several different ways but can't quite seem to find a way to make this work without the malloc(). It seems that changing file_name to const requires changes elsewhere, and even making a const file_info_t sfile and passing that to follow() runs into issues. I must be missing something.

I was suggesting a change along these lines: D31113

@swills Do you plan to commit your patch or make any further changes?

Pong @swills . Do you plan yo commit this change?

This revision is now accepted and ready to land.Dec 13 2021, 3:02 PM

Pong @swills . Do you plan yo commit this change?

I ended up committing a different version: 7e11889959a6c92f05e1c1949deb73295ce60bac