7:41 p.m.
Hi,
I have switched my dev environment to Ubuntu 11.04, and so I am using GCC 4.5.2.
Building oFono returns 2 erros due to uninit var. One is a false negative, so I have fix
it with the 'magic define', and will send the patch soon (VPN is dead so I cannot
send it).
But for the second one, I am not sure it's a false negative, so I would like to have
the thought of the mailing list, and even better the author or guys who has some knowledge
on this part of the code.
Here it's:
939<http://git.kernel.org/?p=network/ofono/ofono.git;a=blob;f=src/sim....
static void sim_get_image(struct ofono_sim *sim, unsigned char id,
940<http://git.kernel.org/?p=network/ofono/ofono.git;a=blob;f=src/sim....
gpointer user_data)
941<http://git.kernel.org/?p=network/ofono/ofono.git;a=blob;f=src/sim....
{
942<http://git.kernel.org/?p=network/ofono/ofono.git;a=blob;f=src/sim....
unsigned char *efimg;
943<http://git.kernel.org/?p=network/ofono/ofono.git;a=blob;f=src/sim....
char *image;
944<http://git.kernel.org/?p=network/ofono/ofono.git;a=blob;f=src/sim....
unsigned short iidf_id;
945<http://git.kernel.org/?p=network/ofono/ofono.git;a=blob;f=src/sim....
unsigned short iidf_offset;
946<http://git.kernel.org/?p=network/ofono/ofono.git;a=blob;f=src/sim....
unsigned short iidf_len;
947<http://git.kernel.org/?p=network/ofono/ofono.git;a=blob;f=src/sim....
948<http://git.kernel.org/?p=network/ofono/ofono.git;a=blob;f=src/sim....
image = sim_fs_get_cached_image(sim->simfs, id);
949<http://git.kernel.org/?p=network/ofono/ofono.git;a=blob;f=src/sim....
950<http://git.kernel.org/?p=network/ofono/ofono.git;a=blob;f=src/sim....
if (image != NULL) {
951<http://git.kernel.org/?p=network/ofono/ofono.git;a=blob;f=src/sim....
sim_get_image_cb(sim, id, image, FALSE);
952<http://git.kernel.org/?p=network/ofono/ofono.git;a=blob;f=src/sim....
goto watch;
953<http://git.kernel.org/?p=network/ofono/ofono.git;a=blob;f=src/sim....
}
954<http://git.kernel.org/?p=network/ofono/ofono.git;a=blob;f=src/sim....
955<http://git.kernel.org/?p=network/ofono/ofono.git;a=blob;f=src/sim....
if (sim->efimg_length <= (id * 9)) {
956<http://git.kernel.org/?p=network/ofono/ofono.git;a=blob;f=src/sim....
sim_get_image_cb(sim, id, NULL, FALSE);
957<http://git.kernel.org/?p=network/ofono/ofono.git;a=blob;f=src/sim....
return;
958<http://git.kernel.org/?p=network/ofono/ofono.git;a=blob;f=src/sim....
}
959<http://git.kernel.org/?p=network/ofono/ofono.git;a=blob;f=src/sim....
960<http://git.kernel.org/?p=network/ofono/ofono.git;a=blob;f=src/sim....
efimg = &sim->efimg[id * 9];
961<http://git.kernel.org/?p=network/ofono/ofono.git;a=blob;f=src/sim....
962<http://git.kernel.org/?p=network/ofono/ofono.git;a=blob;f=src/sim....
iidf_id = efimg[3] << 8 | efimg[4];
963<http://git.kernel.org/?p=network/ofono/ofono.git;a=blob;f=src/sim....
iidf_offset = efimg[5] << 8 | efimg[6];
964<http://git.kernel.org/?p=network/ofono/ofono.git;a=blob;f=src/sim....
iidf_len = efimg[7] << 8 | efimg[8];
965<http://git.kernel.org/?p=network/ofono/ofono.git;a=blob;f=src/sim....
966<http://git.kernel.org/?p=network/ofono/ofono.git;a=blob;f=src/sim....
/* read the image data */
967<http://git.kernel.org/?p=network/ofono/ofono.git;a=blob;f=src/sim....
ofono_sim_read_bytes(sim->context, iidf_id, iidf_offset, iidf_len,
968<http://git.kernel.org/?p=network/ofono/ofono.git;a=blob;f=src/sim....
sim_iidf_read_cb, sim);
969<http://git.kernel.org/?p=network/ofono/ofono.git;a=blob;f=src/sim....
970<http://git.kernel.org/?p=network/ofono/ofono.git;a=blob;f=src/sim....
watch:
971<http://git.kernel.org/?p=network/ofono/ofono.git;a=blob;f=src/sim....
if (sim->efimg_length <= id * 9)
972<http://git.kernel.org/?p=network/ofono/ofono.git;a=blob;f=src/sim....
return;
973<http://git.kernel.org/?p=network/ofono/ofono.git;a=blob;f=src/sim....
974<http://git.kernel.org/?p=network/ofono/ofono.git;a=blob;f=src/sim....
if (sim->iidf_watch_ids[id] > 0)
975<http://git.kernel.org/?p=network/ofono/ofono.git;a=blob;f=src/sim....
return;
976<http://git.kernel.org/?p=network/ofono/ofono.git;a=blob;f=src/sim....
977<http://git.kernel.org/?p=network/ofono/ofono.git;a=blob;f=src/sim....
sim->iidf_watch_ids[id] = ofono_sim_add_file_watch(sim->context,
978<http://git.kernel.org/?p=network/ofono/ofono.git;a=blob;f=src/sim....
iidf_id, sim_image_data_changed,
979<http://git.kernel.org/?p=network/ofono/ofono.git;a=blob;f=src/sim....
sim, NULL);
980<http://git.kernel.org/?p=network/ofono/ofono.git;a=blob;f=src/sim....
}
The unint var is 'unsigned short iidf_id;'
If the image is in the cache, we will go to the watch directly, and if there is no watch,
we will add one with an invalid iidf_id.
So is it possible to have an image in the cache without watch?
Thanks,
Bertrand
--
Bertrand Aygon Engineering Manager
Bertrand.Aygon@Intel.com<mailto:Bertrand.Aygon@intel.com> Open Source
Technology Center, Intel Corporation
---------------------------------------------------------------------
Intel Corporation SAS (French simplified joint stock company)
Registered headquarters: "Les Montalets"- 2, rue de Paris,
92196 Meudon Cedex, France
Registration Number: 302 456 199 R.C.S. NANTERRE
Capital: 4,572,000 Euros
This e-mail and any attachments may contain confidential material for
the sole use of the intended recipient(s). Any review or distribution
by others is strictly prohibited. If you are not the intended
recipient, please contact the sender and delete all copies.
12:25 p.m.
Hi Bertrand,
The unint var is ‘unsigned short iidf_id;’
If the image is in the cache, we will go to the watch directly, and if
there is no watch, we will add one with an invalid iidf_id.
This is a bug. We need to add a watch to the proper iidf_id, which we
need to obtain from the EFimg look up table. In the case of a cached
image, we're not doing that.
So is it possible to have an image in the cache without watch?
Yes, the cache is persistent (stored in IMSI-keyed filesystem location).
The logic is supposed to work somewhat like this:
1. Upon sim-atom init, read EFimg and populate its contents in a look up
table.
2a. When an application requests an icon with a particular id, look it
up in cache or read it from the SIM. Each icon is assigned to a
particular EFiidf_n file. So record application's interest in that file
by assigning a file watch.
2b. If an EFiidf file is refreshed via STK Refresh, call the file watch
and remove the file watch. The file watch will be re-added again once
the application requests the icon.
However, we have not seen any SIMs with SIM icons, so this part of the
code has not been stress tested...
Regards,
-Denis
12:09 a.m.
Hi Denis,
>
> The unint var is 'unsigned short iidf_id;'
>
> If the image is in the cache, we will go to the watch directly, and if
> there is no watch, we will add one with an invalid iidf_id.
>
This is a bug. We need to add a watch to the proper iidf_id, which we
need to obtain from the EFimg look up table. In the case of a cached
image, we're not doing that.
Thanks for the clarification. This was my first thought, but getting info from the writer
is always better ;)
Like last time I have found an issue, I guess that you have already make the fix... ;)
> So is it possible to have an image in the cache without watch?
Yes, the cache is persistent (stored in IMSI-keyed filesystem location).
The logic is supposed to work somewhat like this:
1. Upon sim-atom init, read EFimg and populate its contents in a look up
table.
2a. When an application requests an icon with a particular id, look it
up in cache or read it from the SIM. Each icon is assigned to a
particular EFiidf_n file. So record application's interest in that file
by assigning a file watch.
2b. If an EFiidf file is refreshed via STK Refresh, call the file watch
and remove the file watch. The file watch will be re-added again once
the application requests the icon.
However, we have not seen any SIMs with SIM icons, so this part of the
code has not been stress tested...
This cannot tested using PhoneSim?
Regards,
Bertrand
---------------------------------------------------------------------
Intel Corporation SAS (French simplified joint stock company)
Registered headquarters: "Les Montalets"- 2, rue de Paris,
92196 Meudon Cedex, France
Registration Number: 302 456 199 R.C.S. NANTERRE
Capital: 4,572,000 Euros
This e-mail and any attachments may contain confidential material for
the sole use of the intended recipient(s). Any review or distribution
by others is strictly prohibited. If you are not the intended
recipient, please contact the sender and delete all copies.
12:52 p.m.
Hi Bertrand,
> Yes, the cache is persistent (stored in IMSI-keyed filesystem
location).
> The logic is supposed to work somewhat like this:
>
> 1. Upon sim-atom init, read EFimg and populate its contents in a look up
> table.
>
> 2a. When an application requests an icon with a particular id, look it
> up in cache or read it from the SIM. Each icon is assigned to a
> particular EFiidf_n file. So record application's interest in that file
> by assigning a file watch.
>
> 2b. If an EFiidf file is refreshed via STK Refresh, call the file watch
> and remove the file watch. The file watch will be re-added again once
> the application requests the icon.
>
> However, we have not seen any SIMs with SIM icons, so this part of the
> code has not been stress tested...
This cannot tested using PhoneSim?
It can be, guess no one thought of it.
We have already simulated icons with phonesim. However sending a
refresh for EFiidf hasn't been added to phonesim yet.
Regards,
-Denis
4:59 a.m.
Hi Bertrand,
On 6 May 2011 11:41, Aygon, Bertrand <bertrand.aygon(a)intel.com> wrote:
Hi,
I have switched my dev environment to Ubuntu 11.04, and so I am using GCC
4.5.2.
Building oFono returns 2 erros due to uninit var. One is a false negative,
so I have fix it with the ‘magic define’, and will send the patch soon (VPN
is dead so I cannot send it).
But for the second one, I am not sure it’s a false negative, so I would like
to have the thought of the mailing list, and even better the author or guys
who has some knowledge on this part of the code.
That one is a bug. I remember it caught my eye at some point but
later I forgot about it. This is more or less how I'd try to fix it:
--- a/src/sim.c
+++ b/src/sim.c
@@ -947,10 +947,8 @@ static void sim_get_image(struct ofono_sim *sim,
unsigned char id,
image = sim_fs_get_cached_image(sim->simfs, id);
- if (image != NULL) {
+ if (image != NULL)
sim_get_image_cb(sim, id, image, FALSE);
- goto watch;
- }
if (sim->efimg_length <= (id * 9)) {
sim_get_image_cb(sim, id, NULL, FALSE);
@@ -963,13 +961,10 @@ static void sim_get_image(struct ofono_sim *sim,
unsigned char id,
iidf_offset = efimg[5] << 8 | efimg[6];
iidf_len = efimg[7] << 8 | efimg[8];
- /* read the image data */
- ofono_sim_read_bytes(sim->context, iidf_id, iidf_offset, iidf_len,
- sim_iidf_read_cb, sim);
-
-watch:
- if (sim->efimg_length <= id * 9)
- return;
+ if (image == NULL)
+ /* read the image data */
+ ofono_sim_read_bytes(sim->context, iidf_id, iidf_offset,
+ iidf_len, sim_iidf_read_cb, sim);
if (sim->iidf_watch_ids[id] > 0)
return;
Best regards
7:51 p.m.
Hi Andrzej,
That one is a bug. I remember it caught my eye at some point but
later I forgot about it. This is more or less how I'd try to fix it:
Thanks for handling this. Your fix looks OK regarding the bug and the uninitialized var,
but I do not have enough background on oFono and SIM file management to tell you if
everything will works. You should submit this patch and let Denis validate it.
Are you using PhoneSim to validate your fix? Or a modem?
Regards,
Bertrand
---------------------------------------------------------------------
Intel Corporation SAS (French simplified joint stock company)
Registered headquarters: "Les Montalets"- 2, rue de Paris,
92196 Meudon Cedex, France
Registration Number: 302 456 199 R.C.S. NANTERRE
Capital: 4,572,000 Euros
This e-mail and any attachments may contain confidential material for
the sole use of the intended recipient(s). Any review or distribution
by others is strictly prohibited. If you are not the intended
recipient, please contact the sender and delete all copies.
4150
days inactive
4153
days old
6 comments
3 participants
participants (3)
-
Andrzej Zaborowski -
Aygon, Bertrand -
Denis Kenzior