[SSSD] [PATCH] Implement type-safe getters for primitive types and their arrays
Stef Walter
stefw at redhat.com
Thu May 22 11:09:06 UTC 2014
On 21.05.2014 22:49, Jakub Hrozek wrote:
> On Fri, May 16, 2014 at 01:09:12PM +0200, Lukas Slebodnik wrote:
>> On (16/05/14 00:48), Jakub Hrozek wrote:
>>> On Tue, May 13, 2014 at 07:04:22PM +0200, Stef Walter wrote:
>>>> On 12.05.2014 23:27, Jakub Hrozek wrote:
>>>>> Hi,
>>>>>
>>>>> the attached patchset implement the
>>>>> org.freedesktop.DBus.Properties.Get() interface for properties of
>>>>> primitive types and arrays of primitive types (except for array of bools
>>>>> as DBus bool is of different size than C-sized bool).
>>>>
>>>> This looks nice to me. A few comments.
>>>>
>>>> + dbus_message_iter_init_append(reply, &iter);
>>>> + dbret = dbus_message_iter_open_container(&iter, DBUS_TYPE_VARIANT,
>>>> + strtype, &valiter);
>>>> + if (!dbret) {
>>>> + sbus_request_finish(dbus_req, NULL);
>>>> + ret = EINVAL;
>>>> + goto done;
>>>> + }
>>>>
>>>> You probably want some diagnostics here, as this is where you'll end up
>>>> if you pass in an invalid @type.
>>>
>>> Thanks, I was fooled by the doxygen comment that said the function only
>>> returns false on OOM. But the code is pretty conclusive, so I've added a
>>> nicer error messages for this case and the others as well.
>>>
>>>>
>>>>> + for (i = 0; i < len; i++) {
>>>>> + addr = values + i * item_size;
>>>>> + dbret = dbus_message_iter_append_basic(&array_iter, type, addr);
>>>>> + if (!dbret) {
>>>>> + sbus_request_finish(dbus_req, NULL);
>>>>> + ret = ENOMEM;
>>>>> + goto done;
>>>>> + }
>>>>> + }
>>>>
>>>> Are you planning on using this for byte arrays and such? If so it may be
>>>> worth looking into special casing arrays of bytes/ints with
>>>> dbus_message_iter_append_fixed_array()
>>>>
>>>> http://dbus.freedesktop.org/doc/api/html/group__DBusMessage.html#ga6e2d1e936c3c61fe00d80a3f22fd5e76
>>>>
>>>> But maybe that's not needed in this initial patchset?
>>>
>>> I haven't changed this part in the attached patchset, but I'm fine with
>>> the change you propose -- maybe I just don't see the rationale. Is there
>>> a performance boost? Is it worth the additional if-else?
>>>
>>>>
>>>>> From d23886c484bc955bf4bc03998601aeec1650ce02 Mon Sep 17 00:00:00 2001
>>>>> From: Stef Walter <stefw at redhat.com>
>>>>> Date: Tue, 25 Feb 2014 18:31:03 +0100
>>>>> Subject: [PATCH 4/6] WIP properties
>>>>
>>>> Should I give this a better commit message? :)
>>>
>>> Oops, that's something I meant to change before sending the patches and
>>> then didn't. The attached patchset changes the message to something
>>> hopefully better..feel free to propose a different commit message,
>>> though.
>>>
>>>>
>>>>> + if prop.type == "s":
>>>>> + out(" out_val = prop_val == NULL ? \"\" : prop_val;")
>>>>
>>>> You'll probably also want to special case object paths, returning "/" in
>>>> those cases.
>>>
>>> Yes. I also added unit tests for both these special cases.
>>>
>>> Thank you very much for the review. New patches are attached.
>>
>> I let review for others.
>>
>> I just want to write that problem with builing patches on rhle6 is fixed
>> and warnings from static analysers are gone.
>>
>> LS
>
> Thanks for running the patches through static analysers and testing the
> build!
>
> I'm attaching a new patchset that has some simplifications in the unit
> test warranted by the GetAll patchset and include the Get method in the
> introspection XML.
>
> Any comments on the patches are welcome!
ACK.
Although I do wonder if all interfaces should have the properties
interface in their introspect data unconditionally. I guess pretty much
any infopipe interface will have DBus Properties ... so this is fine for
now.
Stef
More information about the sssd-devel
mailing list