Re: RFC: [PATCH] Re: Test case for fakeroot-hurd failure with a socket
Hello, I have eventually applied this version: Svante Signell, le Wed 13 May 2015 09:35:28 +0200, a écrit : > +error_t > +netfs_set_translator (struct iouser *cred, struct node *np, > + char *argz, size_t argzlen) > +{ > + return file_set_translator (netfs_node_netnode (np)->file, > + FS_TRANS_EXCL|FS_TRANS_SET, > + FS_TRANS_EXCL|FS_TRANS_SET, 0, > + argz, argzlen, > + MACH_PORT_NULL, MACH_MSG_TYPE_COPY_SEND); > +} Thanks a lot for the investigation and proposed fix! Samuel
Re: RFC: [PATCH] Re: Test case for fakeroot-hurd failure with a socket
Svante Signell, le Wed 13 May 2015 10:31:42 +0200, a écrit : > On Wed, 2015-05-13 at 10:04 +0200, Samuel Thibault wrote: > > Samuel Thibault, le Wed 13 May 2015 09:57:27 +0200, a écrit : > > > > + return file_set_translator (netfs_node_netnode (np)->file, > > > > + FS_TRANS_EXCL|FS_TRANS_SET, > > > > + FS_TRANS_EXCL|FS_TRANS_SET, 0, > > > > + argz, argzlen, > > > > + MACH_PORT_NULL, MACH_MSG_TYPE_COPY_SEND); > > > > > > Mmm, I don't think you want to set the active_flags parameter, see the > > > comment above: > > > > And you have set the active parameter to MACH_PORT_NULL, so active_flags > > should really be set to 0, it doesn't really make sense otherwise. > > This is copeied verbatim from fakeroot.c:netfs_attempt_mksymlink() and > netfs_attempt_mkdev() so the active_flags parameter should be zero there > too? Mmm, thinking about it more, making it MACH_PORT_NULL can actually make sense: it'll make any existing active translator go away, which is perhaps a good thing actually. Samuel
Re: RFC: [PATCH] Re: Test case for fakeroot-hurd failure with a socket
On Wed, 2015-05-13 at 10:04 +0200, Samuel Thibault wrote: > Samuel Thibault, le Wed 13 May 2015 09:57:27 +0200, a écrit : > > > + return file_set_translator (netfs_node_netnode (np)->file, > > > + FS_TRANS_EXCL|FS_TRANS_SET, > > > + FS_TRANS_EXCL|FS_TRANS_SET, 0, > > > + argz, argzlen, > > > + MACH_PORT_NULL, MACH_MSG_TYPE_COPY_SEND); > > > > Mmm, I don't think you want to set the active_flags parameter, see the > > comment above: > > And you have set the active parameter to MACH_PORT_NULL, so active_flags > should really be set to 0, it doesn't really make sense otherwise. This is copeied verbatim from fakeroot.c:netfs_attempt_mksymlink() and netfs_attempt_mkdev() so the active_flags parameter should be zero there too? New patch attached! Index: hurd-0.6/trans/fakeroot.c === --- hurd-0.6.orig/trans/fakeroot.c +++ hurd-0.6/trans/fakeroot.c @@ -450,6 +450,20 @@ netfs_S_dir_lookup (struct protid *dirus return err; } +/* The user may define this function. Attempt to set the passive + translator record for FILE to ARGZ (of length ARGZLEN) for user + CRED. */ +error_t +netfs_set_translator (struct iouser *cred, struct node *np, + char *argz, size_t argzlen) +{ + return file_set_translator (netfs_node_netnode (np)->file, + FS_TRANS_EXCL|FS_TRANS_SET, + 0, 0, + argz, argzlen, + MACH_PORT_NULL, MACH_MSG_TYPE_COPY_SEND); +} + /* These callbacks are used only by the standard netfs_S_dir_lookup, which we do not use. But the shared library requires us to define them. */ error_t
Re: RFC: [PATCH] Re: Test case for fakeroot-hurd failure with a socket
Samuel Thibault, le Wed 13 May 2015 09:57:27 +0200, a écrit : > > + return file_set_translator (netfs_node_netnode (np)->file, > > + FS_TRANS_EXCL|FS_TRANS_SET, > > + FS_TRANS_EXCL|FS_TRANS_SET, 0, > > + argz, argzlen, > > + MACH_PORT_NULL, MACH_MSG_TYPE_COPY_SEND); > > Mmm, I don't think you want to set the active_flags parameter, see the > comment above: And you have set the active parameter to MACH_PORT_NULL, so active_flags should really be set to 0, it doesn't really make sense otherwise. Samuel
Re: RFC: [PATCH] Re: Test case for fakeroot-hurd failure with a socket
Svante Signell, le Wed 13 May 2015 09:35:28 +0200, a écrit : > Q: Why does it work without fakeroot-hurd? You mean on a bare filesystem? They use libdiskfs, which implements diskfs_S_file_set_translator, which calls diskfs_set_translator, which is implemented by filesystems. And the filesystems which don't use libdiskfs but libnetfs quite often don't implement netfs_set_translator indeed, and then sockets can't be made on them, but that's usually not a problem since that's /proc, /dev/vcs, etc. > +/* The user may define this function. Attempt to set the passive > + translator record for FILE to ARGZ (of length ARGZLEN) for user > + CRED. */ > +error_t > +netfs_set_translator (struct iouser *cred, struct node *np, > + char *argz, size_t argzlen) > +{ > + return file_set_translator (netfs_node_netnode (np)->file, > + FS_TRANS_EXCL|FS_TRANS_SET, > + FS_TRANS_EXCL|FS_TRANS_SET, 0, > + argz, argzlen, > + MACH_PORT_NULL, MACH_MSG_TYPE_COPY_SEND); > +} Mmm, I don't think you want to set the active_flags parameter, see the comment above: netfs_S_file_set_translator will have already set the active translator thanks to its fshelp_set_active call, so only the passive translator should be set here. Samuel
Re: RFC: [PATCH] Re: Test case for fakeroot-hurd failure with a socket
On Wed, 2015-05-13 at 00:52 +0200, Samuel Thibault wrote: > Hello, > > Svante Signell, le Tue 12 May 2015 22:09:33 +0200, a écrit : > > netfs_attempt_chmod() returns an error of EOPNOTSUPP and hits the > Err, no, see what I wrote earlier: “implement the > netfs_set_translator stub by just calling file_set_translator on the > underlying node to set the passive translator.” Do not overwrite > netfs_S_file_set_translator, it'd disable everything that is done in the > netfs_S_file_set_translator provided by libnetfs... > > > + char trans[sizeof _HURD_IFSOCK + passivelen]; > > + memcpy (trans, _HURD_IFSOCK, sizeof _HURD_IFSOCK); > > + memcpy (&trans[sizeof _HURD_IFSOCK], passive, passivelen); > > Err, no, don't build the translator path by hand, it's already given to > you in the "passive" parameter! Simply pass passive and passivelen to > file_set_translator. New patch attached. Explanation as follows: Implement the netfs_set_translator stub by calling file_set_translator on the underlying node to set the passive translator. libnetfs/file_set_translator.c:netfs_S_file_set_translator does not handle the case when the passive translator is a socket. Q: Why does it work without fakeroot-hurd? Index: hurd-0.6/trans/fakeroot.c === --- hurd-0.6.orig/trans/fakeroot.c +++ hurd-0.6/trans/fakeroot.c @@ -450,6 +450,20 @@ netfs_S_dir_lookup (struct protid *dirus return err; } +/* The user may define this function. Attempt to set the passive + translator record for FILE to ARGZ (of length ARGZLEN) for user + CRED. */ +error_t +netfs_set_translator (struct iouser *cred, struct node *np, + char *argz, size_t argzlen) +{ + return file_set_translator (netfs_node_netnode (np)->file, + FS_TRANS_EXCL|FS_TRANS_SET, + FS_TRANS_EXCL|FS_TRANS_SET, 0, + argz, argzlen, + MACH_PORT_NULL, MACH_MSG_TYPE_COPY_SEND); +} + /* These callbacks are used only by the standard netfs_S_dir_lookup, which we do not use. But the shared library requires us to define them. */ error_t
Re: RFC: [PATCH] Re: Test case for fakeroot-hurd failure with a socket
Hello, Svante Signell, le Tue 12 May 2015 22:09:33 +0200, a écrit : > netfs_attempt_chmod() returns an error of EOPNOTSUPP and hits the > fallback case calling the stub in > libnetfs/set-get-trans.c:netfs_set_translator() returning EOPNOTSUPP. > > Implementing netfs_S_file_set_translator() calling file_set_translator() > in trans/fakeroot.c overrides this call and solves the problem. Err, no, see what I wrote earlier: “implement the netfs_set_translator stub by just calling file_set_translator on the underlying node to set the passive translator.” Do not overwrite netfs_S_file_set_translator, it'd disable everything that is done in the netfs_S_file_set_translator provided by libnetfs... > - Are the checks in the beginning really needed doesn't > file_set_translator() take care of that? It seems you are seeing things backwards. It's libnetfs' netfs_S_file_set_translator which is called first, does checks, and calls file_set_translator, which you'd implement in fakeroot. As another example, see libnetfs' netfs_S_dir_rmdir which does checks, and calls netfs_attempt_rmdir, which in fakeroot.c just calls dir_rmdir on the underlying node. > - Is the explanation really OK? Yes, it's the sort of things a reviewer needs, to be able to answer you promptly without spending hours trying to figure out what you tried to do. > +netfs_S_file_set_translator (struct protid *user, > + int passive_flags, int active_flags, > + int killtrans_flags, char *passive, > + mach_msg_type_number_t passivelen, > + mach_port_t active) > +{ ... > + > + if (passivelen && passive[passivelen - 1]) > +return EINVAL; > + > + char trans[sizeof _HURD_IFSOCK + passivelen]; > + memcpy (trans, _HURD_IFSOCK, sizeof _HURD_IFSOCK); > + memcpy (&trans[sizeof _HURD_IFSOCK], passive, passivelen); Err, no, don't build the translator path by hand, it's already given to you in the "passive" parameter! Simply pass passive and passivelen to file_set_translator. Samuel
RFC: [PATCH] Re: Test case for fakeroot-hurd failure with a socket
On Thu, 2015-05-07 at 16:51 +0200, Samuel Thibault wrote: > Svante Signell, le Thu 07 May 2015 10:14:23 +0200, a écrit : > > I think I understand most of your writing above :) > > > > Should the netfs_set_translator stub be implemented in trans/fakeroot.c > > or in libnetfs/set-get-trans.c? > > The former. > > > Still confusing is which functions are called: The ones in libnetfs vs > > the ones in fakeroot? > > The ones in libnetfs, and they call stubs from fakeroot. Attached is a patch that solves the test case for sockets. An attempt to explain: libnetfs/file-set-translator.c:netfs_S_file_set_translator() hits the default case in the newmode switch statement which in netfs_attempt_chmod() returns an error of EOPNOTSUPP and hits the fallback case calling the stub in libnetfs/set-get-trans.c:netfs_set_translator() returning EOPNOTSUPP. Implementing netfs_S_file_set_translator() calling file_set_translator() in trans/fakeroot.c overrides this call and solves the problem. Some questions: - Are the checks in the beginning really needed doesn't file_set_translator() take care of that? - Is the explanation really OK? - AOB? Index: hurd-0.6/trans/fakeroot.c === --- hurd-0.6.orig/trans/fakeroot.c +++ hurd-0.6/trans/fakeroot.c @@ -450,6 +450,39 @@ netfs_S_dir_lookup (struct protid *dirus return err; } +/* The user must define this function. Attempt to turn locked node NP + (user USER) into a socket with target NAME. */ +error_t +netfs_S_file_set_translator (struct protid *user, + int passive_flags, int active_flags, + int killtrans_flags, char *passive, + mach_msg_type_number_t passivelen, + mach_port_t active) +{ + struct node *np; + + if (!user) +return EOPNOTSUPP; + + if (!(passive_flags & FS_TRANS_SET) && !(active_flags & FS_TRANS_SET)) +return 0; + + if (passivelen && passive[passivelen - 1]) +return EINVAL; + + np = user->po->np; + + char trans[sizeof _HURD_IFSOCK + passivelen]; + memcpy (trans, _HURD_IFSOCK, sizeof _HURD_IFSOCK); + memcpy (&trans[sizeof _HURD_IFSOCK], passive, passivelen); + return file_set_translator (netfs_node_netnode (np)->file, + FS_TRANS_EXCL|FS_TRANS_SET, + FS_TRANS_EXCL|FS_TRANS_SET, 0, + trans, sizeof trans, + MACH_PORT_NULL, MACH_MSG_TYPE_COPY_SEND); +} + + /* These callbacks are used only by the standard netfs_S_dir_lookup, which we do not use. But the shared library requires us to define them. */ error_t
Re: Test case for fakeroot-hurd failure with a socket
Svante Signell, le Thu 07 May 2015 10:14:23 +0200, a écrit : > I think I understand most of your writing above :) > > Should the netfs_set_translator stub be implemented in trans/fakeroot.c > or in libnetfs/set-get-trans.c? The former. > Still confusing is which functions are called: The ones in libnetfs vs > the ones in fakeroot? The ones in libnetfs, and they call stubs from fakeroot. Samuel
Re: Test case for fakeroot-hurd failure with a socket
On Wed, 2015-05-06 at 23:30 +0200, Samuel Thibault wrote: > Hello, > > Svante Signell, le Mon 04 May 2015 14:14:16 +0200, a écrit : > > Attached is the test code where fakeroot-hurd fails. > > fakeroot-hurd ./test_sockets > > bind: Operation not supported > > Mmm, I don't think we want to implement it in netfs_attempt_chmod > as you are trying to do. See the code netfs_S_file_set_translator: > netfs_attempt_chmod() is called only because it's the "Short circuited > translators" case, i.e. when the FS can perhaps store the translator > directly in the mode. You only stored the mode in np->nn_stat.st_mode, > without calling some file_* function to actually make the change on the > underlying FS. That's why you end up with: > > > 67<--66(pid9244)->file_set_translator (6 6 0 "/hurd/ifsock" (null)) = > > 0x402d (Operation not supported) > > Since the underlying node is still a normal file. You also can't just > call file_chmod since that won't change the file type. > > Thus, do not modify netfs_attempt_chmod, rather implement the > netfs_set_translator stub by just calling file_set_translator on the > underlying node to set the passive translator. I think I understand most of your writing above :) Should the netfs_set_translator stub be implemented in trans/fakeroot.c or in libnetfs/set-get-trans.c? Still confusing is which functions are called: The ones in libnetfs vs the ones in fakeroot?
Re: Test case for fakeroot-hurd failure with a socket
Hello, Svante Signell, le Mon 04 May 2015 14:14:16 +0200, a écrit : > Attached is the test code where fakeroot-hurd fails. > fakeroot-hurd ./test_sockets > bind: Operation not supported Mmm, I don't think we want to implement it in netfs_attempt_chmod as you are trying to do. See the code netfs_S_file_set_translator: netfs_attempt_chmod() is called only because it's the "Short circuited translators" case, i.e. when the FS can perhaps store the translator directly in the mode. You only stored the mode in np->nn_stat.st_mode, without calling some file_* function to actually make the change on the underlying FS. That's why you end up with: > 67<--66(pid9244)->file_set_translator (6 6 0 "/hurd/ifsock" (null)) = > 0x402d (Operation not supported) Since the underlying node is still a normal file. You also can't just call file_chmod since that won't change the file type. Thus, do not modify netfs_attempt_chmod, rather implement the netfs_set_translator stub by just calling file_set_translator on the underlying node to set the passive translator. Thanks, Samuel
Test case for fakeroot-hurd failure with a socket
Hi, Attached is the test code where fakeroot-hurd fails. fakeroot-hurd ./test_sockets bind: Operation not supported Using fakeroot-tcp or no fakeroot succeeds: ./test_sockets Receiving via datagram socket plain (similar with fakeroot-tcp) = rpctrace ./test_sockets ... 67<--141(pid9219)->dir_unlink ("test_sockets_socket") = 0x4002 (No such file or directory) task131(pid9219)->mach_port_deallocate (pn{ 5}) = 0 task131(pid9219)->vm_allocate (17189648 4096 1) = 0 16957440 109<--142(pid9219)->dir_lookup ("servers/socket/1" 0 0) = 0 1 "" 158<--157(pid9219) 158<--157(pid9219)->socket_create (2 0) = 0160<--159(pid9219) task131(pid9219)->mach_port_mod_refs (pn{ 5} 0 1) = 0 67<--141(pid9219)->dir_mkfile (16 438) = 0162<--161(pid9219) 162<--161(pid9219)->file_set_translator (6 6 0 "/hurd/ifsock" (null)) = 0 162<--161(pid9219)->dir_lookup ("" 0 0) = 0 1 ""164<--163(pid9219) 164<--163(pid9219)->ifsock_getsockaddr () = 0166<--165(pid9219) 162<--161(pid9219)->file_chmod (420) = 0 67<--141(pid9219)->dir_link ( 162<--161(pid9219) "test_sockets_socket" 1) = 0 task131(pid9219)->mach_port_deallocate (pn{ 21}) = 0 task131(pid9219)->mach_port_deallocate (pn{ 20}) = 0 task131(pid9219)->mach_port_deallocate (pn{ 5}) = 0 160<--159(pid9219)->socket_bind ( 166<--165(pid9219)) = 0 task131(pid9219)->mach_port_deallocate (pn{ 22}) = 0 67<--141(pid9219)->dir_lookup ("test_sockets_socket" 0 0) = 0 1 "" 166<--167(pid9219) 166<--167(pid9219)->file_chmod (292) = 0 task131(pid9219)->mach_port_deallocate (pn{ 22}) = 0 156<--155(pid9219)->io_write ("Receiving via datagram socket\n" -1)Receiving via datagram socket = 0 30 140<--144(pid9219)->proc_mark_exit_request (0 0) = 0 task131(pid9219)->task_terminate () = 0 Child 9219 exited with 0 fakeroot-hurd = fakeroot-hurd rpctrace ./test_sockets 10<--46(pid9244)->dir_unlink ("test_sockets_socket") = 0x4002 (No such file or directory) task34(pid9244)->mach_port_deallocate (pn{ 7}) = 0 task34(pid9244)->vm_allocate (17189648 4096 1) = 0 16957440 9<--47(pid9244)->dir_lookup ("servers/socket/1" 0 0) = 0 1 "" 63<--62(pid9244) 63<--62(pid9244)->socket_create (2 0) = 065<--64(pid9244) task34(pid9244)->mach_port_mod_refs (pn{ 7} 0 1) = 0 10<--46(pid9244)->dir_mkfile (16 438) = 067<--66(pid9244) 67<--66(pid9244)->file_set_translator (6 6 0 "/hurd/ifsock" (null)) = 0x402d (Operation not supported) task34(pid9244)->mach_port_deallocate (pn{ 22}) = 0 task34(pid9244)->mach_port_deallocate (pn{ 7}) = 0 task34(pid9244)->mach_port_deallocate (pn{ 21}) = 0 task34(pid9244)->mach_port_mod_refs (pn{ 6} 0 1) = 0 task34(pid9244)->mach_port_mod_refs (pn{ 19} 0 1) = 0 11<--45(pid9244)->io_get_openmodes () = 0 259 11<--45(pid9244)->io_stat () = 0 {14 999 0 0 0 1368812073 0 8397200 1 1000 5 0 0 1430739968 0 1430739968 0 1430739968 0 512 8 0 0 0 0 0 0 0 0 0 0 0} 11<--45(pid9244)->io_seek (0 1) = 0x401d (Illegal seek) 61<--60(pid9244)->io_write ("bind: Operation not supported\n" -1)bind: Operation not supported = 0 30 task34(pid9244)->mach_port_deallocate (pn{ 19}) = 0 task34(pid9244)->mach_port_deallocate (pn{ 6}) = 0 43<--49(pid9244)->proc_mark_exit_request (256 0) = 0 task34(pid9244)->task_terminate () = 0 Child 9244 exited with 1 ./my_fakeroot-hurd rpctrace ./test_sockets With original code in trans/fakeroot.c: netfs_attempt_chmod (struct iouser *cred, struct node *np, mode_t mode) { if ((mode & S_IFMT) == 0) mode |= np->nn_stat.st_mode & S_IFMT; if ((mode & S_IFMT) != (np->nn_stat.st_mode & S_IFMT)) return EOPNOTSUPP; <<<- Errors out here ->>> /* We don't bother with error checking since the fake mode change should always succeed--worst case a later open will get EACCES. */ (void) file_chmod (netfs_node_netnode (np)->file, mode); set_faked_attribute (np, FAKE_MODE); np->nn_stat.st_mode = mode; return 0; } With modifications to handle the socket case: (and printouts in netfs_S_dir_lookup()) ... if ((mode & S_IFMT) == 0) { mode |= np->nn_stat.st_mode & S_IFMT; /* We don't bother with error checking since the fake mode change should always succeed--worst case a later open will get EACCES. */ (void) file_chmod (netfs_node_netnode (np)->file, mode); set_faked_attribute (np, FAKE_MODE); np->nn_stat.st_mode = mode; } if ((mode & S_IFMT) != 0) { fprintf(stderr,"trans/fakeroot.c: netfs_attempt_chmod: mode=%0o \n", mode)\ ; fprintf(stderr,"trans/fakeroot.c: netfs_attempt_chmod: (mode&S_IFMT)=%0o,\ S_IFSOCK=%0o\n", (mode & S_IFMT), S_IFSOCK); fprintf(stderr,"trans/fakeroot.c: netfs_attempt_chmod: np->nn_stat.st_mod\ e=%0o, (np->nn_stat.st_mode & S_IFMT)=%0o\n", np->nn_stat.st_mode, (np->nn_stat\ .st_mode & S_IFMT)); fflush(stderr); if ((mode & S_IFMT) != (np->nn_stat.st_mode & S_IFMT)) { switch (mode & S_IFMT) { ... case