[PATCH v1 0/4] xdrgen fixes

[PATCH v1 0/4] xdrgen fixes

[Chuck Lever]

Four fixes I found while converting NFSD's NFS_ACLv3 XDR encoders
and decoders to use xdrgen.

Chuck Lever (4):
  xdrgen: Handle a union whose arms are all void
  xdrgen: Key the width tables by type name only
  xdrgen: Register optional-data names only for typedefs
  xdrgen: Take a union's numeric discriminant width from the declaration

 include/linux/sunrpc/xdrgen/nfs3.h     |  8 ++---
 include/linux/sunrpc/xdrgen/nfs_acl2.h |  2 +-
 tools/net/sunrpc/xdrgen/xdr_ast.py     | 42 +++++---------------------
 3 files changed, 13 insertions(+), 39 deletions(-)

-- 
2.56.0

[PATCH v1 1/4] xdrgen: Handle a union whose arms are all void

[Chuck Lever]

_XdrUnion.symbolic_width() assigns its result variable only when
an arm's width exceeds the running maximum, and the maximum starts
at zero. A union whose arms are all void has no such arm, so the
return statement reads a variable that was never bound. The
definitions command then reports "cannot access local variable
'width'" and exits without generating the header. The sibling
max_width() method starts from zero and handles the same union.

Start the symbolic width from an empty list, so an all-void union
reports just the width of its discriminant.

Fixes: ce5a75d9939f ("xdrgen: XDR width for union types")
Signed-off-by: Chuck Lever <cel@kernel.org>
---
 tools/net/sunrpc/xdrgen/xdr_ast.py | 1 +
 1 file changed, 1 insertion(+)

diff --git a/tools/net/sunrpc/xdrgen/xdr_ast.py b/tools/net/sunrpc/xdrgen/xdr_ast.py
index 5b2a78ca745c..537f056a0c27 100644
--- a/tools/net/sunrpc/xdrgen/xdr_ast.py
+++ b/tools/net/sunrpc/xdrgen/xdr_ast.py
@@ -546,6 +546,7 @@ class _XdrUnion(_XdrAst):
     def symbolic_width(self) -> List:
         """Return list containing XDR width of type's components"""
         max_width = 0
+        width = []
         for case in self.cases:
             if case.arm.max_width() > max_width:
                 max_width = case.arm.max_width()
-- 
2.56.0

[PATCH v1 2/4] xdrgen: Key the width tables by type name only

[Chuck Lever]

Every declaration class registers its width in max_widths and
symbolic_widths under its own name, and a declaration's name is the
field name when it appears inside a struct or a union. Those tables
are otherwise keyed by type name, so a field named after a type
overwrites that type's entry. In nfs_acl2.x, "unsigned int uid" in
struct fattr replaces the entry for the "uid" typedef, and the
generated NFS_ACL2_aclent_sz sizes the aclent id as XDR_unsigned_int
rather than NFS_ACL2_uid_sz. In nfs3.x, the "time_how set_it"
discriminant of set_atime replaces the entry a "bool set_it"
discriminant registered, so the generated set_mode3, set_uid3,
set_gid3, and set_size3 size macros count a time_how in place of a
bool. The macros evaluate to the right value only because the
colliding types happen to share a width.

Register a declaration's width under its name only when the
declaration is a typedef, where the name is the new type's name, and
compute a union discriminant's width from the discriminant itself
rather than looking it up by name. Regenerate the affected headers;
only size macros change.

Signed-off-by: Chuck Lever <cel@kernel.org>
---
 include/linux/sunrpc/xdrgen/nfs3.h     |  8 +++----
 include/linux/sunrpc/xdrgen/nfs_acl2.h |  2 +-
 tools/net/sunrpc/xdrgen/xdr_ast.py     | 32 +++-----------------------
 3 files changed, 8 insertions(+), 34 deletions(-)

diff --git a/include/linux/sunrpc/xdrgen/nfs3.h b/include/linux/sunrpc/xdrgen/nfs3.h
index 734f60f502c6..4e0b0afe8227 100644
--- a/include/linux/sunrpc/xdrgen/nfs3.h
+++ b/include/linux/sunrpc/xdrgen/nfs3.h
@@ -885,13 +885,13 @@ enum {
 	(XDR_bool + NFS3_nfs_fh3_sz)
 #define NFS3_time_how_sz                (XDR_int)
 #define NFS3_set_mode3_sz               \
-	(NFS3_time_how_sz + NFS3_mode3_sz)
+	(XDR_bool + NFS3_mode3_sz)
 #define NFS3_set_uid3_sz                \
-	(NFS3_time_how_sz + NFS3_uid3_sz)
+	(XDR_bool + NFS3_uid3_sz)
 #define NFS3_set_gid3_sz                \
-	(NFS3_time_how_sz + NFS3_gid3_sz)
+	(XDR_bool + NFS3_gid3_sz)
 #define NFS3_set_size3_sz               \
-	(NFS3_time_how_sz + NFS3_size3_sz)
+	(XDR_bool + NFS3_size3_sz)
 #define NFS3_set_atime_sz               \
 	(NFS3_time_how_sz + NFS3_nfstime3_sz)
 #define NFS3_set_mtime_sz               \
diff --git a/include/linux/sunrpc/xdrgen/nfs_acl2.h b/include/linux/sunrpc/xdrgen/nfs_acl2.h
index cc0b0db4c738..98dcada4786c 100644
--- a/include/linux/sunrpc/xdrgen/nfs_acl2.h
+++ b/include/linux/sunrpc/xdrgen/nfs_acl2.h
@@ -232,7 +232,7 @@ enum {
 #define NFS_ACL2_o_mode_sz              \
 	(XDR_unsigned_short)
 #define NFS_ACL2_aclent_sz              \
-	(XDR_int + XDR_unsigned_int + NFS_ACL2_o_mode_sz)
+	(XDR_int + NFS_ACL2_uid_sz + NFS_ACL2_o_mode_sz)
 #define NFS_ACL2_secattr_sz             \
 	(XDR_unsigned_int + XDR_int + XDR_unsigned_int + (NFS_ACL_MAX_ENTRIES * (NFS_ACL2_aclent_sz)) + XDR_int + XDR_unsigned_int + (NFS_ACL_MAX_ENTRIES * (NFS_ACL2_aclent_sz)))
 #define NFS_ACL2_ftype_sz               (XDR_int)
diff --git a/tools/net/sunrpc/xdrgen/xdr_ast.py b/tools/net/sunrpc/xdrgen/xdr_ast.py
index 537f056a0c27..a3595aa40a57 100644
--- a/tools/net/sunrpc/xdrgen/xdr_ast.py
+++ b/tools/net/sunrpc/xdrgen/xdr_ast.py
@@ -186,10 +186,6 @@ class _XdrFixedLengthOpaque(_XdrDeclaration):
         """Return list containing XDR width of type's components"""
         return ["XDR_QUADLEN(" + self.size + ")"]
 
-    def __post_init__(self):
-        max_widths[self.name] = self.max_width()
-        symbolic_widths[self.name] = self.symbolic_width()
-
 
 @dataclass
 class _XdrVariableLengthOpaque(_XdrDeclaration):
@@ -210,10 +206,6 @@ class _XdrVariableLengthOpaque(_XdrDeclaration):
             widths.append("XDR_QUADLEN(" + self.maxsize + ")")
         return widths
 
-    def __post_init__(self):
-        max_widths[self.name] = self.max_width()
-        symbolic_widths[self.name] = self.symbolic_width()
-
 
 @dataclass
 class _XdrString(_XdrDeclaration):
@@ -234,10 +226,6 @@ class _XdrString(_XdrDeclaration):
             widths.append("XDR_QUADLEN(" + self.maxsize + ")")
         return widths
 
-    def __post_init__(self):
-        max_widths[self.name] = self.max_width()
-        symbolic_widths[self.name] = self.symbolic_width()
-
 
 @dataclass
 class _XdrFixedLengthArray(_XdrDeclaration):
@@ -257,10 +245,6 @@ class _XdrFixedLengthArray(_XdrDeclaration):
         item_width = " + ".join(symbolic_widths[self.spec.type_name])
         return ["(" + self.size + " * (" + item_width + "))"]
 
-    def __post_init__(self):
-        max_widths[self.name] = self.max_width()
-        symbolic_widths[self.name] = self.symbolic_width()
-
 
 @dataclass
 class _XdrVariableLengthArray(_XdrDeclaration):
@@ -283,10 +267,6 @@ class _XdrVariableLengthArray(_XdrDeclaration):
             widths.append("(" + self.maxsize + " * (" + item_width + "))")
         return widths
 
-    def __post_init__(self):
-        max_widths[self.name] = self.max_width()
-        symbolic_widths[self.name] = self.symbolic_width()
-
 
 @dataclass
 class _XdrOptionalData(_XdrDeclaration):
@@ -307,8 +287,6 @@ class _XdrOptionalData(_XdrDeclaration):
     def __post_init__(self):
         structs.add(self.name)
         pass_by_reference.add(self.name)
-        max_widths[self.name] = self.max_width()
-        symbolic_widths[self.name] = self.symbolic_width()
 
 
 @dataclass
@@ -327,10 +305,6 @@ class _XdrBasic(_XdrDeclaration):
         """Return list containing XDR width of type's components"""
         return symbolic_widths[self.spec.type_name]
 
-    def __post_init__(self):
-        max_widths[self.name] = self.max_width()
-        symbolic_widths[self.name] = self.symbolic_width()
-
 
 @dataclass
 class _XdrVoid(_XdrDeclaration):
@@ -475,8 +449,8 @@ class _XdrTypedef(_XdrAst):
                     varlen_object_typedefs[new_type.name] = varlen_object_typedefs[
                         new_type.spec.type_name
                     ]
-                max_widths[new_type.name] = self.max_width()
-                symbolic_widths[new_type.name] = self.symbolic_width()
+        max_widths[self.declaration.name] = self.max_width()
+        symbolic_widths[self.declaration.name] = self.symbolic_width()
 
 
 def pages_member_maxsize(field: _XdrDeclaration):
@@ -555,7 +529,7 @@ class _XdrUnion(_XdrAst):
             if self.default.arm.max_width() > max_width:
                 max_width = self.default.arm.max_width()
                 width = self.default.arm.symbolic_width()
-        return symbolic_widths[self.discriminant.name] + width
+        return self.discriminant.symbolic_width() + width
 
     def __post_init__(self):
         structs.add(self.name)
-- 
2.56.0

[PATCH v1 3/4] xdrgen: Register optional-data names only for typedefs

[Chuck Lever]

_XdrOptionalData adds its name to structs and to pass_by_reference
when it is constructed, and that name is the field name when the
declaration is a member of a struct or a union. Both sets are
otherwise keyed by type name: _XdrDefinedType consults structs to
emit a "struct" classifier, and the encoder templates consult
pass_by_reference to pass an argument by address. A field named
after a scalar typedef turns every later reference to that typedef
into a struct passed by reference, and the generated C does not
compile. No in-tree .x file declares such a field, so the generated
headers do not change.

Add the name only when the optional-data declaration is a typedef,
beside the width registration in _XdrTypedef.

Signed-off-by: Chuck Lever <cel@kernel.org>
---
 tools/net/sunrpc/xdrgen/xdr_ast.py | 7 +++----
 1 file changed, 3 insertions(+), 4 deletions(-)

diff --git a/tools/net/sunrpc/xdrgen/xdr_ast.py b/tools/net/sunrpc/xdrgen/xdr_ast.py
index a3595aa40a57..6b012f0f7a42 100644
--- a/tools/net/sunrpc/xdrgen/xdr_ast.py
+++ b/tools/net/sunrpc/xdrgen/xdr_ast.py
@@ -284,10 +284,6 @@ class _XdrOptionalData(_XdrDeclaration):
         """Return list containing XDR width of type's components"""
         return ["XDR_bool"]
 
-    def __post_init__(self):
-        structs.add(self.name)
-        pass_by_reference.add(self.name)
-
 
 @dataclass
 class _XdrBasic(_XdrDeclaration):
@@ -440,6 +436,9 @@ class _XdrTypedef(_XdrAst):
     def __post_init__(self):
         if isinstance(self.declaration, (_XdrVariableLengthOpaque, _XdrString)):
             varlen_object_typedefs[self.declaration.name] = self.declaration.maxsize
+        if isinstance(self.declaration, _XdrOptionalData):
+            structs.add(self.declaration.name)
+            pass_by_reference.add(self.declaration.name)
         if isinstance(self.declaration, _XdrBasic):
             new_type = self.declaration
             if isinstance(new_type.spec, _XdrDefinedType):
-- 
2.56.0

[PATCH v1 4/4] xdrgen: Take a union's numeric discriminant width from the declaration

[Chuck Lever]

_XdrUnion.max_width() counts the discriminant as one XDR word, while
symbolic_width() takes the discriminant's width from the discriminant
declaration. For a hyper discriminant, or a typedef of one, the _sz
macro therefore emits two words where the numeric width counts one.
program.py compares the numeric widths of each procedure's argument
type to choose the type that the *_MAX_ARGS_SZ macro expands to, so a
union argument with a wide discriminant can lose that comparison to a
narrower type. RFC 4506 restricts the discriminant to int, unsigned
int, bool, and enum, and every in-tree .x file complies, so the
generated headers do not change.

Take the numeric discriminant width from the discriminant declaration
as well, so the two methods agree by construction.

Signed-off-by: Chuck Lever <cel@kernel.org>
---
 tools/net/sunrpc/xdrgen/xdr_ast.py | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/tools/net/sunrpc/xdrgen/xdr_ast.py b/tools/net/sunrpc/xdrgen/xdr_ast.py
index 6b012f0f7a42..a57b5f42da1f 100644
--- a/tools/net/sunrpc/xdrgen/xdr_ast.py
+++ b/tools/net/sunrpc/xdrgen/xdr_ast.py
@@ -514,7 +514,7 @@ class _XdrUnion(_XdrAst):
         if self.default:
             if self.default.arm.max_width() > max_width:
                 max_width = self.default.arm.max_width()
-        return 1 + max_width
+        return self.discriminant.max_width() + max_width
 
     def symbolic_width(self) -> List:
         """Return list containing XDR width of type's components"""
-- 
2.56.0