Re: RFC: [PATCH] trans/fakeroot.c
The commit I have just pushed, which does make sense, does fix gpsd at least. Samuel
Re: RFC: [PATCH] trans/fakeroot.c
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.0 +0200
> +++ b/trans/fakeroot.c2015-10-08 22:34:47.0 +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
Re: RFC: [PATCH] trans/fakeroot.c
On Thu, 2015-05-21 at 10:56 +0200, Svante Signell wrote:
> On Fri, 2015-05-15 at 20:31 +0200, Samuel Thibault wrote:
> > Svante Signell, le Tue 12 May 2015 09:15:46 +0200, a écrit :
>
> > > - if (file != MACH_PORT_NULL && (nn->openmodes & ~newmodes))
> > > + if (file != MACH_PORT_NULL && (nn->openmodes & newmodes ))
> > > /* works */
> >
> > This change needs to be motivated and explained.
...
> A few notes:
> - the condition (nn->openmodes & ~newmodes) is wrong since this
> triggers
> on newmodes being complementary to nn->openmodes but the comment in
> the
> code says:
> /* Intersecting sets.
>We need yet another new peropen on this node. */
> The correct condition for intersection is the and operation, i.e.
> (nn->openmodes & newmodes ), one example being
> nn->openmodes = 1 = O_READ and
> newmodes = 3 = O_READ | O_WRITE,; the intersection being O_READ.
>
> For the test case given before and the wrong condition triggers the
> following:
> ./my_fakeroot-hurd rpctrace make 2>&1 | tee rpctrace_nOK.out
> ...
> 9<--154(pid16637)->dir_lookup ("dev/null" 1 0)
> Intersecting sets
> newmodes=1, (nn->openmodes=2 & ~newmodes=376) = 2
> (nn->openmodes & newmodes) = 0
>
> file == MACH_PORT_NULL
> nn->file=62
> (nn->openmodes=2 | newmodes=1) = 3
> bad_retryname=NULL, file=60
> = 0 1 ""175<--183(pid16637)
>
> with no bad effects but
>
> 146<--178(pid16637)->dir_lookup ("gnatvsn_from/alloc.ali" 1 0)
> Intersecting sets
> newmodes=1, (nn->openmodes=2 & ~newmodes=376) = 2
> (nn->openmodes & newmodes) = 0
>
> file == MACH_PORT_NULL
> nn->file=84
> (nn->openmodes=2 | newmodes=1) = 3
> bad_retryname=NULL, file=0
> = 0x400d (Permission denied)
>
> has, since the file returned is zero.
>
> According to dir_lookup in fs.defs a file name of null is equivalent
> to
> a reopen of that file.
> err = dir_lookup (nn->file, "", nn->openmodes | newmodes,
> 0,
> &bad_retry, bad_retryname, &file);
> 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.
I have now built the failing packages with the patch inlined below:
- gpsd 3.15-1 (patched to build, bug #)
- frama-c 20150201+sodium+dfsg-2
- snd 11.7-4 (patched to build, bug #) Have to check the failing
package. With a fixed patch snd b11.7-4 uilds fine with the old
fakeroot and with the corrected one.
- gcc-snapshot 20150817-1, (20150913-1 has problems with some pthread
headers when building g++,same problem on the buildd)
--- a/trans/fakeroot.c.orig2015-10-08 22:32:09.0 +0200
+++ b/trans/fakeroot.c 2015-10-08 22:34:47.0 +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,
we need yet another new peropen on this node. */
mach_port_deallocate (mach_task_self (), file);
file = MACH_PORT_NULL;
I'll be submitting bug reports to make the packages build later
(outside fakeroot), I'm currently connected a very low quota wireless
connection (the main one is broken due to an unexpected cable being cut
off. Repairs will be made later this week.)
I told you, the test should be inclusive, not exclusive :)
Re: [PATCH] hurd: align -p and -pg behavior on Linux
Ping?
(I don't have commit access)
Samuel Thibault, le Sat 19 Sep 2015 14:00:23 +0200, a écrit :
> On Linux, -p and -pg do not make gcc link against libc_p.a, only
> -profile does (as documented in r11246), and thus people expect -p
> and -pg to work without libc_p.a installed (it is actually even not
> available any more in Debian). We should thus rather make the Hurd port
> do the same to avoid build failures.
>
> Samuel
>
> * gcc/config/gnu.h (LIB_SPEC) [-p|-pg]: Link with -lc instead of -lc_p.
> * gcc/config/i386/gnu.h (STARTFILE_SPEC) [-p|-pg]: Use gcrt1.o
> instead of gcrt0.o.
>
> --- gcc/config/gnu.h.orig 2015-09-16 00:43:09.785570853 +0200
> +++ gcc/config/gnu.h 2015-09-16 00:43:12.513550418 +0200
> @@ -25,7 +25,7 @@
>
> /* Default C library spec. */
> #undef LIB_SPEC
> -#define LIB_SPEC "%{pthread:-lpthread} %{pg|p|profile:-lc_p;:-lc}"
> +#define LIB_SPEC "%{pthread:-lpthread} %{profile:-lc_p;:-lc}"
>
> #undef GNU_USER_TARGET_OS_CPP_BUILTINS
> #define GNU_USER_TARGET_OS_CPP_BUILTINS()\
> --- gcc/config/i386/gnu.h.orig2015-09-17 21:41:13.0 +
> +++ gcc/config/i386/gnu.h 2015-09-17 23:03:57.0 +
> @@ -27,11 +27,11 @@
> #undef STARTFILE_SPEC
> #if defined HAVE_LD_PIE
> #define STARTFILE_SPEC \
> - "%{!shared:
> %{pg|p|profile:gcrt0.o%s;pie:Scrt1.o%s;static:crt0.o%s;:crt1.o%s}} \
> + "%{!shared:
> %{pg|p:gcrt1.o%s;profile:gcrt0.o%s;pie:Scrt1.o%s;static:crt0.o%s;:crt1.o%s}} \
> crti.o%s %{static:crtbeginT.o%s;shared|pie:crtbeginS.o%s;:crtbegin.o%s}"
> #else
> #define STARTFILE_SPEC \
> - "%{!shared: %{pg|p|profile:gcrt0.o%s;static:crt0.o%s;:crt1.o%s}} \
> + "%{!shared: %{pg|p:gcrt1.o%s;profile:gcrt0.o%s;static:crt0.o%s;:crt1.o%s}}
> \
> crti.o%s %{static:crtbeginT.o%s;shared|pie:crtbeginS.o%s;:crtbegin.o%s}"
> #endif
>
Re: Meta: Three-phase patch review
Quoting Olaf Buddenhagen (2015-10-06 05:56:51) > I just stumbled upon this, and it seems to me the Hurd project could > benefit from this approach: > >http://sarah.thesharps.us/2014/09/01/the-gentle-art-of-patch-review/ I like it, and I agree with it. I'm a bit curious however why you brought it up. I think we're doing fine with respect to patch reviews lately. Justus signature.asc Description: signature
