jsvc: Fix regression around invalid lockf(3) semantics #304
Conversation
michael-o
force-pushed
the
lockf-regression
branch
from
f362e89 to
58e1b5a
Compare
|
Is this a regression or did adding the logging expose a bug that had existed in the lock code for a quite a while (looks like the last lock related change was 8 years ago)? Based on the proposed fix, it looks like there was a bug in the lock code that was exposed by adding the logging. |
Both, but the rather the latter. The logging code triggered this bug which has always been there. So your understand of the issue is correct. |
|
The change didn't simply add logging, but also some new |
|
@markt-asf Any objections to merge this? |
| char buff[80]; | ||
|
|
||
| fd = open(args->pidf, O_RDONLY, 0); | ||
| fd = open(args->pidf, O_RDWR, 0); |
There was a problem hiding this comment.
Since this flag is used more than once, should it be extracted into a constant to ensure we show the intent of using the same value?
There was a problem hiding this comment.
You mean the zero? Or O_RDWR?
There was a problem hiding this comment.
I was talking about the flags parameter but I suppose the same could be said of the mode.
There was a problem hiding this comment.
I don't see a benefit adding a define for a define.
There was a problem hiding this comment.
Agreed, but I think the reason for allowing WRite should be documented in the code.
Possibly consider a private function to obtain the fd; this can then be fully documented.
lockf(3) mandates that the passed file descriptor must be in O_RDWR or O_WRONLY mode. This was caused by f361cf1. Downstream bug: https://bugs.freebsd.org/bugzilla/show_bug.cgi?id=291822
michael-o
force-pushed
the
lockf-regression
branch
from
58e1b5a to
6a1134e
Compare
|
Looking at this now. There look to be fixes for multiple issues wrapped up into a single commit. I'll likely break each one out into a separate commit so the commit message for each change can explain why the change is being made. I might have follow up questions for some of the changes if it isn't obvious to me why the change has been made. |
Totally fine |
|
Fixed in a series of commits heavily based on this PR. |
5 participants
lock(3) mandates that the passed file descriptor must be in O_RDWR or O_WRONLY mode. This was caused by f361cf1.
Downstream bug: https://bugs.freebsd.org/bugzilla/show_bug.cgi?id=291822