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: 2060 bytes --]

Hi Denis,

On 28/04/2011 21:41, Denis Kenzior wrote:
> Hi Guillaume,
>
>>>>>> +        /* 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.
>>
> Right.  Your approach doesn't sound completely correct, but I will let
> you figure this out.  Just make sure to close the fd in all circumstances.

In looking the previous implementation in ppp_net_new(),
there is just:

fd = open("/dev/net/tun", ...);
...
net->channel = g_io_channel_unix_new(fd);

However, the fd is never closed when we do ppp_net_free()
We just unref the net->channel

So should we do in ppp_net_free() a:
fd = g_io_channel_unix_get_fd(net->channel);
close(fd);
g_io_channel_unref(net->channel);

Or maybe I miss something?

In the new implementation, in the case we don't establish the IPCP, we 
still have the fd of ConnMan opened.
ppp->net == NULL so in the g_at_ppp_unref() we can do:

if (ppp->net)
         ppp_net_free(ppp->net); /* We close the fd  like above */
else {
         if( fd >= 0)
                 close(ppp->fd); /* We don't have established IPCP, we 
need to close the fd from ConnMan */
         }

Kind regards,
Guillaume