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
_______________________________________________
ofono mailing list
ofono@ofono.org
http://lists.ofono.org/listinfo/ofono
