Re: [PATCH v4 3/6] mbmmodem: add location-reporting driver implementation

[Denis Kenzior <denkenz@gmail.com>]

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

Hi Lucas,

On 02/23/2011 08:26 PM, Lucas De Marchi wrote:
> Hi Denis
> 
> On Wed, Feb 23, 2011 at 9:57 PM, Denis Kenzior <denkenz@gmail.com> wrote:
>> Hi Lucas,
>>
>>> +static int mbm_create_data_chat(struct ofono_location_reporting *lr)
>>> +{
>>> +     struct gps_data *gd = ofono_location_reporting_get_data(lr);
>>> +     struct ofono_modem *modem;
>>> +     const char *gps_dev;
>>> +     GAtSyntax *syntax;
>>> +     GIOChannel *channel;
>>> +     int fd;
>>> +
>>> +     modem = ofono_location_reporting_get_modem(lr);
>>> +     gps_dev = ofono_modem_get_string(modem, "GPSDevice");
>>> +
>>> +     channel = g_at_tty_open(gps_dev, NULL);
>>> +     if (channel == NULL)
>>> +             return -1;
>>> +
>>> +     syntax = g_at_syntax_new_gsm_permissive();
>>> +     gd->data_chat = g_at_chat_new(channel, syntax);
>>> +     fd = g_io_channel_unix_get_fd(channel);
>>> +
>>> +     g_at_syntax_unref(syntax);
>>> +     g_io_channel_unref(channel);
>>> +
>>> +     if (gd->data_chat == NULL)
>>> +             return -1;
>>> +
>>> +     return fd;
>>> +}
>>> +
>>
>> Why do you bother creating a GAtChat for the NMEA port?  It seems that a
>> GIOChannel or a simple fd would be enough.
> 
> This is because of this line:
> 
> +       if (g_at_chat_send(gd->data_chat, "AT*E2GPSNPD", NULL, NULL, NULL,
> +                                                               NULL) > 0) {
> 
> AT*E2GPSNPD is sent on the tty that will contain the nmea stream, not
> the control one.
> 

Then I'm confused how this even works.  The GAtChat is installing its
own IO watches on this channel and would be reading from the stream.
Wouldn't this cause you to have a race between the location reporting
client and ofono itself?

Maybe you should be sending the E2GPSNPD, unrefing the data_chat and
then sending the fd upwards?

>> Also, what happens if the channel is hupped by the modem?  Shouldn't we
>> have a watch in the core for that and set Enabled accordingly?
> 
> Humn... maybe. Though client should have called Release(). Otherwise,
> isn't the Release() method superfluous?
> 

Assuming the client is well behaved..

Also, what are we doing with the open socket if oFono is forcefully shut
down?  Shouldn't we shut it down as well?

Regards,
-Denis