Re: Unint var with GCC 4.5.2

Re: Unint var with GCC 4.5.2

[Denis Kenzior]

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

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

Re: Unint var with GCC 4.5.2

[Denis Kenzior]

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

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

Unint var with GCC 4.5.2

[Aygon, Bertrand]

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

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.c;h=af7a715a1609769cd2e1d85e865cb6e02823d7bb;hb=HEAD#l939> 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.c;h=af7a715a1609769cd2e1d85e865cb6e02823d7bb;hb=HEAD#l940>                                 gpointer user_data)
941<http://git.kernel.org/?p=network/ofono/ofono.git;a=blob;f=src/sim.c;h=af7a715a1609769cd2e1d85e865cb6e02823d7bb;hb=HEAD#l941> {
942<http://git.kernel.org/?p=network/ofono/ofono.git;a=blob;f=src/sim.c;h=af7a715a1609769cd2e1d85e865cb6e02823d7bb;hb=HEAD#l942>         unsigned char *efimg;
943<http://git.kernel.org/?p=network/ofono/ofono.git;a=blob;f=src/sim.c;h=af7a715a1609769cd2e1d85e865cb6e02823d7bb;hb=HEAD#l943>         char *image;
944<http://git.kernel.org/?p=network/ofono/ofono.git;a=blob;f=src/sim.c;h=af7a715a1609769cd2e1d85e865cb6e02823d7bb;hb=HEAD#l944>         unsigned short iidf_id;
945<http://git.kernel.org/?p=network/ofono/ofono.git;a=blob;f=src/sim.c;h=af7a715a1609769cd2e1d85e865cb6e02823d7bb;hb=HEAD#l945>         unsigned short iidf_offset;
946<http://git.kernel.org/?p=network/ofono/ofono.git;a=blob;f=src/sim.c;h=af7a715a1609769cd2e1d85e865cb6e02823d7bb;hb=HEAD#l946>         unsigned short iidf_len;
947<http://git.kernel.org/?p=network/ofono/ofono.git;a=blob;f=src/sim.c;h=af7a715a1609769cd2e1d85e865cb6e02823d7bb;hb=HEAD#l947>
948<http://git.kernel.org/?p=network/ofono/ofono.git;a=blob;f=src/sim.c;h=af7a715a1609769cd2e1d85e865cb6e02823d7bb;hb=HEAD#l948>         image = sim_fs_get_cached_image(sim->simfs, id);
949<http://git.kernel.org/?p=network/ofono/ofono.git;a=blob;f=src/sim.c;h=af7a715a1609769cd2e1d85e865cb6e02823d7bb;hb=HEAD#l949>
950<http://git.kernel.org/?p=network/ofono/ofono.git;a=blob;f=src/sim.c;h=af7a715a1609769cd2e1d85e865cb6e02823d7bb;hb=HEAD#l950>         if (image != NULL) {
951<http://git.kernel.org/?p=network/ofono/ofono.git;a=blob;f=src/sim.c;h=af7a715a1609769cd2e1d85e865cb6e02823d7bb;hb=HEAD#l951>                 sim_get_image_cb(sim, id, image, FALSE);
952<http://git.kernel.org/?p=network/ofono/ofono.git;a=blob;f=src/sim.c;h=af7a715a1609769cd2e1d85e865cb6e02823d7bb;hb=HEAD#l952>                 goto watch;
953<http://git.kernel.org/?p=network/ofono/ofono.git;a=blob;f=src/sim.c;h=af7a715a1609769cd2e1d85e865cb6e02823d7bb;hb=HEAD#l953>         }
954<http://git.kernel.org/?p=network/ofono/ofono.git;a=blob;f=src/sim.c;h=af7a715a1609769cd2e1d85e865cb6e02823d7bb;hb=HEAD#l954>
955<http://git.kernel.org/?p=network/ofono/ofono.git;a=blob;f=src/sim.c;h=af7a715a1609769cd2e1d85e865cb6e02823d7bb;hb=HEAD#l955>         if (sim->efimg_length <= (id * 9)) {
956<http://git.kernel.org/?p=network/ofono/ofono.git;a=blob;f=src/sim.c;h=af7a715a1609769cd2e1d85e865cb6e02823d7bb;hb=HEAD#l956>                 sim_get_image_cb(sim, id, NULL, FALSE);
957<http://git.kernel.org/?p=network/ofono/ofono.git;a=blob;f=src/sim.c;h=af7a715a1609769cd2e1d85e865cb6e02823d7bb;hb=HEAD#l957>                 return;
958<http://git.kernel.org/?p=network/ofono/ofono.git;a=blob;f=src/sim.c;h=af7a715a1609769cd2e1d85e865cb6e02823d7bb;hb=HEAD#l958>         }
959<http://git.kernel.org/?p=network/ofono/ofono.git;a=blob;f=src/sim.c;h=af7a715a1609769cd2e1d85e865cb6e02823d7bb;hb=HEAD#l959>
960<http://git.kernel.org/?p=network/ofono/ofono.git;a=blob;f=src/sim.c;h=af7a715a1609769cd2e1d85e865cb6e02823d7bb;hb=HEAD#l960>         efimg = &sim->efimg[id * 9];
961<http://git.kernel.org/?p=network/ofono/ofono.git;a=blob;f=src/sim.c;h=af7a715a1609769cd2e1d85e865cb6e02823d7bb;hb=HEAD#l961>
962<http://git.kernel.org/?p=network/ofono/ofono.git;a=blob;f=src/sim.c;h=af7a715a1609769cd2e1d85e865cb6e02823d7bb;hb=HEAD#l962>         iidf_id = efimg[3] << 8 | efimg[4];
963<http://git.kernel.org/?p=network/ofono/ofono.git;a=blob;f=src/sim.c;h=af7a715a1609769cd2e1d85e865cb6e02823d7bb;hb=HEAD#l963>         iidf_offset = efimg[5] << 8 | efimg[6];
964<http://git.kernel.org/?p=network/ofono/ofono.git;a=blob;f=src/sim.c;h=af7a715a1609769cd2e1d85e865cb6e02823d7bb;hb=HEAD#l964>         iidf_len = efimg[7] << 8 | efimg[8];
965<http://git.kernel.org/?p=network/ofono/ofono.git;a=blob;f=src/sim.c;h=af7a715a1609769cd2e1d85e865cb6e02823d7bb;hb=HEAD#l965>
966<http://git.kernel.org/?p=network/ofono/ofono.git;a=blob;f=src/sim.c;h=af7a715a1609769cd2e1d85e865cb6e02823d7bb;hb=HEAD#l966>         /* read the image data */
967<http://git.kernel.org/?p=network/ofono/ofono.git;a=blob;f=src/sim.c;h=af7a715a1609769cd2e1d85e865cb6e02823d7bb;hb=HEAD#l967>         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.c;h=af7a715a1609769cd2e1d85e865cb6e02823d7bb;hb=HEAD#l968>                                 sim_iidf_read_cb, sim);
969<http://git.kernel.org/?p=network/ofono/ofono.git;a=blob;f=src/sim.c;h=af7a715a1609769cd2e1d85e865cb6e02823d7bb;hb=HEAD#l969>
970<http://git.kernel.org/?p=network/ofono/ofono.git;a=blob;f=src/sim.c;h=af7a715a1609769cd2e1d85e865cb6e02823d7bb;hb=HEAD#l970> watch:
971<http://git.kernel.org/?p=network/ofono/ofono.git;a=blob;f=src/sim.c;h=af7a715a1609769cd2e1d85e865cb6e02823d7bb;hb=HEAD#l971>         if (sim->efimg_length <= id * 9)
972<http://git.kernel.org/?p=network/ofono/ofono.git;a=blob;f=src/sim.c;h=af7a715a1609769cd2e1d85e865cb6e02823d7bb;hb=HEAD#l972>                 return;
973<http://git.kernel.org/?p=network/ofono/ofono.git;a=blob;f=src/sim.c;h=af7a715a1609769cd2e1d85e865cb6e02823d7bb;hb=HEAD#l973>
974<http://git.kernel.org/?p=network/ofono/ofono.git;a=blob;f=src/sim.c;h=af7a715a1609769cd2e1d85e865cb6e02823d7bb;hb=HEAD#l974>         if (sim->iidf_watch_ids[id] > 0)
975<http://git.kernel.org/?p=network/ofono/ofono.git;a=blob;f=src/sim.c;h=af7a715a1609769cd2e1d85e865cb6e02823d7bb;hb=HEAD#l975>                 return;
976<http://git.kernel.org/?p=network/ofono/ofono.git;a=blob;f=src/sim.c;h=af7a715a1609769cd2e1d85e865cb6e02823d7bb;hb=HEAD#l976>
977<http://git.kernel.org/?p=network/ofono/ofono.git;a=blob;f=src/sim.c;h=af7a715a1609769cd2e1d85e865cb6e02823d7bb;hb=HEAD#l977>         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.c;h=af7a715a1609769cd2e1d85e865cb6e02823d7bb;hb=HEAD#l978>                                         iidf_id, sim_image_data_changed,
979<http://git.kernel.org/?p=network/ofono/ofono.git;a=blob;f=src/sim.c;h=af7a715a1609769cd2e1d85e865cb6e02823d7bb;hb=HEAD#l979>                                         sim, NULL);
980<http://git.kernel.org/?p=network/ofono/ofono.git;a=blob;f=src/sim.c;h=af7a715a1609769cd2e1d85e865cb6e02823d7bb;hb=HEAD#l980> }

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(a)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.

[-- Attachment #2: attachment.html --]
[-- Type: text/html, Size: 15824 bytes --]

RE: Unint var with GCC 4.5.2

[Aygon, Bertrand]

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

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.

Re: Unint var with GCC 4.5.2

[Andrzej Zaborowski]

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

Hi Bertrand,

On 6 May 2011 11:41, Aygon, Bertrand <bertrand.aygon@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

Re: Unint var with GCC 4.5.2

[Denis Kenzior]

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

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:

I pushed a fix b1f4e981f4935bff1b198a24cf110a6e838e42a9.  Please review
it and let me know if I screwed it up.

Regards,
-Denis

RE: Unint var with GCC 4.5.2

[Aygon, Bertrand]

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

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.