Re: [PATCH_v2 1/4] gatppp: Add new contructor to use external fd

[Guillaume Zajac <guillaume.zajac@linux.intel.com>]

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

Hi Denis,

On 28/04/2011 16:44, Denis Kenzior wrote:
> Hi Guillaume,
>
>>>> -    if (ppp_net_set_mtu(ppp->net, ppp->mtu) == FALSE)
>>>> -        DBG(ppp, "Unable to set MTU");
>>>> +    /*
>>>> +     * If we have opened the tun interface locally,
>>>> +     * we have to set a MTU value.
>>>> +     */
>>> So I don't agree with this.  The MTU is negotiated during LCP phase, so
>>> unless I'm missing something, this value should still be set to the
>>> negotiated one.
>>>
>> Right , I thought that as I was setting a MTU value for ConnMan tun
>> interface I didn't need to set the MTU value into oFono.
>> Obviously I was wrong :)
>>
> I assume the MTU is set to the default 1500 in Connman?

Exactly

>>>> +        net->if_name = strdup("Server ppp");
>>> This looks wrong.  You do actually want to return a proper network
>>> interface here, not make something up.
>> In fact, only ConnMan needs to know each name of its interfaces,
>> so do we need to fill this field in when ConnMan creates the interface?
>>
> Remember that we're writing a library, so things must be consistent.
> Besides, isn't this is a matter of running a TUNGETIFF ioctl?

Yes with TUNGETIFF I can get the name of the interface created by ConnMan

>>>> +        /* create a channel for reading and writing to this
>>>> interface */
>>>> +        channel = g_io_channel_unix_new(fd);
>>>> +    }
>>> There's a small problem of symmetry here.  If IPCP is established
>>> correctly, then the fd will eventually be closed by ppp_net.  However,
>>> if we never properly establish IPCP, then fd will not be closed.  What
>>> is actually expected by ConnMan?
>>>
>> In the case we don't properly establish the IPCP, ppp_disconnect CB of
>> emulator will be called.
>> Then, we will call the release_private_network DBus method. This method
>> will close the fd if it is opened.
>>
> Again, we're writing a library.  We cannot assume some behavior external
> to the library is going to save us.  So I really suggest we make closing
> of the fd symmetric.
So I will close the fd into g_at_ppp_unref() if it is >=0.
I have also to close it if we fail to create the PPP server.


Kind regards,
Guillaume