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