[SSSD] [PATCH] Implement type-safe getters for primitive types and their arrays

Lukas Slebodnik lslebodn at redhat.com
Fri May 16 11:09:12 UTC 2014


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



More information about the sssd-devel mailing list