On Aug 14, 2009, at 12:57 PM, Trond Myklebust wrote:
On Fri, 2009-08-14 at 12:32 -0400, Chuck Lever wrote:
On Aug 14, 2009, at 10:19 AM, Benny Halevy wrote:
Coalesce multilpe constant RESERVE_SPACEs into one
Signed-off-by: Benny Halevy <bhalevy@xxxxxxxxxxx>
---
fs/nfs/nfs4xdr.c | 22 +++++-----------------
1 files changed, 5 insertions(+), 17 deletions(-)
diff --git a/fs/nfs/nfs4xdr.c b/fs/nfs/nfs4xdr.c
index 17915c8..d460d81 100644
--- a/fs/nfs/nfs4xdr.c
+++ b/fs/nfs/nfs4xdr.c
@@ -1562,17 +1562,15 @@ static void encode_create_session(struct
xdr_stream *xdr,
uint32_t len;
struct nfs_client *clp = args->client;
- RESERVE_SPACE(4);
- *p++ = cpu_to_be32(OP_CREATE_SESSION);
+ len = scnprintf(machine_name, sizeof(machine_name), "%s",
+ clp->cl_ipaddr);
- RESERVE_SPACE(8);
+ RESERVE_SPACE(20 + 2*28 + 20 + len + 12);
It would be nicer if we could use the foo_maxsz macros or "n *
sizeof(__be32)" here somehow instead of integers.
Note the use of "somehow" implying that my suggestion is not supposed
to be a precise proscription for how to code it. I presume we all
understand well enough the meaning of these macros to get the idea of
what I was trying to say.
No. sizeof(__be32) is a constant == 4. Spelling it out in every
reserve_space would be bloat, not documentation.
foo_maxsz is something completely different: it spells out the maximum
possible buffer size. Please don't confuse that with the actual buffer
content size.
The effective difference between foo_maxsz and the other macros is
only in the variable size parts of each argument set. maxsz can be
constructed with a number of fixed size pieces that can be used
separately or combined into a single macro that _is_ appropriate to
use in the XDR routines. Since we are explicitly adding the actual
lengths of variable sized arguments before calling reserve_space(), I
don't see a problem with using length macros for each argument.
The risk of using naked integers in the XDR routines is that if
there's a length bug in the maxsz macros, we also need to visit the
XDR routines carefully to find other instances of the same problem.
If we use macros everywhere, then fixing one length macro will address
all similar instances at once...
Maybe this isn't a big deal for something like the kernel's rpcbind
client, but nfs4xdr.c is already complex enough that I would easily
accept a slight decrease in readability to reduce the nontrivial risk
of getting these values wrong. Take it from someone who has found and
fixed several such problems over the past few years: this is not a
documentation issue.
Benny has combined the reserve_space() calls into a single call in
each XDR routine, thus visually separating the marshalling of each
argument from its related XDR buffer management. If you already know
what you're looking at, that's not much of an issue. But for those
who have never seen this code before, I think this reduces legibility
in favor of a slight code optimization. Using length macros would
make it easier to understand the relationship between marshalling and
buffer management in each routine.
IOW: Please just leave the above as it is.
--
Chuck Lever
chuck[dot]lever[at]oracle[dot]com
--
To unsubscribe from this list: send the line "unsubscribe linux-nfs" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
