From: Denis Kenzior <denkenz@gmail.com>
To: ofono@ofono.org
Subject: Re: [PATCH 7/9] network: Add CPHS SPN fallback
Date: Fri, 02 Dec 2011 16:35:12 -0600 [thread overview]
Message-ID: <4ED952A0.6000903@gmail.com> (raw)
In-Reply-To: <1322824237-10039-8-git-send-email-oleg.zhurakivskyy@intel.com>
[-- Attachment #1: Type: text/plain, Size: 2648 bytes --]
Hi Oleg,
On 12/02/2011 05:10 AM, Oleg Zhurakivskyy wrote:
> ---
> src/network.c | 32 ++++++++++++++++++++++++--------
> 1 files changed, 24 insertions(+), 8 deletions(-)
>
> diff --git a/src/network.c b/src/network.c
> index 6e08876..1354edf 100644
> --- a/src/network.c
> +++ b/src/network.c
> @@ -1719,21 +1719,37 @@ static void sim_spn_read_cb(int ok, int length, int record,
> int record_length, void *user_data)
> {
> struct ofono_netreg *netreg = user_data;
> - unsigned char dcbyte;
> + const unsigned char *dcbyte = NULL;
> + int offset = 0;
>
> - if (!ok)
> + if (!ok) {
> + if (!(netreg->flags & NETWORK_REGISTRATION_FLAG_CPHS_SPN)) {
> +
> + netreg->flags |= NETWORK_REGISTRATION_FLAG_CPHS_SPN;
> +
> + ofono_sim_read(netreg->sim_context,
> + SIM_EF_CPHS_SPN_FILEID,
> + OFONO_SIM_FILE_STRUCTURE_TRANSPARENT,
> + sim_spn_read_cb, netreg);
> + }
> return;
> + }
>
> - dcbyte = data[0];
> + if (!(netreg->flags & NETWORK_REGISTRATION_FLAG_CPHS_SPN)) {
> + dcbyte = data;
> + offset = 1;
> + }
>
> - if (!sim_spn_parse(data + 1, length - 1, &netreg->spn))
> + if (!sim_spn_parse(data + offset, length - offset, &netreg->spn))
> return;
>
> - ofono_sim_read(netreg->sim_context, SIM_EFSPDI_FILEID,
> - OFONO_SIM_FILE_STRUCTURE_TRANSPARENT,
> - sim_spdi_read_cb, netreg);
> + if (!(netreg->flags & NETWORK_REGISTRATION_FLAG_CPHS_SPN))
> + ofono_sim_read(netreg->sim_context, SIM_EFSPDI_FILEID,
> + OFONO_SIM_FILE_STRUCTURE_TRANSPARENT,
> + sim_spdi_read_cb, netreg);
>
> - sim_spn_display_condition_parse(netreg, dcbyte);
> + if (dcbyte != NULL)
> + sim_spn_display_condition_parse(netreg, *dcbyte);
>
> if (netreg->current_operator)
> ofono_netreg_operator_display_name_notify(netreg);
Actually I don't like this approach. There are some interactions with
the SIM refresh logic triggered by sim_spn_changed and this approach
makes it very hard to puzzle out whether all cases are covered properly.
Also, the preference is always towards clearer, more readable code,
even if at the expense of being more verbose. Given how tiny
sim_spn_read_cb is now, I'd rather you handle the CPHS variants separately.
You are also not taking care of the CAT/SAT Refresh cases for the CPHS
variants.
Thinking more about it, the way I'd try to do this is to look up the
necessary bits in the CPHS Information Service Table (CPHS 4.2 Section
B.3.1.1). Then only read CPHS SPN if the service is active / allocated.
Only read the short form if CPHS SPN is not available and the short SPN is.
Regards,
-Denis
next prev parent reply other threads:[~2011-12-02 22:35 UTC|newest]
Thread overview: 18+ messages / expand[flat|nested] mbox.gz Atom feed top
2011-12-02 11:10 [PATCH 0/9] CPHS SPN, short-SPN support Oleg Zhurakivskyy
2011-12-02 11:10 ` [PATCH 1/9] network: M9 coding style corrections Oleg Zhurakivskyy
2011-12-02 22:03 ` Denis Kenzior
2011-12-02 11:10 ` [PATCH 2/9] network: Refactor sim_spn_read_cb() Oleg Zhurakivskyy
2011-12-02 22:03 ` Denis Kenzior
2011-12-02 11:10 ` [PATCH 3/9] network: Rename spname to spn Oleg Zhurakivskyy
2011-12-02 22:03 ` Denis Kenzior
2011-12-02 11:10 ` [PATCH 4/9] gprs-provision: Fix crash if no SPN present Oleg Zhurakivskyy
2011-12-02 22:04 ` Denis Kenzior
2011-12-02 11:10 ` [PATCH 5/9] simutil: Add CPHS SPN and short-SPN IDs Oleg Zhurakivskyy
2011-12-02 22:04 ` Denis Kenzior
2011-12-07 9:42 ` Oleg Zhurakivskyy
2011-12-02 11:10 ` [PATCH 6/9] network: Add CPHS SPN, short-SPN flags Oleg Zhurakivskyy
2011-12-02 11:10 ` [PATCH 7/9] network: Add CPHS SPN fallback Oleg Zhurakivskyy
2011-12-02 22:35 ` Denis Kenzior [this message]
2011-12-07 9:23 ` Oleg Zhurakivskyy
2011-12-02 11:10 ` [PATCH 8/9] network: Add CPHS short-SPN fallback Oleg Zhurakivskyy
2011-12-02 11:10 ` [PATCH 9/9] TODO: Remove completed CPHS SPN and short-SPN tasks Oleg Zhurakivskyy
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=4ED952A0.6000903@gmail.com \
--to=denkenz@gmail.com \
--cc=ofono@ofono.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.