1. Patchew
  2. Libvirt
  3. View series

[libvirt PATCH v2] scripts/rpcgen: fix 64 unsigned int test on macOS

Daniel P. Berrangé posted 1 patch 4 months, 4 weeks ago
Patches applied successfully (tree, apply log)
git fetch https://github.com/patchew-project/libvirt tags/patchew/20231130140755.54071-1-berrange@redhat.com
scripts/rpcgen/tests/test_demo.c | 4 ++++
1 file changed, 4 insertions(+)
[libvirt PATCH v2] scripts/rpcgen: fix 64 unsigned int test on macOS
Posted by Daniel P. Berrangé 4 months, 4 weeks ago 
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
_______________________________________________
Devel mailing list -- devel@lists.libvirt.org
To unsubscribe send an email to devel-leave@lists.libvirt.org
Re: [libvirt PATCH v2] scripts/rpcgen: fix 64 unsigned int test on macOS
Posted by Andrea Bolognani 4 months, 4 weeks ago 
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
_______________________________________________
Devel mailing list -- devel@lists.libvirt.org
To unsubscribe send an email to devel-leave@lists.libvirt.org
Re: [libvirt PATCH v2] scripts/rpcgen: fix 64 unsigned int test on macOS
Posted by Andrea Bolognani 4 months, 4 weeks ago 
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
_______________________________________________
Devel mailing list -- devel@lists.libvirt.org
To unsubscribe send an email to devel-leave@lists.libvirt.org
Re: [libvirt PATCH v2] scripts/rpcgen: fix 64 unsigned int test on macOS
Posted by Michal Prívozník 4 months, 4 weeks ago 
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
_______________________________________________
Devel mailing list -- devel@lists.libvirt.org
To unsubscribe send an email to devel-leave@lists.libvirt.org
Re: [libvirt PATCH v2] scripts/rpcgen: fix 64 unsigned int test on macOS
Posted by Andrea Bolognani 4 months, 4 weeks ago 
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
_______________________________________________
Devel mailing list -- devel@lists.libvirt.org
To unsubscribe send an email to devel-leave@lists.libvirt.org
Re: [libvirt PATCH v2] scripts/rpcgen: fix 64 unsigned int test on macOS
Posted by Daniel P. Berrangé 4 months, 4 weeks ago 
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 :|
_______________________________________________
Devel mailing list -- devel@lists.libvirt.org
To unsubscribe send an email to devel-leave@lists.libvirt.org
Re: [libvirt PATCH v2] scripts/rpcgen: fix 64 unsigned int test on macOS
Posted by Andrea Bolognani 4 months, 4 weeks ago 
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
_______________________________________________
Devel mailing list -- devel@lists.libvirt.org
To unsubscribe send an email to devel-leave@lists.libvirt.org
Re: [libvirt PATCH v2] scripts/rpcgen: fix 64 unsigned int test on macOS
Posted by Andrea Bolognani 4 months, 4 weeks ago 
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/R4YI4JZVWZGP6G2XZTG3SC3FEW7O3TH2/

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
_______________________________________________
Devel mailing list -- devel@lists.libvirt.org
To unsubscribe send an email to devel-leave@lists.libvirt.org
Re: [libvirt PATCH v2] scripts/rpcgen: fix 64 unsigned int test on macOS
Posted by Daniel P. Berrangé 4 months, 4 weeks ago 
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 :|
_______________________________________________
Devel mailing list -- devel@lists.libvirt.org
To unsubscribe send an email to devel-leave@lists.libvirt.org
Re: [libvirt PATCH v2] scripts/rpcgen: fix 64 unsigned int test on macOS
Posted by Daniel P. Berrangé 4 months, 4 weeks ago 
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 :|
_______________________________________________
Devel mailing list -- devel@lists.libvirt.org
To unsubscribe send an email to devel-leave@lists.libvirt.org

Patchew 2.1-devel-0870f84

© 2016 - 2024 Red Hat, Inc.