Here's the next set of DBus patches. This implements automatic packing and unpacking of arguments for method handlers.
This means adding a handler to a vtable is now type safe and checked by the compiler. Type safe reply xxx_finish() are also generated. Together these avoid the complexity and the pitfalls of building and parsing DBus messages via varargs (such as dbus_bool_t != bool).
Handlers have their pack/unpack method handlers automatically generated unless they have this annotation in their DBus XML definition:
<annotation name="org.freedesktop.sssd.RawHandler" value="true"/>
Only simple argument types, and arrays of them are supported. More complex arguments can continue to be handled via the "RawHandler" annotation. sbus_codegen will force you to add this annotation for methods that have complex arguments. In later work we could add commit for more complex, but common arguments such as dictionaries.
Because so many of the internal sssd DBus methods do strange things with their arguments, I haven't migrated them to automatic packing and unpacking of arguments. They are all now marked with the above "RawHandler" annotation. At some point someone could pick through these and see which ones are candidates for moving away from "RawHandler".
You can find this as a branch here:
https://github.com/stefwalter/sssd/commits/dbus-invoke
Cheers,
Stef
On 03/20/2014 08:11 AM, Stef Walter wrote:
On 14.03.2014 23:00, Stef Walter wrote:
Here's the next set of DBus patches. This implements automatic packing and unpacking of arguments for method handlers.
Rebased on master. Also the last patches had incorrectly generated files due to a rebase.
Stef
Hi, I'm sorry for the delay. Can you rebase the patches one more time please?
On Thu, Apr 17, 2014 at 02:46:06PM +0200, Pavel Březina wrote:
On 03/20/2014 08:11 AM, Stef Walter wrote:
On 14.03.2014 23:00, Stef Walter wrote:
Here's the next set of DBus patches. This implements automatic packing and unpacking of arguments for method handlers.
Rebased on master. Also the last patches had incorrectly generated files due to a rebase.
Stef
Hi, I'm sorry for the delay. Can you rebase the patches one more time please?
I took the liberty of rebasing the patches on Stef's behalf as the only conflict was in the Introspection piece I wrote. The first patch (a new one) silences the warning about undefined method.
The second and third are what used to be first and second previously.
On 04/17/2014 05:17 PM, Jakub Hrozek wrote:
On Thu, Apr 17, 2014 at 02:46:06PM +0200, Pavel Březina wrote:
On 03/20/2014 08:11 AM, Stef Walter wrote:
On 14.03.2014 23:00, Stef Walter wrote:
Here's the next set of DBus patches. This implements automatic packing and unpacking of arguments for method handlers.
Rebased on master. Also the last patches had incorrectly generated files due to a rebase.
Stef
Hi, I'm sorry for the delay. Can you rebase the patches one more time please?
I took the liberty of rebasing the patches on Stef's behalf as the only conflict was in the Introspection piece I wrote. The first patch (a new one) silences the warning about undefined method.
The second and third are what used to be first and second previously.
Thanks.
First patch should be the last one, since invoker field is not present before Stef's patches.
0001: Ack, but maybe amend the commit message that it fixes the warning.
0002: C-code ack... I didn't go through the Python code though, I left that for someone more experienced with Python.
0003: Ack.
Smoke tests went fine.
Great work Stef!
I'm attaching two more patches that fixes few warnings. Feel free to squash them into Stef's first patch.
On Fri, Apr 18, 2014 at 10:37:57AM +0200, Pavel Březina wrote:
On 04/17/2014 05:17 PM, Jakub Hrozek wrote:
On Thu, Apr 17, 2014 at 02:46:06PM +0200, Pavel Březina wrote:
On 03/20/2014 08:11 AM, Stef Walter wrote:
On 14.03.2014 23:00, Stef Walter wrote:
Here's the next set of DBus patches. This implements automatic packing and unpacking of arguments for method handlers.
Rebased on master. Also the last patches had incorrectly generated files due to a rebase.
Stef
Hi, I'm sorry for the delay. Can you rebase the patches one more time please?
I took the liberty of rebasing the patches on Stef's behalf as the only conflict was in the Introspection piece I wrote. The first patch (a new one) silences the warning about undefined method.
The second and third are what used to be first and second previously.
Thanks.
First patch should be the last one, since invoker field is not present before Stef's patches.
I've actually added the invoker in patch #2. The zero is vtable offset. It's not very clean, though, so I'm even considering including the introspection method into all the vtables..but I don't think we should block including these patches.
0001: Ack, but maybe amend the commit message that it fixes the warning.
Done.
0002: C-code ack... I didn't go through the Python code though, I left that for someone more experienced with Python.
I went through the patches as well. Ack++, the Python code looks good. I've done one more change I didn't notice until I was reviewing the patches -- the ifp.xml file should explicitly include a RawHandler for the 'ping' method. I've fixed that now and attached a new patchset.
0003: Ack.
Ack++
Smoke tests went fine.
Great work Stef!
I'm attaching two more patches that fixes few warnings. Feel free to squash them into Stef's first patch.
From c50ca1fca02a1e5d4c2d7571b27ad5171192c7b8 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Pavel=20B=C5=99ezina?= pbrezina@redhat.com Date: Fri, 18 Apr 2014 10:17:20 +0200 Subject: [PATCH 4/5] sbus_tests: fix missing invoker in initializer
ACK
From 00601045be47cfa4504e5f02fe456027163e56ef Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Pavel=20B=C5=99ezina?= pbrezina@redhat.com Date: Fri, 18 Apr 2014 10:26:14 +0200 Subject: [PATCH 5/5] sbus request: fix error initialization
fixes: sssd_dbus_request.c:28:1: error: missing initializer [-Werror=missing-field-initializers] sssd_dbus_request.c:28:1: error: (near initialization for 'error_internal.dummy1') [-Werror=missing-field-initializers]
I'm not opposed to pushing this patch, but I think the C warning is spurious. Especially when using 'static', I would have expected all the fields not set explicitly to be zeroed out. Also, I had to disable missing-field-initializers in my CFLAGS, because I've seen many warnings like this with the autogenerated code.
But your change is fine, too, I think, so if you agree with adding the RawHandler to IFP's 'ping', I'll just push the patches.
On 04/18/2014 10:59 AM, Jakub Hrozek wrote:
On Fri, Apr 18, 2014 at 10:37:57AM +0200, Pavel Březina wrote:
On 04/17/2014 05:17 PM, Jakub Hrozek wrote:
On Thu, Apr 17, 2014 at 02:46:06PM +0200, Pavel Březina wrote:
On 03/20/2014 08:11 AM, Stef Walter wrote:
On 14.03.2014 23:00, Stef Walter wrote:
Here's the next set of DBus patches. This implements automatic packing and unpacking of arguments for method handlers.
Rebased on master. Also the last patches had incorrectly generated files due to a rebase.
Stef
Hi, I'm sorry for the delay. Can you rebase the patches one more time please?
I took the liberty of rebasing the patches on Stef's behalf as the only conflict was in the Introspection piece I wrote. The first patch (a new one) silences the warning about undefined method.
The second and third are what used to be first and second previously.
Thanks.
First patch should be the last one, since invoker field is not present before Stef's patches.
I've actually added the invoker in patch #2. The zero is vtable offset. It's not very clean, though, so I'm even considering including the introspection method into all the vtables..but I don't think we should block including these patches.
Ah, ok. I missed that.
0001: Ack, but maybe amend the commit message that it fixes the warning.
Done.
0002: C-code ack... I didn't go through the Python code though, I left that for someone more experienced with Python.
I went through the patches as well. Ack++, the Python code looks good. I've done one more change I didn't notice until I was reviewing the patches -- the ifp.xml file should explicitly include a RawHandler for the 'ping' method. I've fixed that now and attached a new patchset.
Ack.
0003: Ack.
Ack++
Smoke tests went fine.
Great work Stef!
I'm attaching two more patches that fixes few warnings. Feel free to squash them into Stef's first patch.
From c50ca1fca02a1e5d4c2d7571b27ad5171192c7b8 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Pavel=20B=C5=99ezina?= pbrezina@redhat.com Date: Fri, 18 Apr 2014 10:17:20 +0200 Subject: [PATCH 4/5] sbus_tests: fix missing invoker in initializer
ACK
From 00601045be47cfa4504e5f02fe456027163e56ef Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Pavel=20B=C5=99ezina?= pbrezina@redhat.com Date: Fri, 18 Apr 2014 10:26:14 +0200 Subject: [PATCH 5/5] sbus request: fix error initialization
fixes: sssd_dbus_request.c:28:1: error: missing initializer [-Werror=missing-field-initializers] sssd_dbus_request.c:28:1: error: (near initialization for 'error_internal.dummy1') [-Werror=missing-field-initializers]
I'm not opposed to pushing this patch, but I think the C warning is spurious. Especially when using 'static', I would have expected all the fields not set explicitly to be zeroed out. Also, I had to disable missing-field-initializers in my CFLAGS, because I've seen many warnings like this with the autogenerated code.
Interesting, this was the only one I saw. I guess it's time to update from my prehistoric distro :)
But your change is fine, too, I think, so if you agree with adding the RawHandler to IFP's 'ping', I'll just push the patches.
Ack to all.
On Fri, 2014-04-18 at 15:31 +0200, Pavel Březina wrote:
But your change is fine, too, I think, so if you agree with adding the RawHandler to IFP's 'ping', I'll just push the patches.
Ack to all.
Pushed all to master. btw here is some discussion about the initialization, the topmost answer has some interesting links, too: https://stackoverflow.com/questions/1538943/why-is-the-compiler-throwing-thi...
On Sat, Apr 19, 2014 at 10:40:19AM +0200, Jakub Hrozek wrote:
On Fri, 2014-04-18 at 15:31 +0200, Pavel Březina wrote:
But your change is fine, too, I think, so if you agree with adding the RawHandler to IFP's 'ping', I'll just push the patches.
Ack to all.
Pushed all to master. btw here is some discussion about the initialization, the topmost answer has some interesting links, too: https://stackoverflow.com/questions/1538943/why-is-the-compiler-throwing-thi...
Also pushed to sssd-1-11: 70dd9c63ba4ee1e115c7194bc1f1bf0b757034ba e38c58d968594cb466163e64a43a7bfb959e48b0
On Tue, Jun 03, 2014 at 01:22:43PM +0200, Jakub Hrozek wrote:
On Sat, Apr 19, 2014 at 10:40:19AM +0200, Jakub Hrozek wrote:
On Fri, 2014-04-18 at 15:31 +0200, Pavel Březina wrote:
But your change is fine, too, I think, so if you agree with adding the RawHandler to IFP's 'ping', I'll just push the patches.
Ack to all.
Pushed all to master. btw here is some discussion about the initialization, the topmost answer has some interesting links, too: https://stackoverflow.com/questions/1538943/why-is-the-compiler-throwing-thi...
Also pushed to sssd-1-11: 70dd9c63ba4ee1e115c7194bc1f1bf0b757034ba e38c58d968594cb466163e64a43a7bfb959e48b0
Sorry, I didn't include all the hashes. The rest of the commits from master were pushed to sssd-1-11 as: 8ded3b5bd988c479ede2cadac29359c65f65d0b5 0385ce180f780e6ae5c5f8f3c5a364d2a7b06ae1 9805dad27b23a7850c80211d0ee898849c7c46c7
sssd-devel@lists.fedorahosted.org
-
Jakub Hrozek -
Jakub Hrozek -
Pavel Březina -
Stef Walter