On (26/05/14 15:53), Jakub Hrozek wrote:
On Sat, May 24, 2014 at 02:13:28PM +0200, Lukas Slebodnik wrote:
> On (23/05/14 17:39), Jakub Hrozek wrote:
> >On Fri, May 23, 2014 at 11:56:32AM +0200, Jakub Hrozek wrote:
> >> On Fri, May 23, 2014 at 08:31:49AM +0200, Lukas Slebodnik wrote:
> >> > On (21/05/14 22:53), Jakub Hrozek wrote:
> >> > >Hi,
> >> > >
> >> > >the attached patches implement the
> >> > >org.freedesktop.DBus.Properties.GetAll method for retrieving
primitive
> >> > >types and their arrays.
> >> > >
> >> > >The patchset build on the previous set (currently still on review)
that
> >> > >implements the .Get method. The GetAll handler simply walks through
the
> >> > >existing getters and retrieves the results, storing them in a
single
> >> > >variant.
> >> > >
> >> > >The patches include a unit test that tests all currently supported
> >> > >combinations of primitive types and array of primitive types.
> >> >
> >> > >From fc4b278c0914b2c7001393e471f4f6f0b532ba54 Mon Sep 17 00:00:00
2001
> >> > >From: Jakub Hrozek <jhrozek(a)redhat.com>
> >> > >Date: Wed, 21 May 2014 16:48:45 +0200
> >> > >Subject: [PATCH 1/5] SBUS: Add utility function
sbus_add_variant_to_dict
> >> > >
> >> > >---
> >> > > src/sbus/sssd_dbus.h | 5 ++
> >> > > src/sbus/sssd_dbus_properties.c | 116
++++++++++++++++++++++++++++++++++++++++
> >> > > 2 files changed, 121 insertions(+)
> >> > >
> >> > src/sbus/sssd_dbus_properties.c:287:41: error: no member named
'invoker_get_all' in
> >> > 'struct sbus_interface_meta'
> >> > meta->invoker_get_all);
> >> > ~~~~ ^
> >> > It is introduced in 4th aptch
> >> > "SBUS: Implement org.freedesktop.DBus.Properties.GetAll for
primitive types"
> >> >
> >> > >From 32d84445012991dba38ee5b60f5614aca70b889d Mon Sep 17 00:00:00
2001
> >> > >From: Jakub Hrozek <jhrozek(a)redhat.com>
> >> > >Date: Wed, 21 May 2014 20:10:23 +0200
> >> > >Subject: [PATCH 5/5] SBUS: Add
org.freedesktop.DBus.Properties.GetAll to
> >> > > Introspection
> >> >
> >> > FAIL: sbus_tests
> >> >
> >> > sbys_tests.log file is attached.
> >> >
> >> > LS
> >>
> >> Ah, I didn't amend the test to include GetAll in introspection like I
> >> did with Get, thanks. I'll prepare a new set.
> >
> >Hi,
> >
> >new patches are attached, but only the Introspection patch had changed
> >-- I added the GetAll XML to the unit test.
>
> >From 9511be0f60affc2909ec2388f0d1ab5cc8487647 Mon Sep 17 00:00:00 2001
> >From: Jakub Hrozek <jhrozek(a)redhat.com>
> >Date: Wed, 21 May 2014 21:29:15 +0200
> >Subject: [PATCH 4/5] SBUS: Implement org.freedesktop.DBus.Properties.GetAll
> > for primitive types
> >
> >This patch implements the GetAll method of the
> >org.freedesktop.DBus.Properties interface by iterating over the
> >available getters and putting all the results into a single getter.
> >
> >The patch includes a unit test that exercies all currently supported
> >array types.
> >---
> > src/monitor/monitor_iface_generated.c | 2 +
> > src/providers/data_provider_iface_generated.c | 2 +
> > src/responder/ifp/ifp_iface_generated.c | 1 +
> > src/sbus/sbus_codegen | 96 +++++-
> > src/sbus/sssd_dbus_meta.h | 1 +
> > src/tests/sbus_codegen_tests.c | 299 +++++++++++++++++++
> > src/tests/sbus_codegen_tests_generated.c | 415 +++++++++++++++++++++++++-
> > 7 files changed, 806 insertions(+), 10 deletions(-)
> >
>
> //snip
>
> >diff --git a/src/sbus/sssd_dbus_meta.h b/src/sbus/sssd_dbus_meta.h
> >index
f25b1ac28abf1b1ef53fa54a090fa84ef868ab77..2cb9e95e3e12da55b1f101e076a0e05203984215 100644
> >--- a/src/sbus/sssd_dbus_meta.h
> >+++ b/src/sbus/sssd_dbus_meta.h
> >@@ -80,6 +80,7 @@ struct sbus_interface_meta {
> > const struct sbus_method_meta *methods;
> > const struct sbus_signal_meta *signals;
> > const struct sbus_property_meta *properties;
> >+ sbus_method_invoker_fn invoker_get_all;
> > };
> >
> Move this change to the 1st patch.
Fixed.
>
> There is little problem with introspection.
>
> sh# gdbus introspect --system --dest org.freedesktop.sssd.infopipe --object-path /
--recurse
>
> ==11539== Jump to the invalid address stated on the next line
> ==11539== at 0x0: ???
> ==11539== by 0x7C3E25C: sbus_request_invoke_or_finish (sssd_dbus_request.c:69)
> ==11539== by 0x7C3DC47: sbus_properties_dispatch (sssd_dbus_properties.c:361)
> ==11539== by 0x7C3BD87: sbus_handler_got_caller_id (sssd_dbus_connection.c:524)
> ==11539== by 0x4C2D753: tevent_common_loop_immediate (tevent_immediate.c:135)
> ==11539== by 0x4C31FBD: epoll_event_loop_once (tevent_epoll.c:908)
> ==11539== by 0x4C306B6: std_event_loop_once (tevent_standard.c:112)
> ==11539== by 0x4C2CF2C: _tevent_loop_once (tevent.c:530)
> ==11539== by 0x4C2D0CA: tevent_common_loop_wait (tevent.c:634)
> ==11539== by 0x4C30656: std_event_loop_wait (tevent_standard.c:138)
> ==11539== by 0x7C42C12: server_loop (server.c:587)
> ==11539== by 0x10C571: main (ifpsrv.c:397)
> ==11539== Address 0x0 is not stack'd, malloc'd or (recently) free'd
> ==11539==
> ==11539==
> ==11539== Process terminating with default action of signal 11 (SIGSEGV)
> ==11539== Bad permissions for mapped region at address 0x0
> ==11539== at 0x0: ???
> ==11539== by 0x7C3E25C: sbus_request_invoke_or_finish (sssd_dbus_request.c:69)
> ==11539== by 0x7C3DC47: sbus_properties_dispatch (sssd_dbus_properties.c:361)
> ==11539== by 0x7C3BD87: sbus_handler_got_caller_id (sssd_dbus_connection.c:524)
> ==11539== by 0x4C2D753: tevent_common_loop_immediate (tevent_immediate.c:135)
> ==11539== by 0x4C31FBD: epoll_event_loop_once (tevent_epoll.c:908)
> ==11539== by 0x4C306B6: std_event_loop_once (tevent_standard.c:112)
> ==11539== by 0x4C2CF2C: _tevent_loop_once (tevent.c:530)
> ==11539== by 0x4C2D0CA: tevent_common_loop_wait (tevent.c:634)
> ==11539== by 0x4C30656: std_event_loop_wait (tevent_standard.c:138)
> ==11539== by 0x7C42C12: server_loop (server.c:587)
> ==11539== by 0x10C571: main (ifpsrv.c:397)
> ==11539==
> ==11539== HEAP SUMMARY:
> ==11539== in use at exit: 93,205 bytes in 449 blocks
> ==11539== total heap usage: 3,296 allocs, 2,847 frees, 440,463 bytes allocated
>
Thanks, I didn't see this earlier because I never used --recurse.
I modified the codegen a bit so than an invoker is called even if there
are no properties. The invoker creates an empty array, adds nothing (the
property getters would be inserted here by codegen) and returns.
The other way I was considering was checking for properties in the
getall_handler in sssd, but I prefer to let the generated code do as
much work as possible.
Crash is fixed.
Static analysers did not found any issues.
There are few gcc warnings.
CC src/tests/sbus_tests-sbus_tests.o
src/tests/sbus_tests.c:105:1: error: missing initializer for field ‘invoker_get_all’ of
‘const struct sbus_interface_meta’ [-Werror=missing-field-initializers]
};
^
In file included from ../sssd/src/tests/sbus_tests.c:34:0:
src/sbus/sssd_dbus_meta.h:83:28: note: ‘invoker_get_all’ declared here
sbus_method_invoker_fn invoker_get_all;
^
cc1: all warnings being treated as errors
Warning can be fixed with diff:
diff --git a/src/tests/sbus_tests.c b/src/tests/sbus_tests.c
index 31b91c4..fe6ee05 100644
--- a/src/tests/sbus_tests.c
+++ b/src/tests/sbus_tests.c
@@ -102,6 +102,7 @@ const struct sbus_interface_meta pilot_meta = {
pilot_methods,
NULL, /* no signals */
NULL, /* no properties */
+ NULL, /* invoker function */
};
static int blink_handler(struct sbus_request *req, void *data)
And the second warning:
src/tests/sbus_codegen_tests.c: In function ‘check_arr_prop’:
src/tests/sbus_codegen_tests.c:1226:30: error: ‘strings’ may be used uninitialized in this
function [-Werror=maybe-uninitialized]
ck_assert_str_eq(strings[1], pilot_path_array[1]);
^
cc1: all warnings being treated as errors
I am not sure about solution :-)
LS