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
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
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
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: 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
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
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
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
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
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
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