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#ga6e2d1e...
>
> 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(a)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