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
