On Monday 12 June 2023 12:29:46 Tom Rini wrote:
On Sun, Jun 11, 2023 at 05:24:50PM +0200, Christian Gmeiner wrote:
Am So., 11. Juni 2023 um 17:10 Uhr schrieb Pali Rohár pali@kernel.org:
On Sunday 11 June 2023 16:57:07 Christian Gmeiner wrote:
Hello
Hello! I must admit that this patch is broken and does not add any NFSv1 support. Just look below....
So .. let see what happend here.
On Friday 10 March 2023 10:51:55 Christian Gmeiner wrote:
From: Thomas RIENOESSL thomas.rienoessl@bachmann.info
NFSv1 support added by Christian Gmeiner, Thomas Rienoessl, September 27, 2018. As of now, NFSv3 is the default choice. if the server does not support NFSv3, we fall back to versions 2 or 1.
Signed-off-by: Thomas RIENOESSL thomas.rienoessl@bachmann.info
net/nfs.c | 42 +++++++++++++++++++++++++++++++++--------- 1 file changed, 33 insertions(+), 9 deletions(-)
diff --git a/net/nfs.c b/net/nfs.c index 21cae52f35..7a8887ef23 100644 --- a/net/nfs.c +++ b/net/nfs.c @@ -26,6 +26,10 @@
- NFSv2 is still used by default. But if server does not support NFSv2, then
- NFSv3 is used, if available on NFS server. */
+/* NOTE 5: NFSv1 support added by Christian Gmeiner, Thomas Rienoessl,
- September 27, 2018. As of now, NFSv3 is the default choice. If the server
- does not support NFSv3, we fall back to versions 2 or 1. */
#include <common.h> #include <command.h> #include <display_options.h> @@ -78,6 +82,7 @@ static char nfs_path_buff[2048];
enum nfs_version { NFS_UNKOWN = 0,
NFS_V1 = 1, NFS_V2 = 2, NFS_V3 = 3,}; @@ -192,6 +197,7 @@ static void rpc_req(int rpc_prog, int rpc_proc, uint32_t *data, int datalen) switch (rpc_prog) { case PROG_NFS: switch (choosen_nfs_version) {
case NFS_V1: case NFS_V2: rpc_pkt.u.call.vers = htonl(2);So if NFSv1 is chosen then this code uses NFSv2. This is either rebasing problem or just prove that this patch does not add any NFSv1 support.
break;@@ -205,8 +211,26 @@ static void rpc_req(int rpc_prog, int rpc_proc, uint32_t *data, int datalen) break; } break;
case PROG_PORTMAP: case PROG_MOUNT:
switch (choosen_nfs_version) {case NFS_V1:rpc_pkt.u.call.vers = htonl(1);break;And later here for NFSv1 we are trying to use Mount Server, which NFSv1 did not use at all. So this patch really does not have to work with old NFSv1 servers.
Instead NFSv1 uses NFSPROC_ROOT RPC call exported by NFS server. (See that this RPC call is deprecated in NFSv2 and MNT server is used in NFSv2 instead.)
MNTv1 is service used by the NFSv2 and it returns NFSv2 file handles (not NFSv1 file handles). MNTv2 is also used by NFSv2 and as addition to MNTv1, it adds DIRPATH rpc call. So if NFSv2 does not need to use DIRPATH then it is fine to use just MNTv1 in NFSv2.
case NFS_V2:rpc_pkt.u.call.vers = htonl(2);break;case NFS_V3:rpc_pkt.u.call.vers = htonl(3);break;case NFS_UNKOWN:/* nothing to do */break;}break;case PROG_PORTMAP: default: rpc_pkt.u.call.vers = htonl(2); /* portmapper is version 2 */ }@@ -311,7 +335,7 @@ static void nfs_readlink_req(void) p = &(data[0]); p = rpc_add_credentials(p);
if (choosen_nfs_version == NFS_V2) {
if (choosen_nfs_version != NFS_V3) { memcpy(p, filefh, NFS_FHSIZE); p += (NFS_FHSIZE / 4); } else { /* NFS_V3 */@@ -340,7 +364,7 @@ static void nfs_lookup_req(char *fname) p = &(data[0]); p = rpc_add_credentials(p);
if (choosen_nfs_version == NFS_V2) {
if (choosen_nfs_version != NFS_V3) { memcpy(p, dirfh, NFS_FHSIZE); p += (NFS_FHSIZE / 4); *p++ = htonl(fnamelen);@@ -380,7 +404,7 @@ static void nfs_read_req(int offset, int readlen) p = &(data[0]); p = rpc_add_credentials(p);
if (choosen_nfs_version == NFS_V2) {
if (choosen_nfs_version != NFS_V3) { memcpy(p, filefh, NFS_FHSIZE); p += (NFS_FHSIZE / 4); *p++ = htonl(offset);@@ -410,13 +434,13 @@ static void nfs_send(void)
switch (nfs_state) { case STATE_PRCLOOKUP_PROG_MOUNT_REQ:
if (choosen_nfs_version == NFS_V2)
if (choosen_nfs_version != NFS_V3) rpc_lookup_req(PROG_MOUNT, 1); else /* NFS_V3 */ rpc_lookup_req(PROG_MOUNT, 3); break; case STATE_PRCLOOKUP_PROG_NFS_REQ:
if (choosen_nfs_version == NFS_V2)
if (choosen_nfs_version != NFS_V3) rpc_lookup_req(PROG_NFS, 2); else /* NFS_V3 */ rpc_lookup_req(PROG_NFS, 3);@@ -457,7 +481,7 @@ static int rpc_handle_error(struct rpc_t *rpc_pkt) const int min = ntohl(rpc_pkt->u.reply.data[0]); const int max = ntohl(rpc_pkt->u.reply.data[1]);
if (max < NFS_V2 || max > NFS_V3 || min > NFS_V3) {
if (max < NFS_V1 || max > NFS_V3 || min > NFS_V3) { puts("*** ERROR: NFS version not supported"); debug(": Requested: V%d, accepted: min V%d - max V%d\n", choosen_nfs_version,@@ -588,7 +612,7 @@ static int nfs_lookup_reply(uchar *pkt, unsigned len) if (ret) return ret;
if (choosen_nfs_version == NFS_V2) {
if (choosen_nfs_version != NFS_V3) { if (((uchar *)&(rpc_pkt.u.reply.data[0]) - (uchar *)(&rpc_pkt) + NFS_FHSIZE) > len) return -NFS_RPC_DROP; memcpy(filefh, rpc_pkt.u.reply.data + 1, NFS_FHSIZE);@@ -712,7 +736,7 @@ static int nfs_read_reply(uchar *pkt, unsigned len) if (!(nfs_offset % ((NFS_READ_SIZE / 2) * 10))) putc('#');
if (choosen_nfs_version == NFS_V2) {
if (choosen_nfs_version != NFS_V3) { rlen = ntohl(rpc_pkt.u.reply.data[18]); data_ptr = (uchar *)&(rpc_pkt.u.reply.data[19]); } else { /* NFS_V3 */-- 2.39.2
And looking at the other changes here, there is really _no_ code which adds NFSv1 support.
So what is this patch doing? The only thing which it does is that for NFSv1 requests it does NFSv2 calls. On every place is just check that choosen_nfs_version is not NFS_V3.
Which just basically duplicates NFSv2 to be used two times.
I would suggest to revisit this patch (who reviewed it at all?) and either fix it or revert it. And of course properly test it. (And I really curious where you find NFSv1 server because Linux has already removed also NFSv2 support from userspace...)
Soo. I had a look at RFC 1094 and this patch adds version one of the mount protocol. I am quite unhappy that we got into this state, but the company I worked for uses the term NFSv1 for this in all their configuration tools etc. What would you suggest to improve this situation?
Suggestion: Revert this one patch, then figure out what is needed to be supported (describe all details what kind of protocol and what packets needs to be send and received), find a way and discuss how to implement it and prepare patch for the review.
Here comes a big problem: "find a way and discuss how to implement it and prepare patch for the review." I am not sure if there is really someone interested in discussion about this topic and it is even harder to find someone that reviews U-Boot patches. And this is not specific to networking stuff.
I can prepare a patch that reworks the current implementation (without a revert) and send it out.
If you, who sent this patch, are unhappy about this patch, and also Peter and me pointed issues, then this patch really should not have land into git master branch in this form. It means that nobody is happy with this patch.
To clarify a fact: I am happy with the patch. I am unhappy with the wrong "naming" that my old employer used and influenced this patch. Also the company is shipping thousands of devices per year where these patches are used.
Further clean-ups and clarifications to the support here in terms of what it does and doesn't provide are good. But to the point on reviews, yes, I do wish we had more people interested in various areas, and with time to devote to reviewing code as well. Sadly, we don't always, and I took this particular set of patches as being small enough of a global impact while (hopefully!) making future contributions both in this area and the related platforms using it more likely.
-- Tom
I can help with reviews in this area. In this case, please CC me.