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#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
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!