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

Hi Denis,

On 27/04/2011 05:20, Denis Kenzior wrote:
> Hi Guillaume,
>
> On 04/22/2011 07:06 AM, Guillaume Zajac wrote:
>> ---
>>   gatchat/gatppp.c  |   43 ++++++++++++++++++++++++++++++++++++++++---
>>   gatchat/gatppp.h  |    2 ++
>>   gatchat/ppp.h     |    2 +-
>>   gatchat/ppp_net.c |   46 ++++++++++++++++++++++++++++------------------
>>   4 files changed, 71 insertions(+), 22 deletions(-)
>>
>> diff --git a/gatchat/gatppp.c b/gatchat/gatppp.c
>> index 993b5ea..d59f69b 100644
>> --- a/gatchat/gatppp.c
>> +++ b/gatchat/gatppp.c
>> @@ -76,6 +76,7 @@ struct _GAtPPP {
>>   	gpointer debug_data;
>>   	gboolean sta_pending;
>>   	guint ppp_dead_source;
>> +	int fd;
>>   };
>>
>>   void ppp_debug(GAtPPP *ppp, const char *str)
>> @@ -288,7 +289,7 @@ void ppp_auth_notify(GAtPPP *ppp, gboolean success)
>>   void ppp_ipcp_up_notify(GAtPPP *ppp, const char *local, const char *peer,
>>   					const char *dns1, const char *dns2)
>>   {
>> -	ppp->net = ppp_net_new(ppp);
>> +	ppp->net = ppp_net_new(ppp, ppp->fd);
>>
>>   	if (ppp->net == NULL) {
>>   		ppp->disconnect_reason = G_AT_PPP_REASON_NET_FAIL;
>> @@ -296,8 +297,14 @@ void ppp_ipcp_up_notify(GAtPPP *ppp, const char *local, const char *peer,
>>   		return;
>>   	}
>>
>> -	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 :)

>> +	if (ppp->fd<  0) {
>> +		if (ppp_net_set_mtu(ppp->net, ppp->mtu) == FALSE)
>> +			DBG(ppp, "Unable to set MTU");
>> +	}
>>
>>   	ppp_enter_phase(ppp, PPP_PHASE_LINK_UP);
>>
>> @@ -541,6 +548,9 @@ static GAtPPP *ppp_init_common(GAtHDLC *hdlc, gboolean is_server, guint32 ip)
>>
>>   	ppp->ref_count = 1;
>>
>> +	/* Default value of fd is -1 */
>> +	ppp->fd = -1;
>> +
>>   	/* set options to defaults */
>>   	ppp->mru = DEFAULT_MRU;
>>   	ppp->mtu = DEFAULT_MTU;
>> @@ -629,6 +639,33 @@ GAtPPP *g_at_ppp_server_new_from_io(GAtIO *io, const char *local)
>>   		return NULL;
>>
>>   	ppp = ppp_init_common(hdlc, TRUE, ip);
>> +
>> +	g_at_hdlc_unref(hdlc);
>> +
>> +	return ppp;
>> +}
>> +
>> +GAtPPP *g_at_ppp_server_new_from_io_with_fd(GAtIO *io,
>> +						const char *local, int fd)
>> +{
>> +	GAtHDLC *hdlc;
>> +	GAtPPP *ppp;
>> +	guint32 ip;
>> +
>> +	if (local == NULL)
>> +		ip = 0;
>> +	else if (inet_pton(AF_INET, local,&ip) != 1)
>> +		return NULL;
>> +
>> +	hdlc = g_at_hdlc_new_from_io(io);
>> +	if (hdlc == NULL)
>> +		return NULL;
>> +
>> +	ppp = ppp_init_common(hdlc, TRUE, ip);
>> +
>> +	/* Set the fd value returned by ConnMan */
>> +	ppp->fd = fd;
>> +
>>   	g_at_hdlc_unref(hdlc);
>>
>>   	return ppp;
>> diff --git a/gatchat/gatppp.h b/gatchat/gatppp.h
>> index fb5de4c..4ed4cde 100644
>> --- a/gatchat/gatppp.h
>> +++ b/gatchat/gatppp.h
>> @@ -54,6 +54,8 @@ GAtPPP *g_at_ppp_new(GIOChannel *modem);
>>   GAtPPP *g_at_ppp_new_from_io(GAtIO *io);
>>   GAtPPP *g_at_ppp_server_new(GIOChannel *modem, const char *local);
>>   GAtPPP *g_at_ppp_server_new_from_io(GAtIO *io, const char *local);
>> +GAtPPP *g_at_ppp_server_new_from_io_with_fd(GAtIO *io,
>> +						const char *local, int fd);
> Please name this g_at_ppp_server_new_full()

Ok

>>
>>   void g_at_ppp_open(GAtPPP *ppp);
>>   void g_at_ppp_set_connect_function(GAtPPP *ppp, GAtPPPConnectFunc callback,
>> diff --git a/gatchat/ppp.h b/gatchat/ppp.h
>> index d2786d7..8107820 100644
>> --- a/gatchat/ppp.h
>> +++ b/gatchat/ppp.h
>> @@ -102,7 +102,7 @@ void ppp_chap_free(struct ppp_chap *chap);
>>   void ppp_chap_process_packet(struct ppp_chap *chap, const guint8 *new_packet);
>>
>>   /* TUN / Network related functions */
>> -struct ppp_net *ppp_net_new(GAtPPP *ppp);
>> +struct ppp_net *ppp_net_new(GAtPPP *ppp, int fd);
>>   const char *ppp_net_get_interface(struct ppp_net *net);
>>   void ppp_net_process_packet(struct ppp_net *net, const guint8 *packet);
>>   void ppp_net_free(struct ppp_net *net);
>> diff --git a/gatchat/ppp_net.c b/gatchat/ppp_net.c
>> index 1a6cdf7..59c4b5e 100644
>> --- a/gatchat/ppp_net.c
>> +++ b/gatchat/ppp_net.c
>> @@ -123,12 +123,12 @@ const char *ppp_net_get_interface(struct ppp_net *net)
>>   	return net->if_name;
>>   }
>>
>> -struct ppp_net *ppp_net_new(GAtPPP *ppp)
>> +struct ppp_net *ppp_net_new(GAtPPP *ppp, int fd)
>>   {
>>   	struct ppp_net *net;
>>   	GIOChannel *channel = NULL;
>>   	struct ifreq ifr;
>> -	int fd, err;
>> +	int fdesc, err;
>>
>>   	net = g_try_new0(struct ppp_net, 1);
>>   	if (net == NULL)
>> @@ -140,23 +140,33 @@ struct ppp_net *ppp_net_new(GAtPPP *ppp)
>>   		return NULL;
>>   	}
>>
>> -	/* open a tun interface */
>> -	fd = open("/dev/net/tun", O_RDWR);
>> -	if (fd<  0)
>> -		goto error;
>> -
>> -	memset(&ifr, 0, sizeof(ifr));
>> -	ifr.ifr_flags = IFF_TUN | IFF_NO_PI;
>> -	strcpy(ifr.ifr_name, "ppp%d");
>> -
>> -	err = ioctl(fd, TUNSETIFF, (void *)&ifr);
>> -	if (err<  0)
>> -		goto error;
>> -
>> -	net->if_name = strdup(ifr.ifr_name);
>> +	/*
>> +	 * If the fd value is still the default one,
>> +	 * open the tun interface and configure it.
>> +	 */
>> +	if (fd<  0) {
> Missing space before '<'
>

Ok

>> +		/* open a tun interface */
>> +		fdesc = open("/dev/net/tun", O_RDWR);
>> +		if (fdesc<  0)
>> +			goto error;
>> +
>> +		memset(&ifr, 0, sizeof(ifr));
>> +		ifr.ifr_flags = IFF_TUN | IFF_NO_PI;
>> +		strcpy(ifr.ifr_name, "ppp%d");
>> +
>> +		err = ioctl(fdesc, TUNSETIFF, (void *)&ifr);
>> +		if (err<  0)
>> +			goto error;
>> +
>> +		net->if_name = strdup(ifr.ifr_name);
>> +		/* create a channel for reading and writing to this interface */
>> +		channel = g_io_channel_unix_new(fdesc);
>> +	} else {
>> +		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?

>> +		/* 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.

>>
>> -	/* create a channel for reading and writing to this interface */
>> -	channel = g_io_channel_unix_new(fd);
>>   	if (channel == NULL)
>>   		goto error;
>>
> Regards,
> -Denis
>

Kind regards,
Guillaume