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

[Denis Kenzior <denkenz@gmail.com>]

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

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.

> +	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()

>  
>  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 '<'

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

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

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

Regards,
-Denis