Re: [patch 07/20] SMS: implement SHA256-based message IDs [incomplete]

[Denis Kenzior <denkenz@gmail.com>]

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

On 07/29/2010 04:26 PM, Inaky Perez-Gonzalez wrote:
> On Tue, 2010-07-27 at 10:03 -0700, Denis Kenzior wrote: 
>> Hi Inaky,
>>
>>> -unsigned int __ofono_sms_txq_submit(struct ofono_sms *sms, GSList *list,
>>> -					unsigned int flags,
>>> +struct tx_queue_entry * __ofono_sms_txq_submit(struct ofono_sms *sms, GSList *list,
>>> +					unsigned int flags, unsigned ref,
>>>  					ofono_sms_txq_submit_cb_t cb,
>>>  					void *data, ofono_destroy_func destroy);
>>
>> I disagree with this, you're modifying an ofono internal API function to
>> return a structure which is opaque and cannot be manipulated.  I'd
>> rather see you returning a struct sms_uuid...
> 
> I need a handle to operate on. The D-Bus wrappers need something they
> can use to dispose of the structure (call tx_queue_entry_destroy() on
> the error path) and to pass to sms_msg_cancel(). The structure is not
> manipulated (except for accessing the UUID and #PDUs, which should
> probably be coded out to a helper).
> 
> This could be solved by having at the entry point of those functions or
> wrappers of a lookup function that looks for the UUID in the list of
> messages, but that would be adding more code and wasting time for no
> benefit. I disagree on adding unnecessary steps.
> 

I need to look at this in detail.  Can you simply grab the tx_entry from
the back of the queue for now?  I'm really against modifying txq_submit
right now.

>>> +	/*
>>> +	 * Instead of using the telephone number/address we got from
>>> +	 * D-Bus, we do the reverse formatting, so we get something
>>> +	 * that has been normalized--this is used later and we do it
>>> +	 * here to simplify error handling.
>>> +	 */
>>> +	sms_address_from_string(&receiver, to);
>>> +	if (sms_address_to_hex_string(&receiver, receiver_str)
>>> +	    == FALSE)
>>> +		return __ofono_error_failed(msg);
>>
>> Please avoid mixing tabs and spaces for indentation.  And in this case
>> it is more readale to break up the function than the equals line.  E.g.
>>
>> if (sms_address_to_hex_string(&receiver,
>> 				receiver_str == FALSE)
>> 	return __ofono_error_failed(msg);
> 
> This is truly a matter of personal opinion; I for one cannot see how
> this is more readable than the other form :)
> 

All coding styles are a matter of personal taste.  For oFono we prefer
the above mentioned style.

Regards,
-Denis