Re: [PATCH 1/6] radio settings: add FastDormancy property

[Denis Kenzior <denkenz@gmail.com>]

[-- Attachment #1: Type: text/plain, Size: 1735 bytes --]

Hi Mika,

On 10/25/2010 10:05 AM, Mika.Liljeberg(a)nokia.com wrote:
> Hi Marcel,
> 
>>> @@ -103,14 +103,60 @@ static DBusMessage 
>> *radio_get_properties_reply(DBusMessage *msg,
>>>  					
>> OFONO_PROPERTIES_ARRAY_SIGNATURE,
>>>  					&dict);
>>>  
>>> -	ofono_dbus_dict_append(&dict, "TechnologyPreference",
>>> +	if ((int)rs->mode != -1) {
>>> +		const char *mode = 
>> radio_access_mode_to_string(rs->mode);
>>> +		ofono_dbus_dict_append(&dict, "TechnologyPreference",
>>>  					DBUS_TYPE_STRING, &mode);
>>
>> what is up with this (int) rs->mode cast here. That looks highly wrong
>> to me. The mode is an enum so please don't hack around it like this.
>>
>> If mode can be invalid or not present then we need to extend this enum
>> with an initial value of OFONO_RADIO_ACCESS_MODE_UNKNOWN, but not hack
>> some cast magic into it.
> 
> Yes, it's fishy. Denis introduced the enum in commit 81bc8884b414e6c2d511789d2e183cdad55182f0 but left mode initialized as -1. I'm not sure what's up with that but I did not want to start fixing it. I suppose the initializer could be added to the enum, as you say, or the whole patch could be reverted. Not my call, though.

I must have missed the -1 initialization.  In general the preference is
as follows:

- If the property is queried at the network, then set to a value that
means "unknown".  Otherwise set to a default sane value.
- Only set to the new value if the query succeeds
- If the query fails (a bizarre case if querying the modem), don't reset
the sane value and don't set cached.  The next GetProperties will try to
re-query the setting.
- Don't show the attribute if the query_ method is not provided by the
driver.

Regards,
-Denis