[libvirt PATCH v2] scripts/rpcgen: fix 64 unsigned int test on macOS
macOS XDR library is an oddball using xdr_u_int64_t instead of xdr_uint64_t which everyone else has. The code generator already does the right thing, but the test program previously generated with the Linux rpcgen program does not compile on macOS due to this. Signed-off-by: Daniel P. Berrangé <berrange@redhat.com> --- Changed in v2: - Put compat logic in test_demo.c instead of demo.c, since the latter is liable to be re-generated scripts/rpcgen/tests/test_demo.c | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/scripts/rpcgen/tests/test_demo.c b/scripts/rpcgen/tests/test_demo.c index d6be9e236d..94f1002ac8 100644 --- a/scripts/rpcgen/tests/test_demo.c +++ b/scripts/rpcgen/tests/test_demo.c @@ -3,6 +3,10 @@ #include <rpc/xdr.h> #include <stdbool.h> +#ifdef __APPLE__ +# define xdr_uint64_t xdr_u_int64_t +#endif + #include "demo.h" #include "demo.c" -- 2.41.0
On Thu, Nov 30, 2023 at 02:07:55PM +0000, Daniel P. Berrangé wrote:
+++ b/scripts/rpcgen/tests/test_demo.c @@ -3,6 +3,10 @@ #include <rpc/xdr.h> #include <stdbool.h>
+#ifdef __APPLE__ +# define xdr_uint64_t xdr_u_int64_t +#endif
For the long run, I think it would make more sense to have this workaround as part of the generator's output, so that using VIR_TEST_REGENERATE_OUTPUT will produce the same results regardless of whether it's run on Linux or macOS. It would also avoid the need to add a similar workaround somewhere in the library code the day we start needing uint64_t anywhere in our RPC protocol. As a short-term solution, it's fine :) Reviewed-by: Andrea Bolognani <abologna@redhat.com> -- Andrea Bolognani / Red Hat / Virtualization
On Thu, Nov 30, 2023 at 09:30:28AM -0500, Andrea Bolognani wrote:
On Thu, Nov 30, 2023 at 02:07:55PM +0000, Daniel P. Berrangé wrote:
+++ b/scripts/rpcgen/tests/test_demo.c @@ -3,6 +3,10 @@ #include <rpc/xdr.h> #include <stdbool.h>
+#ifdef __APPLE__ +# define xdr_uint64_t xdr_u_int64_t +#endif
For the long run, I think it would make more sense to have this workaround as part of the generator's output, so that using VIR_TEST_REGENERATE_OUTPUT will produce the same results regardless of whether it's run on Linux or macOS. It would also avoid the need to add a similar workaround somewhere in the library code the day we start needing uint64_t anywhere in our RPC protocol.
Long term my intention was to stop using libxdr entirely, and instead add APIS to virNetMessage to let us serialize/deserialize XDR with a less insane API :) With regards, Daniel -- |: https://berrange.com -o- https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o- https://fstop138.berrange.com :| |: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|
On Thu, Nov 30, 2023 at 09:30:28AM -0500, Andrea Bolognani wrote:
On Thu, Nov 30, 2023 at 02:07:55PM +0000, Daniel P. Berrangé wrote:
+++ b/scripts/rpcgen/tests/test_demo.c @@ -3,6 +3,10 @@ #include <rpc/xdr.h> #include <stdbool.h>
+#ifdef __APPLE__ +# define xdr_uint64_t xdr_u_int64_t +#endif
For the long run, I think it would make more sense to have this workaround as part of the generator's output, so that using VIR_TEST_REGENERATE_OUTPUT will produce the same results regardless of whether it's run on Linux or macOS. It would also avoid the need to add a similar workaround somewhere in the library code the day we start needing uint64_t anywhere in our RPC protocol.
As a short-term solution, it's fine :)
Never mind, this very obviously doesn't pass muster: E AssertionError: assert '\nvoid xdr_T...rn TRUE;\n}\n' == '\nvoid xdr_T...rn TRUE;\n}\n' E Skipping 9072 identical leading characters in diff, use -v to show E - if (!xdr_uint64_t(xdrs, &objp->suh)) E + if (!xdr_u_int64_t(xdrs, &objp->suh)) E ? + E return FALSE; E if (!xdr_bool(xdrs, &objp->sb)) E return FALSE;... E E ...Full output truncated (90 lines hidden), use '-vv' to show My test setup was a bit wonky so I initially failed to catch it. Apologies for the noise. The approach I suggested above would work, I think, but from a very quick look at the generator I wasn't able to figure out how it decides whether to use xdr_uint64_t or xdr_u_int64_t in the output. -- Andrea Bolognani / Red Hat / Virtualization
On Thu, Nov 30, 2023 at 10:05:43AM -0500, Andrea Bolognani wrote:
On Thu, Nov 30, 2023 at 09:30:28AM -0500, Andrea Bolognani wrote:
On Thu, Nov 30, 2023 at 02:07:55PM +0000, Daniel P. Berrangé wrote:
+++ b/scripts/rpcgen/tests/test_demo.c @@ -3,6 +3,10 @@ #include <rpc/xdr.h> #include <stdbool.h>
+#ifdef __APPLE__ +# define xdr_uint64_t xdr_u_int64_t +#endif
For the long run, I think it would make more sense to have this workaround as part of the generator's output, so that using VIR_TEST_REGENERATE_OUTPUT will produce the same results regardless of whether it's run on Linux or macOS. It would also avoid the need to add a similar workaround somewhere in the library code the day we start needing uint64_t anywhere in our RPC protocol.
As a short-term solution, it's fine :)
Never mind, this very obviously doesn't pass muster:
E AssertionError: assert '\nvoid xdr_T...rn TRUE;\n}\n' == '\nvoid xdr_T...rn TRUE;\n}\n' E Skipping 9072 identical leading characters in diff, use -v to show E - if (!xdr_uint64_t(xdrs, &objp->suh)) E + if (!xdr_u_int64_t(xdrs, &objp->suh)) E ? + E return FALSE; E if (!xdr_bool(xdrs, &objp->sb)) E return FALSE;... E E ...Full output truncated (90 lines hidden), use '-vv' to show
My test setup was a bit wonky so I initially failed to catch it. Apologies for the noise.
The approach I suggested above would work, I think, but from a very quick look at the generator I wasn't able to figure out how it decides whether to use xdr_uint64_t or xdr_u_int64_t in the output.
In scripts/rpcgen/rpcgen/generator.py this method: def visit_type_unsignedhyper(self, obj, indent, context): if context == "func" and platform.system() == "Darwin": return "%su_int64_t" % indent else: return "%suint64_t" % indent With regards, Daniel -- |: https://berrange.com -o- https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o- https://fstop138.berrange.com :| |: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|
On 11/30/23 16:05, Andrea Bolognani wrote:
On Thu, Nov 30, 2023 at 09:30:28AM -0500, Andrea Bolognani wrote:
On Thu, Nov 30, 2023 at 02:07:55PM +0000, Daniel P. Berrangé wrote:
+++ b/scripts/rpcgen/tests/test_demo.c @@ -3,6 +3,10 @@ #include <rpc/xdr.h> #include <stdbool.h>
+#ifdef __APPLE__ +# define xdr_uint64_t xdr_u_int64_t +#endif
For the long run, I think it would make more sense to have this workaround as part of the generator's output, so that using VIR_TEST_REGENERATE_OUTPUT will produce the same results regardless of whether it's run on Linux or macOS. It would also avoid the need to add a similar workaround somewhere in the library code the day we start needing uint64_t anywhere in our RPC protocol.
As a short-term solution, it's fine :)
Never mind, this very obviously doesn't pass muster:
E AssertionError: assert '\nvoid xdr_T...rn TRUE;\n}\n' == '\nvoid xdr_T...rn TRUE;\n}\n' E Skipping 9072 identical leading characters in diff, use -v to show E - if (!xdr_uint64_t(xdrs, &objp->suh)) E + if (!xdr_u_int64_t(xdrs, &objp->suh)) E ? + E return FALSE; E if (!xdr_bool(xdrs, &objp->sb)) E return FALSE;... E E ...Full output truncated (90 lines hidden), use '-vv' to show
My test setup was a bit wonky so I initially failed to catch it. Apologies for the noise.
The approach I suggested above would work, I think, but from a very quick look at the generator I wasn't able to figure out how it decides whether to use xdr_uint64_t or xdr_u_int64_t in the output.
libtirpc on Linux has both: # grep -e xdr_u_int64_t -e xdr_uint64_t /usr/include/tirpc/rpc/xdr.h extern bool_t xdr_u_int64_t(XDR *, u_int64_t *); extern bool_t xdr_uint64_t(XDR *, uint64_t *); the macos library has only xdr_u_int64_t so we can hack the generator to use the latter unconditionally. Michal
On Thu, Nov 30, 2023 at 04:10:00PM +0100, Michal Prívozník wrote:
On 11/30/23 16:05, Andrea Bolognani wrote:
On Thu, Nov 30, 2023 at 09:30:28AM -0500, Andrea Bolognani wrote:
On Thu, Nov 30, 2023 at 02:07:55PM +0000, Daniel P. Berrangé wrote:
+++ b/scripts/rpcgen/tests/test_demo.c @@ -3,6 +3,10 @@ #include <rpc/xdr.h> #include <stdbool.h>
+#ifdef __APPLE__ +# define xdr_uint64_t xdr_u_int64_t +#endif
For the long run, I think it would make more sense to have this workaround as part of the generator's output, so that using VIR_TEST_REGENERATE_OUTPUT will produce the same results regardless of whether it's run on Linux or macOS. It would also avoid the need to add a similar workaround somewhere in the library code the day we start needing uint64_t anywhere in our RPC protocol.
As a short-term solution, it's fine :)
Never mind, this very obviously doesn't pass muster:
E AssertionError: assert '\nvoid xdr_T...rn TRUE;\n}\n' == '\nvoid xdr_T...rn TRUE;\n}\n' E Skipping 9072 identical leading characters in diff, use -v to show E - if (!xdr_uint64_t(xdrs, &objp->suh)) E + if (!xdr_u_int64_t(xdrs, &objp->suh)) E ? + E return FALSE; E if (!xdr_bool(xdrs, &objp->sb)) E return FALSE;... E E ...Full output truncated (90 lines hidden), use '-vv' to show
My test setup was a bit wonky so I initially failed to catch it. Apologies for the noise.
The approach I suggested above would work, I think, but from a very quick look at the generator I wasn't able to figure out how it decides whether to use xdr_uint64_t or xdr_u_int64_t in the output.
libtirpc on Linux has both:
# grep -e xdr_u_int64_t -e xdr_uint64_t /usr/include/tirpc/rpc/xdr.h extern bool_t xdr_u_int64_t(XDR *, u_int64_t *); extern bool_t xdr_uint64_t(XDR *, uint64_t *);
the macos library has only xdr_u_int64_t so we can hack the generator to use the latter unconditionally.
We could have test_generator.p to only honour VIR_TEST_REGENERATE_OUTPUT on Linux. With regards, Daniel -- |: https://berrange.com -o- https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o- https://fstop138.berrange.com :| |: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|
On Thu, Nov 30, 2023 at 03:17:10PM +0000, Daniel P. Berrangé wrote:
On Thu, Nov 30, 2023 at 04:10:00PM +0100, Michal Prívozník wrote:
On 11/30/23 16:05, Andrea Bolognani wrote:
On Thu, Nov 30, 2023 at 09:30:28AM -0500, Andrea Bolognani wrote:
On Thu, Nov 30, 2023 at 02:07:55PM +0000, Daniel P. Berrangé wrote:
+++ b/scripts/rpcgen/tests/test_demo.c @@ -3,6 +3,10 @@ #include <rpc/xdr.h> #include <stdbool.h>
+#ifdef __APPLE__ +# define xdr_uint64_t xdr_u_int64_t +#endif
For the long run, I think it would make more sense to have this workaround as part of the generator's output, so that using VIR_TEST_REGENERATE_OUTPUT will produce the same results regardless of whether it's run on Linux or macOS. It would also avoid the need to add a similar workaround somewhere in the library code the day we start needing uint64_t anywhere in our RPC protocol.
As a short-term solution, it's fine :)
Never mind, this very obviously doesn't pass muster:
E AssertionError: assert '\nvoid xdr_T...rn TRUE;\n}\n' == '\nvoid xdr_T...rn TRUE;\n}\n' E Skipping 9072 identical leading characters in diff, use -v to show E - if (!xdr_uint64_t(xdrs, &objp->suh)) E + if (!xdr_u_int64_t(xdrs, &objp->suh)) E ? + E return FALSE; E if (!xdr_bool(xdrs, &objp->sb)) E return FALSE;... E E ...Full output truncated (90 lines hidden), use '-vv' to show
My test setup was a bit wonky so I initially failed to catch it. Apologies for the noise.
The approach I suggested above would work, I think, but from a very quick look at the generator I wasn't able to figure out how it decides whether to use xdr_uint64_t or xdr_u_int64_t in the output.
libtirpc on Linux has both:
# grep -e xdr_u_int64_t -e xdr_uint64_t /usr/include/tirpc/rpc/xdr.h extern bool_t xdr_u_int64_t(XDR *, u_int64_t *); extern bool_t xdr_uint64_t(XDR *, uint64_t *);
the macos library has only xdr_u_int64_t so we can hack the generator to use the latter unconditionally.
We could have test_generator.p to only honour VIR_TEST_REGENERATE_OUTPUT on Linux.
That wouldn't help at all: on macOS, you would still be comparing the output produced on Linux (committed to the repo) to the one produced on macOS (generated on the fly) and they would still differ, causing the test to fail. -- Andrea Bolognani / Red Hat / Virtualization
On Thu, Nov 30, 2023 at 10:27:34AM -0500, Andrea Bolognani wrote:
As a short-term solution, it's fine :)
Never mind, this very obviously doesn't pass muster:
E AssertionError: assert '\nvoid xdr_T...rn TRUE;\n}\n' == '\nvoid xdr_T...rn TRUE;\n}\n' E Skipping 9072 identical leading characters in diff, use -v to show E - if (!xdr_uint64_t(xdrs, &objp->suh)) E + if (!xdr_u_int64_t(xdrs, &objp->suh)) E ? + E return FALSE; E if (!xdr_bool(xdrs, &objp->sb)) E return FALSE;... E E ...Full output truncated (90 lines hidden), use '-vv' to show
My test setup was a bit wonky so I initially failed to catch it. Apologies for the noise.
In the interest of getting 9.10.0 out of the door according to the original schedule, with reasonable test coverage and no broken test suite on macOS, please consider: https://lists.libvirt.org/archives/list/devel@lists.libvirt.org/thread/R4YI4... If that patch is acceptable to you, you can retain my ACK to this patch of yours and push it. I'll push my patches tomorrow. -- Andrea Bolognani / Red Hat / Virtualization
On Thu, Nov 30, 2023 at 04:10:00PM +0100, Michal Prívozník wrote:
On 11/30/23 16:05, Andrea Bolognani wrote:
On Thu, Nov 30, 2023 at 09:30:28AM -0500, Andrea Bolognani wrote:
On Thu, Nov 30, 2023 at 02:07:55PM +0000, Daniel P. Berrangé wrote:
+++ b/scripts/rpcgen/tests/test_demo.c @@ -3,6 +3,10 @@ #include <rpc/xdr.h> #include <stdbool.h>
+#ifdef __APPLE__ +# define xdr_uint64_t xdr_u_int64_t +#endif
For the long run, I think it would make more sense to have this workaround as part of the generator's output, so that using VIR_TEST_REGENERATE_OUTPUT will produce the same results regardless of whether it's run on Linux or macOS. It would also avoid the need to add a similar workaround somewhere in the library code the day we start needing uint64_t anywhere in our RPC protocol.
As a short-term solution, it's fine :)
Never mind, this very obviously doesn't pass muster:
E AssertionError: assert '\nvoid xdr_T...rn TRUE;\n}\n' == '\nvoid xdr_T...rn TRUE;\n}\n' E Skipping 9072 identical leading characters in diff, use -v to show E - if (!xdr_uint64_t(xdrs, &objp->suh)) E + if (!xdr_u_int64_t(xdrs, &objp->suh)) E ? + E return FALSE; E if (!xdr_bool(xdrs, &objp->sb)) E return FALSE;... E E ...Full output truncated (90 lines hidden), use '-vv' to show
My test setup was a bit wonky so I initially failed to catch it. Apologies for the noise.
The approach I suggested above would work, I think, but from a very quick look at the generator I wasn't able to figure out how it decides whether to use xdr_uint64_t or xdr_u_int64_t in the output.
libtirpc on Linux has both:
# grep -e xdr_u_int64_t -e xdr_uint64_t /usr/include/tirpc/rpc/xdr.h extern bool_t xdr_u_int64_t(XDR *, u_int64_t *); extern bool_t xdr_uint64_t(XDR *, uint64_t *);
the macos library has only xdr_u_int64_t so we can hack the generator to use the latter unconditionally.
As Daniel pointed out earlier, portablexdr only has xdr_uint64_t so that would break our Windows builds. I think we should just always emit xdr_uint64_t and include the #ifdef __APPLE__ workaround in the output. That should make things work everywhere. -- Andrea Bolognani / Red Hat / Virtualization
participants (3)
-
Andrea Bolognani -
Daniel P. Berrangé -
Michal Prívozník