fix tail -f on stdin

[oshogbo]

Can you provide some more context?

[swills]

In D30667#690843, @oshogbo wrote:

Can you provide some more context?

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

[oshogbo]

Hym its works for me.

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

[oshogbo]

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]

tail/tail.c

214

Might be better to do that in a separate review.

[swills]

In D30667#690850, @oshogbo wrote:

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.

[swills]

In D30667#690869, @swills wrote:

In D30667#690850, @oshogbo wrote:

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.

[markj]

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();
}

[swills]

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?

[markj]

In D30667#699823, @swills wrote:

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) {.

[swills]

In D30667#699843, @markj wrote:

In D30667#699823, @swills wrote:

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.

[markj]

In D30667#699869, @swills wrote:

In D30667#699843, @markj wrote:

In D30667#699823, @swills wrote:

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

[markj]

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

[oshogbo]

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

[markj]

In D30667#755382, @oshogbo wrote:

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

I ended up committing a different version: 7e11889959a6c92f05e1c1949deb73295ce60bac

[oshogbo]

Thank you @markj