11:56 p.m.
According to dbus documentation, dbus_message_iter_append_basic()
expects a "const char**" if type is string and a simple pointer for
other types. Since we a iterating an array, the value passed is already
a pointer.
---
src/dbus.c | 9 +++++++--
1 files changed, 7 insertions(+), 2 deletions(-)
diff --git a/src/dbus.c b/src/dbus.c
index c24615f..292f11a 100644
--- a/src/dbus.c
+++ b/src/dbus.c
@@ -139,8 +139,13 @@ static void append_dict_variant(DBusMessageIter *iter, int type, void
*val)
dbus_message_iter_append_basic(&entry, DBUS_TYPE_STRING,
&(val_array[i + 0]));
- dbus_message_iter_append_basic(&entry, type,
- &(val_array[i + 1]));
+ if (type == DBUS_TYPE_STRING) {
+ dbus_message_iter_append_basic(&entry, type,
+ &(val_array[i + 1]));
+ } else {
+ dbus_message_iter_append_basic(&entry, type,
+ val_array[i + 1]);
+ }
dbus_message_iter_close_container(&array, &entry);
}
--
1.7.3.4
4:31 a.m.
Hi Lucas,
According to dbus documentation, dbus_message_iter_append_basic()
expects a "const char**" if type is string and a simple pointer for
other types. Since we a iterating an array, the value passed is already
a pointer.
---
src/dbus.c | 9 +++++++--
1 files changed, 7 insertions(+), 2 deletions(-)
diff --git a/src/dbus.c b/src/dbus.c
index c24615f..292f11a 100644
--- a/src/dbus.c
+++ b/src/dbus.c
@@ -139,8 +139,13 @@ static void append_dict_variant(DBusMessageIter *iter, int type,
void *val)
dbus_message_iter_append_basic(&entry, DBUS_TYPE_STRING,
&(val_array[i + 0]));
- dbus_message_iter_append_basic(&entry, type,
- &(val_array[i + 1]));
+ if (type == DBUS_TYPE_STRING) {
+ dbus_message_iter_append_basic(&entry, type,
+ &(val_array[i + 1]));
+ } else {
+ dbus_message_iter_append_basic(&entry, type,
+ val_array[i + 1]);
+ }
this does looks a bit hackish. Can you put it into context please. It
might be that our call chain is wrong.
Regards
Marcel
4:53 a.m.
Hi Marcel,
>
> dbus_message_iter_append_basic(&entry, DBUS_TYPE_STRING,
> &(val_array[i + 0]));
> - dbus_message_iter_append_basic(&entry, type,
> - &(val_array[i + 1]));
> + if (type == DBUS_TYPE_STRING) {
> + dbus_message_iter_append_basic(&entry, type,
> + &(val_array[i + 1]));
> + } else {
> + dbus_message_iter_append_basic(&entry, type,
> + val_array[i + 1]);
> + }
this does looks a bit hackish. Can you put it into context please. It
might be that our call chain is wrong.
We only ever needed (and thus ever added) support for string:string
dicts. E.g. ones that could be g_strfreev-ed. No other dicts were
supported by this function.
What Lucas wants is a dict{string,byte}
Regards,
-Denis
4:56 a.m.
Hi Marcel,
On Tue, Jan 4, 2011 at 3:31 PM, Marcel Holtmann <marcel(a)holtmann.org> wrote:
> diff --git a/src/dbus.c b/src/dbus.c
> index c24615f..292f11a 100644
> --- a/src/dbus.c
> +++ b/src/dbus.c
> @@ -139,8 +139,13 @@ static void append_dict_variant(DBusMessageIter *iter, int type,
void *val)
>
> dbus_message_iter_append_basic(&entry, DBUS_TYPE_STRING,
> &(val_array[i + 0]));
> - dbus_message_iter_append_basic(&entry, type,
> - &(val_array[i + 1]));
> + if (type == DBUS_TYPE_STRING) {
> + dbus_message_iter_append_basic(&entry, type,
> + &(val_array[i + 1]));
> + } else {
> + dbus_message_iter_append_basic(&entry, type,
> + val_array[i + 1]);
> + }
this does looks a bit hackish. Can you put it into context please. It
might be that our call chain is wrong.
As of now there are no calls to ofono_dbus_dict_append_dict() with
type != string. So, there's no problem in current code base. The
problem arrives when I call, for example:
void **array;
array = g_new0(void *, 4);
array[0] = a_string;
array[1] = &a_byte_var;
array[2] = b_string;
array[3] = &b_byte_var;
then if i call:
ofono_dbus_dict_append_dict(&dict, "MyPropName", DBUS_TYPE_BYTE,
&array);
It will not work, because of the following call chain:
ofono_dbus_dict_append_dict()
append_dict_variant(... , void *val)
const void **val_array = *(const void ***) val;
(...)
dbus_message_iter_append_basic(&entry, DBUS_TYPE_BYTE,
&(val_array[i + 1]);
So, you are passing a const void** to dbus_message_iter_append_basic
that will be dereferenced accordingly inside dbus to get the type you
told it. Then, bad things will happen. I've just tested here with my
patches for the retry counter, where this call is needed.
regards,
Lucas De Marchi
4:57 a.m.
Hi Denis,
>>
>> dbus_message_iter_append_basic(&entry, DBUS_TYPE_STRING,
>> &(val_array[i + 0]));
>> - dbus_message_iter_append_basic(&entry, type,
>> - &(val_array[i + 1]));
>> + if (type == DBUS_TYPE_STRING) {
>> + dbus_message_iter_append_basic(&entry, type,
>> + &(val_array[i + 1]));
>> + } else {
>> + dbus_message_iter_append_basic(&entry, type,
>> + val_array[i + 1]);
>> + }
>
> this does looks a bit hackish. Can you put it into context please. It
> might be that our call chain is wrong.
>
We only ever needed (and thus ever added) support for string:string
dicts. E.g. ones that could be g_strfreev-ed. No other dicts were
supported by this function.
so it is the g_strfreev part that makes this hackish. Fine by me then,
but we might need to comment on why string arrays in this case are
special.
Regards
Marcel
5:13 a.m.
On Tue, Jan 4, 2011 at 3:57 PM, Marcel Holtmann <marcel(a)holtmann.org> wrote:
Hi Denis,
> >>
> >> dbus_message_iter_append_basic(&entry, DBUS_TYPE_STRING,
> >> &(val_array[i + 0]));
> >> - dbus_message_iter_append_basic(&entry, type,
> >> - &(val_array[i + 1]));
> >> + if (type == DBUS_TYPE_STRING) {
> >> + dbus_message_iter_append_basic(&entry, type,
> >> + &(val_array[i +
1]));
> >> + } else {
> >> + dbus_message_iter_append_basic(&entry, type,
> >> + val_array[i + 1]);
> >> + }
> >
> > this does looks a bit hackish. Can you put it into context please. It
> > might be that our call chain is wrong.
> >
>
> We only ever needed (and thus ever added) support for string:string
> dicts. E.g. ones that could be g_strfreev-ed. No other dicts were
> supported by this function.
so it is the g_strfreev part that makes this hackish. Fine by me then,
but we might need to comment on why string arrays in this case are
special.
I don't think so. You use g_strfreev if you are using { string, string
} dict to free all strings. Why is this hackish?
Even if we had another interface that didn't required the g_strfreev,
the issue with calling dbus_message_iter_append_basic() would not
change once we continue using a void** to store pointers to different
types at the same time..
Lucas De Marchi
5:56 a.m.
Hi Lucas,
>> >>
>> >> dbus_message_iter_append_basic(&entry,
DBUS_TYPE_STRING,
>> >> &(val_array[i + 0]));
>> >> - dbus_message_iter_append_basic(&entry, type,
>> >> - &(val_array[i + 1]));
>> >> + if (type == DBUS_TYPE_STRING) {
>> >> + dbus_message_iter_append_basic(&entry, type,
>> >> + &(val_array[i +
1]));
>> >> + } else {
>> >> + dbus_message_iter_append_basic(&entry, type,
>> >> + val_array[i + 1]);
>> >> + }
>> >
>> > this does looks a bit hackish. Can you put it into context please. It
>> > might be that our call chain is wrong.
>> >
>>
>> We only ever needed (and thus ever added) support for string:string
>> dicts. E.g. ones that could be g_strfreev-ed. No other dicts were
>> supported by this function.
>
> so it is the g_strfreev part that makes this hackish. Fine by me then,
> but we might need to comment on why string arrays in this case are
> special.
I don't think so. You use g_strfreev if you are using { string, string
} dict to free all strings. Why is this hackish?
Even if we had another interface that didn't required the g_strfreev,
the issue with calling dbus_message_iter_append_basic() would not
change once we continue using a void** to store pointers to different
types at the same time..
the append_basic D-Bus function works fine on **char and *uint8. So
either a pointer to a string (*char) or pointer to a byte (uint8.
You differentiate between pointer to a string and pointer to a byte in
that function. So it is different since normally it is the caller that
has to give the right pointer in the first place.
Regards
Marcel
6:46 a.m.
Hi, Marcel
On Tue, Jan 4, 2011 at 4:56 PM, Marcel Holtmann <marcel(a)holtmann.org> wrote:
the append_basic D-Bus function works fine on **char and *uint8. So
either a pointer to a string (*char) or pointer to a byte (uint8.
You differentiate between pointer to a string and pointer to a byte in
that function. So it is different since normally it is the caller that
has to give the right pointer in the first place.
Yeah, true... But if we don't differentiate the types here I'd need to
cast an uint8 to void* that is not even allowed by our warning flags
Lucas De Marchi
7:11 a.m.
Hi Lucas,
> the append_basic D-Bus function works fine on **char and *uint8.
So
> either a pointer to a string (*char) or pointer to a byte (uint8.
>
> You differentiate between pointer to a string and pointer to a byte in
> that function. So it is different since normally it is the caller that
> has to give the right pointer in the first place.
Yeah, true... But if we don't differentiate the types here I'd need to
cast an uint8 to void* that is not even allowed by our warning flags
as I said, I am fine with checking the type, but I would like to see a
comment in the code why we differentiate in this case.
Regards
Marcel
9:16 a.m.
According to dbus documentation, dbus_message_iter_append_basic()
expects a "const char**" if type is string and a simple pointer for
other types. Since we a iterating an array, the value passed is already
a pointer.
---
src/dbus.c | 20 ++++++++++++++++++--
1 files changed, 18 insertions(+), 2 deletions(-)
diff --git a/src/dbus.c b/src/dbus.c
index c24615f..b719217 100644
--- a/src/dbus.c
+++ b/src/dbus.c
@@ -139,8 +139,24 @@ static void append_dict_variant(DBusMessageIter *iter, int type, void
*val)
dbus_message_iter_append_basic(&entry, DBUS_TYPE_STRING,
&(val_array[i + 0]));
- dbus_message_iter_append_basic(&entry, type,
- &(val_array[i + 1]));
+
+ /*
+ * D-Bus expects a char** or uint8* depending on the type
+ * given. Since we are dealing with an array through a void**
+ * (and thus val_array[i] is a pointer) we need to
+ * differentiate DBUS_TYPE_STRING from the others. The other
+ * option would be the user to pass the exact type to this
+ * function, instead of a pointer to it. However in this case
+ * a cast from type to void* would be needed, which is not
+ * good.
+ */
+ if (type == DBUS_TYPE_STRING) {
+ dbus_message_iter_append_basic(&entry, type,
+ &(val_array[i + 1]));
+ } else {
+ dbus_message_iter_append_basic(&entry, type,
+ val_array[i + 1]);
+ }
dbus_message_iter_close_container(&array, &entry);
}
--
1.7.3.4
4275
days inactive
4275
days old
9 comments
3 participants
participants (3)
-
Denis Kenzior -
Lucas De Marchi -
Marcel Holtmann