Re: [PATCHv6 3/3] Mobile broadband provider info plugin

[Oleg Zhurakivskyy <oleg.zhurakivskyy@intel.com>]

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

Hello Denis,

On 09/09/2011 07:58 AM, Denis Kenzior wrote:
> After reviewing this patch I got the impression that you were still
> making things just way too complicated and not taking full advantage of
> the push/pop semantics.  So just for fun I wrote my own version in
> plugins/mbpi.[ch] and reworked tools/lookup-apn to use that.  The code
> seems to be valgrind clean and can handle duplicate detection, bail out
> early, etc.
>
> Can you please take a look and suggest improvements?  Once you think it
> is good enough, I'd like you to rewrite this patch series to utilize
> mbpi.c parser.

Thanks, I have just looked. I am fine with this approach too.

One of the ideas of the implementation in PATCHv6 is to have the naming of 
tag_start() / tag_end() handlers to correspond to the actual tag names.

Can the same be achieved here? Can you please check PATCHv6 once again in order 
to check that idea?

A few cosmetic issues:

- the naming of struct gsm_data, perhaps, parser_data or provider_info would fit?
- gsm_start() could be broken down into smaller functions?
- Marcel wished the plugin naming "mobile-broadband-provider-info", even though 
it's a very long name. Shall we stick to that? I am fine with both.

Perhaps, some diagnostic output should be added too, in case anybody will need 
to troubleshoot it. For that, something like body_isprint() and ap_type_name() 
would be needed.

Are you fine with these?

Regards,
Oleg
-- 
Intel Finland Oy
Registered Address: PL 281, 00181 Helsinki
Business Identity Code: 0357606 - 4
Domiciled in Helsinki