Re: [PATCH 1/4] radio settings: allow for more than one property

[Denis Kenzior <denkenz@gmail.com>]

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

Hi Mika,

On 10/21/2010 03:55 AM, Mika.Liljeberg(a)nokia.com wrote:
> Hi,
> 
>> Please don't mix and match tabs and spaces for indentation
> 
> Oops, will fix. For some reason my checkpatch script did not complain about this.
> 
> You must be using a script as well. To avoid unnecessary fuss like this, would it be possible to integrate it to the oFono tree so everyone would check their patches using the same thing?

Not a script, but Johan's magic vim macro.  I paste it here for reference:
highlight RedundantWhitespace ctermbg=red guibg=red
match RedundantWhitespace /\s\+$\|\t\+\ze \+[^*]\| \+\ze\t/

Between checkpatch, git am and this one we catch most issues.

> 
>> I prefer you keep the current implementation as it is consistent with
>> the rest of ofono core.  The way the rest of the core works 
>> is that you
>> query each in sequence, then set the the CACHED flag (only one is
>> actually required).
>>
>> We use a single cached flag to keep the logic simpler and optimize for
>> the general case (e.g. UI will query the properties first before
>> allowing the user to set them)  While this does lead to us 
>> querying some
>> attributes unnecessarily in certain extremely unlikely corner cases, I
>> believe it is an acceptable compromise.
> 
> This was my intention as well but I did it in a different way, as I didn't realize there was an existing pattern for it.
> 
> My code does actually query all the properties up front but I serialized the driver queries in a different way. I.e., I used a separate CACHED flag for each property to keep track of whether the value is already valid. Each driver callback attempts to create the pending reply. The attempt to create the reply either succeeds or triggers a new driver query for another missing property value. This goes on until all the properties are cached and the pending reply can be sent.
> 
> I now see that the other services use a separate state machine to fetch the properties. That introduces more code than my solution but I guess I can do it that way if you feel it is easier to read.
> 

In the beginning we tried many approaches, including ones similar to the
path you took.  We found that the current approach used in the core,
while while by no means perfect in every situation, was the best
compromise.  It is quite scalable as the number of attributes / queries
grows and it blocks multiple (potentially malicious) applications from
DDoSing the modem when the information is not cached.

So while it might be a bit more code, I think it is the right approach
here as well; both for consistency and code readability reasons.

Regards,
-Denis