From: Denis Kenzior <denkenz@gmail.com>
To: Sicelo <absicsz@gmail.com>
Cc: ofono@lists.linux.dev
Subject: Re: [PATCH] isimodem: parse extra details from REG_STATUS_IND
Date: Thu, 23 Nov 2023 09:33:37 -0600 [thread overview]
Message-ID: <20e913ef-c357-4a37-8936-9f5b98862755@gmail.com> (raw)
In-Reply-To: <ZV5AERD8NNEXvkKg@tp440p.steeds.sam>
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
prev parent reply other threads:[~2023-11-23 15:33 UTC|newest]
Thread overview: 4+ messages / expand[flat|nested] mbox.gz Atom feed top
2023-11-18 12:29 [PATCH] isimodem: parse extra details from REG_STATUS_IND Sicelo A. Mhlongo
2023-11-20 15:07 ` Denis Kenzior
2023-11-22 17:53 ` Sicelo
2023-11-23 15:33 ` Denis Kenzior [this message]
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=20e913ef-c357-4a37-8936-9f5b98862755@gmail.com \
--to=denkenz@gmail.com \
--cc=absicsz@gmail.com \
--cc=ofono@lists.linux.dev \
/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 a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).