Re: [PATCH_v4 1/5] gatppp: Add new contructor to use external fd

[Denis Kenzior <denkenz@gmail.com>]

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

Hi Guillaume,

On 05/06/2011 09:01 AM, Guillaume Zajac wrote:
> ---
>  gatchat/gatppp.c  |   33 ++++++++++++++++++++++++++++++++-
>  gatchat/gatppp.h  |    1 +
>  gatchat/ppp.h     |    2 +-
>  gatchat/ppp_net.c |   40 +++++++++++++++++++++++++---------------
>  4 files changed, 59 insertions(+), 17 deletions(-)
> 
> diff --git a/gatchat/gatppp.c b/gatchat/gatppp.c
> index 993b5ea..c4cad90 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);

So please be careful here, you're passing in the file descriptor and you
do not establish a clear ownership of it.  Is ppp_net_new responsible to
close the fd in all cases?  Right now there are error cases (e.g.
ppp_net_new returns NULL) in which the fd is closed and others where it
is not.  You have to be extra paranoid when writing a library and make
sure every case has been thought through.

>  
>  	if (ppp->net == NULL) {
>  		ppp->disconnect_reason = G_AT_PPP_REASON_NET_FAIL;
> @@ -314,6 +315,7 @@ void ppp_ipcp_down_notify(GAtPPP *ppp)
>  		return;
>  
>  	ppp_net_free(ppp->net);
> +	ppp->fd = -1;
>  	ppp->net = NULL;
>  }
>  
> @@ -498,6 +500,8 @@ void g_at_ppp_unref(GAtPPP *ppp)
>  
>  	if (ppp->net)
>  		ppp_net_free(ppp->net);
> +	else if (ppp->fd >= 0)
> +		close(ppp->fd);
>  
>  	if (ppp->chap)
>  		ppp_chap_free(ppp->chap);
> @@ -541,6 +545,8 @@ static GAtPPP *ppp_init_common(GAtHDLC *hdlc, gboolean is_server, guint32 ip)
>  
>  	ppp->ref_count = 1;
>  
> +	ppp->fd = -1;
> +
>  	/* set options to defaults */
>  	ppp->mru = DEFAULT_MRU;
>  	ppp->mtu = DEFAULT_MTU;
> @@ -633,3 +639,28 @@ GAtPPP *g_at_ppp_server_new_from_io(GAtIO *io, const char *local)
>  
>  	return ppp;
>  }
> +
> +GAtPPP *g_at_ppp_server_new_full(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..825c022 100644
> --- a/gatchat/gatppp.h
> +++ b/gatchat/gatppp.h
> @@ -54,6 +54,7 @@ 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_full(GAtIO *io, const char *local, int fd);
>  
>  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..0f25299 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 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;
> +	/*
> +	 * If the fd value is still the default one,
> +	 * open the tun interface and configure it.
> +	 */
> +	if (fd < 0) {
> +		/* 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;
> +	} else {
> +		err = ioctl(fd, TUNGETIFF, (void *) &ifr);
> +		if (err < 0)
> +			goto error;
> +	}
>  
>  	net->if_name = strdup(ifr.ifr_name);
> -

Please avoid such changes.  Resist the urge to change formatting or
white-spacing when submitting feature patches.  Send a separate patch,
otherwise it confuses the reviewer.

Please note that in this particular case the original formatting is the
preferred style already.

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

Regards,
-Denis