| Message ID | 20231118122948.418378-1-absicsz@gmail.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | isimodem: parse extra details from REG_STATUS_IND | expand |
Hi Sicelo, On 11/18/23 06:29, Sicelo A. Mhlongo wrote: > Extract additional operator information available in REG_STATUS_IND messages. > These include mnc, mnc, and different variations of operator name. > > Additionally, in case NET_OPER_NAME_READ_RESP is an error result, which happens > with some operators, report the extracted information to core instead of > bailing out. > --- > drivers/isimodem/debug.c | 1 + > drivers/isimodem/network-registration.c | 73 ++++++++++++++++++++++++- > drivers/isimodem/network.h | 1 + > 3 files changed, 74 insertions(+), 1 deletion(-) > I do get a compiler warning here: drivers/isimodem/network-registration.c:537:3: error: misleading indentation; statement is not part of the previous 'if' [-Werror,-Wmisleading-indentation] strncpy(op.name, nd->nitz_name, OFONO_MAX_OPERATOR_NAME_LENGTH); ^ drivers/isimodem/network-registration.c:532:2: note: previous statement is here if (!check_response_status(msg, NET_OLD_OPER_NAME_READ_RESP) && ^ A few nits: > @@ -193,6 +195,12 @@ static void reg_status_ind_cb(const GIsiMessage *msg, void *data) > struct netreg_data *nd = ofono_netreg_get_data(netreg); > GIsiSubBlockIter iter; > > + uint8_t len = 0; > + uint8_t type = 0; > + char *abbrev = NULL; > + char *an = NULL; > + char *fn = NULL; > + You set these to NULL here, but ... > if (netreg == NULL || nd == NULL) > return; > > @@ -209,16 +217,69 @@ static void reg_status_ind_cb(const GIsiMessage *msg, void *data) > > if (!parse_common_info(&iter, &nd->reg)) > return; > + > + /* Parse abbreviated operator name */ > + if (!g_isi_sb_iter_get_byte(&iter, &len, 7)) > + return; > + > + len *= 2; > + > + if (!g_isi_sb_iter_get_alpha_tag(&iter, &abbrev, len, 8)) > + return; Is the intent really to ignore the whole message if this attribute can't be parsed? > break; > > case NET_GSM_REG_INFO: > > if (!parse_gsm_info(&iter, &nd->gsm)) > return; > + > + /* Parse mcc and mnc */ > + if (!g_isi_sb_iter_get_oper_code(&iter, nd->mcc, > + nd->mnc, 8)) > + return; > + break; > + > + case NET_REG_STATUS_IND_OPER_NAME: > + > + if (!g_isi_sb_iter_get_byte(&iter, &type, 2)) > + return; > + > + if (!g_isi_sb_iter_get_byte(&iter, &len, 5)) > + return; > + > + /* Name is UCS-2 encoded */ > + len *= 2; > + > + switch (type) { > + case 2: > + /* > + * Alternate full name, which is similar to > + * long name, but may have less whitespace > + */ > + g_isi_sb_iter_get_alpha_tag(&iter, &an, len, 6); > + break; > + case 3: > + /* Full name */ > + g_isi_sb_iter_get_alpha_tag(&iter, &fn, len, 6); > + break; But you don't check the error returns of these two calls > + } > break; > } > } > > + if (fn[0] != '\0') > + strncpy(nd->nitz_name, fn, OFONO_MAX_OPERATOR_NAME_LENGTH); > + else if (an[0] != '\0') > + strncpy(nd->nitz_name, an, OFONO_MAX_OPERATOR_NAME_LENGTH); So how do you know fn/an aren't NULL? > + else if (abbrev[0] != '\0') > + strncpy(nd->nitz_name, abbrev, OFONO_MAX_OPERATOR_NAME_LENGTH); Use g_strlcpy instead of strncpy since it takes care of setting the terminating NULL. > + > + nd->nitz_name[OFONO_MAX_OPERATOR_NAME_LENGTH] = '\0'; > + > + g_free(an); > + g_free(fn); > + g_free(abbrev); > + > ofono_netreg_status_notify(netreg, isi_status_to_at_status(&nd->reg), > nd->gsm.lac, nd->gsm.ci, > isi_to_at_tech(&nd->rat, &nd->gsm)); > @@ -458,6 +519,8 @@ static void name_get_resp_cb(const GIsiMessage *msg, void *data) > { > struct isi_cb_data *cbd = data; > ofono_netreg_operator_cb_t cb = cbd->cb; > + struct ofono_netreg *netreg = cbd->user; > + struct netreg_data *nd = ofono_netreg_get_data(netreg); > struct ofono_network_operator op; > > GIsiSubBlockIter iter; > @@ -468,7 +531,14 @@ static void name_get_resp_cb(const GIsiMessage *msg, void *data) > > if (!check_response_status(msg, NET_OLD_OPER_NAME_READ_RESP) && > !check_response_status(msg, NET_OPER_NAME_READ_RESP)) > - goto error; > + if (nd->nitz_name[0] == '\0') > + goto error; > + > + strncpy(op.name, nd->nitz_name, OFONO_MAX_OPERATOR_NAME_LENGTH); > + op.name[OFONO_MAX_OPERATOR_NAME_LENGTH] = '\0'; > + strncpy(op.mcc, nd->mcc, OFONO_MAX_MCC_LENGTH); > + strncpy(op.mnc, nd->mnc, OFONO_MAX_MNC_LENGTH); ditto here, g_strlcpy. > + goto success; > > for (g_isi_sb_iter_init(&iter, msg, 6); > g_isi_sb_iter_is_valid(&iter); > @@ -500,6 +570,7 @@ static void name_get_resp_cb(const GIsiMessage *msg, void *data) > } > } > > +success: > CALLBACK_WITH_SUCCESS(cb, &op, cbd->data); > return; > Regards, -Denis
Hello Denis On Mon, Nov 20, 2023 at 09:07:26AM -0600, Denis Kenzior wrote: > I do get a compiler warning here: > > drivers/isimodem/network-registration.c:537:3: error: misleading > indentation; statement is not part of the previous 'if' > [-Werror,-Wmisleading-indentation] > strncpy(op.name, nd->nitz_name, OFONO_MAX_OPERATOR_NAME_LENGTH); > ^ > drivers/isimodem/network-registration.c:532:2: note: previous statement is here > if (!check_response_status(msg, NET_OLD_OPER_NAME_READ_RESP) && > ^ Interestingly this one was not caught on my compilation runs. I will fix in v2. > > + g_isi_sb_iter_get_alpha_tag(&iter, &an, len, 6); > > + break; > > + case 3: > > + /* Full name */ > > + g_isi_sb_iter_get_alpha_tag(&iter, &fn, len, 6); > > + break; > > But you don't check the error returns of these two calls I left out the checking here because in both cases, we are breaking at the next statement, regardless of the success or failure of the calls. I will see if I can implement checking. > > + if (fn[0] != '\0') > > + strncpy(nd->nitz_name, fn, OFONO_MAX_OPERATOR_NAME_LENGTH); > > + else if (an[0] != '\0') > > + strncpy(nd->nitz_name, an, OFONO_MAX_OPERATOR_NAME_LENGTH); > > So how do you know fn/an aren't NULL? Please excuse my confusion here - I thought the `if` conditions do check whether an or fn are NULL. I will appreciate assistance or suggestion for an alternative way to ensure this, since I definitely do not want to copy over garbage into nd->nitz_name at this point in the code. > Use g_strlcpy instead of strncpy since it takes care of setting the > terminating NULL. I will do this in v2. While going through some older commits, I also found use of l_strlcpy. Should I prefer g_strlcpy in this instance? Thank you very much for the review, and looking forward to hearing from you soon. Sincerely Sicelo
Hi Sicelo, > >>> + g_isi_sb_iter_get_alpha_tag(&iter, &an, len, 6); >>> + break; >>> + case 3: >>> + /* Full name */ >>> + g_isi_sb_iter_get_alpha_tag(&iter, &fn, len, 6); >>> + break; >> >> But you don't check the error returns of these two calls > > I left out the checking here because in both cases, we are breaking at > the next statement, regardless of the success or failure of the calls. I > will see if I can implement checking. You break out of the outer switch, but you still don't know whether obtaining the alpha tag succeeded or not. So fn/an (or both) can still be NULL. > >>> + if (fn[0] != '\0') >>> + strncpy(nd->nitz_name, fn, OFONO_MAX_OPERATOR_NAME_LENGTH); >>> + else if (an[0] != '\0') >>> + strncpy(nd->nitz_name, an, OFONO_MAX_OPERATOR_NAME_LENGTH); >> >> So how do you know fn/an aren't NULL? > > Please excuse my confusion here - I thought the `if` conditions do check > whether an or fn are NULL. I will appreciate assistance or suggestion No, you're checking that fn/an are not equal to an empty string, not that fn/an are NULL. You want something like: if (fn && fn[0] != '\0') ... > for an alternative way to ensure this, since I definitely do not want to > copy over garbage into nd->nitz_name at this point in the code. > Same thing for abbrev. You're checking whether or not abbrev is an empty string "", but the whole TLV might be absent and g_isi_sb_iter_get_alpha_tag would never be executed. >> Use g_strlcpy instead of strncpy since it takes care of setting the >> terminating NULL. > > I will do this in v2. While going through some older commits, I also > found use of l_strlcpy. Should I prefer g_strlcpy in this instance? They're equivalent, you can use either. g_strlcpy is the GLib version. l_strlcpy is the ell version. oFono will slowly be migrating away from GLib to use ell. Since you're already using l_utf8_from_ucs2be inside gisi/*, you might as well use the ell version. Regards, -Denis
diff --git a/drivers/isimodem/debug.c b/drivers/isimodem/debug.c index 18055791..8edcd972 100644 --- a/drivers/isimodem/debug.c +++ b/drivers/isimodem/debug.c @@ -1190,6 +1190,7 @@ const char *net_subblock_name(enum net_subblock value) _(NET_REGISTRATION_CONF1_INFO); _(NET_ROAMING_CONF1_INFO); _(NET_AVAIL_NETWORK_INFO_COMMON); + _(NET_REG_STATUS_IND_OPER_NAME); _(NET_OPER_NAME_INFO); } return "NET_<UNKNOWN>"; diff --git a/drivers/isimodem/network-registration.c b/drivers/isimodem/network-registration.c index 8f70b3ee..bdfb8fd0 100644 --- a/drivers/isimodem/network-registration.c +++ b/drivers/isimodem/network-registration.c @@ -80,6 +80,8 @@ struct netreg_data { struct rat_info rat; GIsiVersion version; char nitz_name[OFONO_MAX_OPERATOR_NAME_LENGTH + 1]; + char mcc[OFONO_MAX_MCC_LENGTH + 1]; + char mnc[OFONO_MAX_MNC_LENGTH + 1]; }; static inline guint8 *mccmnc_to_bcd(const char *mcc, const char *mnc, @@ -193,6 +195,12 @@ static void reg_status_ind_cb(const GIsiMessage *msg, void *data) struct netreg_data *nd = ofono_netreg_get_data(netreg); GIsiSubBlockIter iter; + uint8_t len = 0; + uint8_t type = 0; + char *abbrev = NULL; + char *an = NULL; + char *fn = NULL; + if (netreg == NULL || nd == NULL) return; @@ -209,16 +217,69 @@ static void reg_status_ind_cb(const GIsiMessage *msg, void *data) if (!parse_common_info(&iter, &nd->reg)) return; + + /* Parse abbreviated operator name */ + if (!g_isi_sb_iter_get_byte(&iter, &len, 7)) + return; + + len *= 2; + + if (!g_isi_sb_iter_get_alpha_tag(&iter, &abbrev, len, 8)) + return; break; case NET_GSM_REG_INFO: if (!parse_gsm_info(&iter, &nd->gsm)) return; + + /* Parse mcc and mnc */ + if (!g_isi_sb_iter_get_oper_code(&iter, nd->mcc, + nd->mnc, 8)) + return; + break; + + case NET_REG_STATUS_IND_OPER_NAME: + + if (!g_isi_sb_iter_get_byte(&iter, &type, 2)) + return; + + if (!g_isi_sb_iter_get_byte(&iter, &len, 5)) + return; + + /* Name is UCS-2 encoded */ + len *= 2; + + switch (type) { + case 2: + /* + * Alternate full name, which is similar to + * long name, but may have less whitespace + */ + g_isi_sb_iter_get_alpha_tag(&iter, &an, len, 6); + break; + case 3: + /* Full name */ + g_isi_sb_iter_get_alpha_tag(&iter, &fn, len, 6); + break; + } break; } } + if (fn[0] != '\0') + strncpy(nd->nitz_name, fn, OFONO_MAX_OPERATOR_NAME_LENGTH); + else if (an[0] != '\0') + strncpy(nd->nitz_name, an, OFONO_MAX_OPERATOR_NAME_LENGTH); + else if (abbrev[0] != '\0') + strncpy(nd->nitz_name, abbrev, OFONO_MAX_OPERATOR_NAME_LENGTH); + + nd->nitz_name[OFONO_MAX_OPERATOR_NAME_LENGTH] = '\0'; + + g_free(an); + g_free(fn); + g_free(abbrev); + ofono_netreg_status_notify(netreg, isi_status_to_at_status(&nd->reg), nd->gsm.lac, nd->gsm.ci, isi_to_at_tech(&nd->rat, &nd->gsm)); @@ -458,6 +519,8 @@ static void name_get_resp_cb(const GIsiMessage *msg, void *data) { struct isi_cb_data *cbd = data; ofono_netreg_operator_cb_t cb = cbd->cb; + struct ofono_netreg *netreg = cbd->user; + struct netreg_data *nd = ofono_netreg_get_data(netreg); struct ofono_network_operator op; GIsiSubBlockIter iter; @@ -468,7 +531,14 @@ static void name_get_resp_cb(const GIsiMessage *msg, void *data) if (!check_response_status(msg, NET_OLD_OPER_NAME_READ_RESP) && !check_response_status(msg, NET_OPER_NAME_READ_RESP)) - goto error; + if (nd->nitz_name[0] == '\0') + goto error; + + strncpy(op.name, nd->nitz_name, OFONO_MAX_OPERATOR_NAME_LENGTH); + op.name[OFONO_MAX_OPERATOR_NAME_LENGTH] = '\0'; + strncpy(op.mcc, nd->mcc, OFONO_MAX_MCC_LENGTH); + strncpy(op.mnc, nd->mnc, OFONO_MAX_MNC_LENGTH); + goto success; for (g_isi_sb_iter_init(&iter, msg, 6); g_isi_sb_iter_is_valid(&iter); @@ -500,6 +570,7 @@ static void name_get_resp_cb(const GIsiMessage *msg, void *data) } } +success: CALLBACK_WITH_SUCCESS(cb, &op, cbd->data); return; diff --git a/drivers/isimodem/network.h b/drivers/isimodem/network.h index 7449a1d0..3282b383 100644 --- a/drivers/isimodem/network.h +++ b/drivers/isimodem/network.h @@ -90,6 +90,7 @@ enum net_subblock { NET_REGISTRATION_CONF1_INFO = 0x59, NET_ROAMING_CONF1_INFO = 0x5A, NET_AVAIL_NETWORK_INFO_COMMON = 0xE1, + NET_REG_STATUS_IND_OPER_NAME = 0xE3, NET_OPER_NAME_INFO = 0xE7, };