Svante Signell, le Sun 11 Oct 2015 23:02:16 +0200, a écrit :
> > We also see from the printout that
> > nn->openmodes = 2 = O_WRITE and
> > newmodes = 1 = O_READ i.e. no intersecting sets.
> >
>
> The above condition is really happening when building (patched to
> build, not related to fakeroot) gpsd.
Ok. It makes complete sense: we've been writing to a file, and now we
are reading from it.
> --- a/trans/fakeroot.c.orig 2015-10-08 22:32:09.000000000 +0200
> +++ b/trans/fakeroot.c 2015-10-08 22:34:47.000000000 +0200
> @@ -216,9 +216,9 @@
> {
> /* The user wants openmodes we haven't tried before. */
>
> - if (file != MACH_PORT_NULL && (nn->openmodes & ~newmodes))
> + if (file != MACH_PORT_NULL && (nn->openmodes & newmodes))
> {
> - /* Intersecting sets with no inclusion. `file' doesn't fit
> either,
> + /* Intersecting sets. `file' doesn't fit either,
But this doesn't make sense! Let me explain in detail what
check_openmodes does:
- we have an existing node, nn, which contains an opened file nn->file,
opened with modes nn->openmodes.
- we want to have it opened with nn->openmodes | newmodes.
- the 'file' port parameter happens to be that file opened with
newmodes.
so:
- either nn->openmodes already contains the newmodes bits, nn->file is
thus already the file opened with modes nn->openmodes | newmodes
- or it doesn't, and so we have to do something:
- if newmodes actually contains all the nn->openmodes bit, then the
'file' port is just what we need, and there is no need to reopen the
file.
- otherwise the 'file' port is useless, so we should just close it,
and really reopen the file with nn->openmodes | newmodes.
So the condition for closing the file and making it null is
"nn->openmodes has bits that newmodes doesn't have". That happens to
translate to "nn->openmodes & ~newmodes". If I'm wrong somewhere in my
reasoning (which can probably be very true), please tell me where.
Just saying "that testcase works with my patch" is not enough to
convince me to apply it, because there are so many other cases. In the
testcase you mention above:
> > nn->openmodes = 2 = O_WRITE and
> > newmodes = 1 = O_READ i.e. no intersecting sets.
(nn->openmodes & ~newmodes) will be O_WRITE, which shows that we have an
existing file opened with O_WRITE, but the provided 'file' port doesn't
have it (as announced by newmodes). And thus we *have* to destroy the
'file' port and not use it, and instead reopen the file to really get
O_WRITE|O_READ: otherwise we wouldn't have O_WRITE any more, which will
pose problem whenever we want to write to it again.
In the other testcase you mentioned in your previous mail:
> nn->openmodes = 1 = O_READ and
> newmodes = 3 = O_READ | O_WRITE,
We have an existing file opened in O_READ, and now we want
O_READ|O_WRITE. The existing file is not enough, it's missing O_WRITE
(newmodes &~ nn->openmodes returns O_WRITE), but the provided 'file'
port has both O_READ|O_WRITE, so we can just use it, there is no need to
reopen the file, it'll cover both the original need (O_READ) as tested
by nn->openmodes & ~newmodes being 0, and the new need (O_READ|O_WRITE).
> I told you, the test should be inclusive, not exclusive :)
You didn't prove why. Testcases are good when they cover all cases.
Otherwise they are just an indication of something is going wrong, not
that the proposed fix is always right.
Either what I said above is wrong at some point, or the bug is somewhere
else (see the previous changes we had to make in netfs_attempt_chmod for
instance). What call *actually* produces the permission denied error,
BTW? In our previous exchange it was actually the reopen operation.
Samuel
