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

Jakub Hrozek jhrozek at redhat.com
Thu May 22 14:01:31 UTC 2014


On Thu, May 22, 2014 at 03:36:06PM +0200, Stef Walter wrote:
> On 22.05.2014 14:00, Jakub Hrozek wrote:
> > On Thu, May 22, 2014 at 01:09:06PM +0200, Stef Walter wrote:
> >> 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
> > 
> > I was wondering that as well and wasn't sure what's the best practice in
> > the DBus world. I can only add the interface to Introspection if there
> > are properties in the meta, if that's what you'd prefer.
> > 
> > My reasoning was that you can always send the Get method to the
> > interface, even if there are no properties, the sbus handler would just
> > kick you out.
> 
> Makes sense ... and that's likely how it's implemented in a lot of
> places. Haven't checked.
> 
> Stef

OK, I will keep the patches as they are for now.

For what it's worth, I checked systemd when I implemented the patch as
systemd is one of the few projects that implement a DBus interface
without using glib.

Thanks a lot for the review, Stef, much appreciated!



More information about the sssd-devel mailing list