[PATCH_v3 0/4] ACFC and PFC options implementation

[PATCH_v3 0/4] ACFC and PFC options implementation

[Guillaume Zajac]

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

Hi,

Change log from v2 is:
	- restore send_lcp_frame() function
	- global ACFC and PFC activation option is done into lcp_ppp.c
	  using lcp->req_options field
	- add ACFC and PFC options activation into emulator and gsmdial
	- by default ACFC and PFC options are turned off

Guillaume Zajac (4):
  GAtPPP: Add ACFC option support
  GAtPPP: Add PFC option support
  emulator: Activate ACFC and PFC options onto GAtPPP object
  gsmdial: Activate ACFC and PFC options on GAtPPP object

 gatchat/gatppp.c  |  155 +++++++++++++++++++++++++++++++++++++++++++++--------
 gatchat/gatppp.h  |    3 +
 gatchat/gsmdial.c |    3 +
 gatchat/ppp.h     |   12 ++++
 gatchat/ppp_lcp.c |   55 +++++++++++++++++--
 src/emulator.c    |    3 +
 6 files changed, 205 insertions(+), 26 deletions(-)

[PATCH_v3 1/4] GAtPPP: Add ACFC option support

[Guillaume Zajac]

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

---
 gatchat/gatppp.c  |   96 ++++++++++++++++++++++++++++++++++++++++-------------
 gatchat/gatppp.h  |    2 +
 gatchat/ppp.h     |    8 ++++
 gatchat/ppp_lcp.c |   31 +++++++++++++++--
 4 files changed, 110 insertions(+), 27 deletions(-)

diff --git a/gatchat/gatppp.c b/gatchat/gatppp.c
index 5fb4146..fe34ea1 100644
--- a/gatchat/gatppp.c
+++ b/gatchat/gatppp.c
@@ -83,6 +83,7 @@ struct _GAtPPP {
 	int fd;
 	guint guard_timeout_source;
 	gboolean suspended;
+	gboolean xmit_acfc;
 };
 
 void ppp_debug(GAtPPP *ppp, const char *str)
@@ -168,8 +169,19 @@ static inline gboolean ppp_drop_packet(GAtPPP *ppp, guint16 protocol)
 static void ppp_receive(const unsigned char *buf, gsize len, void *data)
 {
 	GAtPPP *ppp = data;
-	guint16 protocol = ppp_proto(buf);
-	const guint8 *packet = ppp_info(buf);
+	struct ppp_header *header = (struct ppp_header *) buf;
+	gboolean acfc_frame = (header->address != PPP_ADDR_FIELD
+			|| header->control != PPP_CTRL);
+	guint16 protocol;
+	const guint8 *packet;
+
+	if (acfc_frame) {
+		protocol = ppp_acfc_proto(buf);
+		packet = ppp_acfc_info(buf);
+	} else {
+		protocol = ppp_proto(buf);
+		packet = ppp_info(buf);
+	}
 
 	if (ppp_drop_packet(ppp, protocol))
 		return;
@@ -196,37 +208,29 @@ static void ppp_receive(const unsigned char *buf, gsize len, void *data)
 	};
 }
 
-/*
- * transmit out through the lower layer interface
- *
- * infolen - length of the information part of the packet
- */
-void ppp_transmit(GAtPPP *ppp, guint8 *packet, guint infolen)
+static void ppp_send_lcp_frame(GAtPPP *ppp, guint8 *packet, guint infolen)
 {
 	struct ppp_header *header = (struct ppp_header *) packet;
-	guint16 proto = ppp_proto(packet);
 	guint8 code;
-	gboolean lcp = (proto == LCP_PROTOCOL);
 	guint32 xmit_accm = 0;
 	gboolean sta = FALSE;
+	gboolean lcp;
 
 	/*
 	 * all LCP Link Configuration, Link Termination, and Code-Reject
 	 * packets must be sent with the default sending ACCM
 	 */
-	if (lcp) {
-		code = pppcp_get_code(packet);
-		lcp = code > 0 && code < 8;
-
-		/*
-		 * If we're going down, we try to make sure to send the final
-		 * ack before informing the upper layers via the ppp_disconnect
-		 * function.  Once we enter PPP_DEAD phase, no further packets
-		 * will be sent
-		 */
-		if (code == PPPCP_CODE_TYPE_TERMINATE_ACK)
-			sta = TRUE;
-	}
+	code = pppcp_get_code(packet);
+	lcp = code > 0 && code < 8;
+
+	/*
+	 * If we're going down, we try to make sure to send the final
+	 * ack before informing the upper layers via the ppp_disconnect
+	 * function.  Once we enter PPP_DEAD phase, no further packets
+	 * will be sent
+	 */
+	if (code == PPPCP_CODE_TYPE_TERMINATE_ACK)
+		sta = TRUE;
 
 	if (lcp) {
 		xmit_accm = g_at_hdlc_get_xmit_accm(ppp->hdlc);
@@ -251,6 +255,42 @@ void ppp_transmit(GAtPPP *ppp, guint8 *packet, guint infolen)
 		g_at_hdlc_set_xmit_accm(ppp->hdlc, xmit_accm);
 }
 
+static void ppp_send_acfc_frame(GAtPPP *ppp, guint8 *packet,
+					guint infolen)
+{
+	struct ppp_header *header = (struct ppp_header *) packet;
+	guint offset = 0;
+
+	if (ppp->xmit_acfc)
+		offset = 2;
+
+	if (g_at_hdlc_send(ppp->hdlc, packet + offset,
+				infolen + sizeof(*header) - offset)
+			== FALSE)
+		DBG(ppp, "Failed to send a frame\n");
+}
+
+/*
+ * transmit out through the lower layer interface
+ *
+ * infolen - length of the information part of the packet
+ */
+void ppp_transmit(GAtPPP *ppp, guint8 *packet, guint infolen)
+{
+	guint16 proto = ppp_proto(packet);
+
+	switch (proto) {
+	case LCP_PROTOCOL:
+		ppp_send_lcp_frame(ppp, packet, infolen);
+		break;
+	case CHAP_PROTOCOL:
+	case IPCP_PROTO:
+	case PPP_IP_PROTO:
+		ppp_send_acfc_frame(ppp, packet, infolen);
+		break;
+	}
+}
+
 static inline void ppp_enter_phase(GAtPPP *ppp, enum ppp_phase phase)
 {
 	DBG(ppp, "%d", phase);
@@ -390,6 +430,11 @@ void ppp_set_mtu(GAtPPP *ppp, const guint8 *data)
 	ppp->mtu = mtu;
 }
 
+void ppp_set_xmit_acfc(GAtPPP *ppp, gboolean acfc)
+{
+	ppp->xmit_acfc = acfc;
+}
+
 static void io_disconnect(gpointer user_data)
 {
 	GAtPPP *ppp = user_data;
@@ -658,6 +703,11 @@ void g_at_ppp_set_server_info(GAtPPP *ppp, const char *remote,
 	ipcp_set_server_info(ppp->ipcp, r, d1, d2);
 }
 
+void g_at_ppp_set_acfc_enabled(GAtPPP *ppp)
+{
+	lcp_turn_on_acfc(ppp->lcp);
+}
+
 static GAtPPP *ppp_init_common(gboolean is_server, guint32 ip)
 {
 	GAtPPP *ppp;
diff --git a/gatchat/gatppp.h b/gatchat/gatppp.h
index f0930a7..428f08f 100644
--- a/gatchat/gatppp.h
+++ b/gatchat/gatppp.h
@@ -79,6 +79,8 @@ void g_at_ppp_set_recording(GAtPPP *ppp, const char *filename);
 void g_at_ppp_set_server_info(GAtPPP *ppp, const char *remote_ip,
 				const char *dns1, const char *dns2);
 
+void g_at_ppp_set_acfc_enabled(GAtPPP *ppp);
+
 #ifdef __cplusplus
 }
 #endif
diff --git a/gatchat/ppp.h b/gatchat/ppp.h
index 023d779..9d118b0 100644
--- a/gatchat/ppp.h
+++ b/gatchat/ppp.h
@@ -85,10 +85,17 @@ static inline void __put_unaligned_short(void *p, guint16 val)
 #define ppp_proto(packet) \
 	(get_host_short(packet + 2))
 
+#define ppp_acfc_info(packet) \
+	(packet + 2)
+
+#define ppp_acfc_proto(packet) \
+	(get_host_short(packet))
+
 /* LCP related functions */
 struct pppcp_data *lcp_new(GAtPPP *ppp, gboolean dormant);
 void lcp_free(struct pppcp_data *lcp);
 void lcp_protocol_reject(struct pppcp_data *lcp, guint8 *packet, gsize len);
+void lcp_turn_on_acfc(struct pppcp_data *pppcp);
 
 /* IPCP related functions */
 struct pppcp_data *ipcp_new(GAtPPP *ppp, gboolean is_server, guint32 ip);
@@ -125,4 +132,5 @@ void ppp_lcp_finished_notify(GAtPPP *ppp);
 void ppp_set_recv_accm(GAtPPP *ppp, guint32 accm);
 void ppp_set_xmit_accm(GAtPPP *ppp, guint32 accm);
 void ppp_set_mtu(GAtPPP *ppp, const guint8 *data);
+void ppp_set_xmit_acfc(GAtPPP *ppp, gboolean acfc);
 struct ppp_header *ppp_packet_new(gsize infolen, guint16 protocol);
diff --git a/gatchat/ppp_lcp.c b/gatchat/ppp_lcp.c
index ce9dae2..9dfd534 100644
--- a/gatchat/ppp_lcp.c
+++ b/gatchat/ppp_lcp.c
@@ -58,11 +58,12 @@ enum lcp_options {
 	ACFC			= 8,
 };
 
-/* Maximum size of all options, we only ever request ACCM and MRU */ 
-#define MAX_CONFIG_OPTION_SIZE 10
+/* Maximum size of all options, we only ever request ACCM, MRU and ACFC */
+#define MAX_CONFIG_OPTION_SIZE 12
 
 #define REQ_OPTION_ACCM	0x1
 #define REQ_OPTION_MRU	0x2
+#define REQ_OPTION_ACFC	0x4
 
 struct lcp_data {
 	guint8 options[MAX_CONFIG_OPTION_SIZE];
@@ -100,13 +101,19 @@ static void lcp_generate_config_options(struct lcp_data *lcp)
 		len += 4;
 	}
 
+	if (lcp->req_options & REQ_OPTION_ACFC) {
+		lcp->options[len] = ACFC;
+		lcp->options[len + 1] = 2;
+
+		len += 2;
+	}
+
 	lcp->options_len = len;
 }
 
 static void lcp_reset_config_options(struct lcp_data *lcp)
 {
 	/* Using the default ACCM */
-
 	lcp_generate_config_options(lcp);
 }
 
@@ -286,9 +293,15 @@ static enum rcr_result lcp_rcr(struct pppcp_data *pppcp,
 			break;
 		case MAGIC_NUMBER:
 		case PFC:
-		case ACFC:
 			/* don't care */
 			break;
+		case ACFC:
+		{
+			struct lcp_data *lcp = pppcp_get_data(pppcp);
+			if (lcp->req_options & REQ_OPTION_ACFC)
+				ppp_set_xmit_acfc(ppp, TRUE);
+			break;
+		}
 		}
 	}
 
@@ -338,3 +351,13 @@ struct pppcp_data *lcp_new(GAtPPP *ppp, gboolean is_server)
 
 	return pppcp;
 }
+
+void lcp_turn_on_acfc(struct pppcp_data *pppcp)
+{
+	struct lcp_data *lcp = pppcp_get_data(pppcp);
+
+	lcp->req_options |= REQ_OPTION_ACFC;
+	lcp_generate_config_options(lcp);
+
+	pppcp_set_local_options(pppcp, lcp->options, lcp->options_len);
+}
-- 
1.7.1

[PATCH_v3 2/4] GAtPPP: Add PFC option support

[Guillaume Zajac]

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

---
 gatchat/gatppp.c  |   63 ++++++++++++++++++++++++++++++++++++++++++++++++++++-
 gatchat/gatppp.h  |    1 +
 gatchat/ppp.h     |    4 +++
 gatchat/ppp_lcp.c |   30 ++++++++++++++++++++++--
 4 files changed, 94 insertions(+), 4 deletions(-)

diff --git a/gatchat/gatppp.c b/gatchat/gatppp.c
index fe34ea1..01446d1 100644
--- a/gatchat/gatppp.c
+++ b/gatchat/gatppp.c
@@ -84,6 +84,7 @@ struct _GAtPPP {
 	guint guard_timeout_source;
 	gboolean suspended;
 	gboolean xmit_acfc;
+	gboolean xmit_pfc;
 };
 
 void ppp_debug(GAtPPP *ppp, const char *str)
@@ -172,6 +173,7 @@ static void ppp_receive(const unsigned char *buf, gsize len, void *data)
 	struct ppp_header *header = (struct ppp_header *) buf;
 	gboolean acfc_frame = (header->address != PPP_ADDR_FIELD
 			|| header->control != PPP_CTRL);
+	gboolean pfc_frame = FALSE;
 	guint16 protocol;
 	const guint8 *packet;
 
@@ -183,6 +185,20 @@ static void ppp_receive(const unsigned char *buf, gsize len, void *data)
 		packet = ppp_info(buf);
 	}
 
+	pfc_frame = (protocol != LCP_PROTOCOL && protocol != CHAP_PROTOCOL &&
+			protocol != IPCP_PROTO && protocol != PPP_IP_PROTO);
+
+	if (pfc_frame) {
+		guint8 proto = (protocol >> 8) & 0xFF ;
+		packet = packet - 1;
+		/*
+		 * The only protocol that can be compressed is PPP_IP_PROTO
+		 * because first byte is 0x00.
+		 */
+		if (proto == PPP_IP_COMPRESSED_PROTO)
+			protocol = PPP_IP_PROTO;
+	}
+
 	if (ppp_drop_packet(ppp, protocol))
 		return;
 
@@ -264,6 +280,32 @@ static void ppp_send_acfc_frame(GAtPPP *ppp, guint8 *packet,
 	if (ppp->xmit_acfc)
 		offset = 2;
 
+	/* We remove the only address and control field */
+	if (g_at_hdlc_send(ppp->hdlc, packet + offset,
+				infolen + sizeof(*header) - offset)
+			== FALSE)
+		DBG(ppp, "Failed to send a frame\n");
+}
+
+static void ppp_send_acfc_pfc_frame(GAtPPP *ppp, guint8 *packet,
+					guint infolen)
+{
+	struct ppp_header *header = (struct ppp_header *) packet;
+	guint offset = 0;
+
+	if (ppp->xmit_acfc && ppp->xmit_pfc)
+		offset = 3;
+	else if (ppp->xmit_acfc)
+		offset = 2;
+	else if (ppp->xmit_pfc) {
+		/*
+		 * We remove only the 1st byte that is 0x00 of protocol field.
+		 */
+		packet[2] = packet[1];
+		packet[1] = packet[0];
+		offset = 1;
+	}
+
 	if (g_at_hdlc_send(ppp->hdlc, packet + offset,
 				infolen + sizeof(*header) - offset)
 			== FALSE)
@@ -285,9 +327,18 @@ void ppp_transmit(GAtPPP *ppp, guint8 *packet, guint infolen)
 		break;
 	case CHAP_PROTOCOL:
 	case IPCP_PROTO:
-	case PPP_IP_PROTO:
+		/*
+		 * We can't use PFC option because first byte of CHAP_PROTOCOL
+		 * and IPCP_PROTO is not equal to 0x00
+		 */
 		ppp_send_acfc_frame(ppp, packet, infolen);
 		break;
+	case PPP_IP_PROTO:
+		/*
+		 * We can't use both compression options if they are negotiated
+		 */
+		ppp_send_acfc_pfc_frame(ppp, packet, infolen);
+		break;
 	}
 }
 
@@ -435,6 +486,11 @@ void ppp_set_xmit_acfc(GAtPPP *ppp, gboolean acfc)
 	ppp->xmit_acfc = acfc;
 }
 
+void ppp_set_xmit_pfc(GAtPPP *ppp, gboolean pfc)
+{
+	ppp->xmit_pfc = pfc;
+}
+
 static void io_disconnect(gpointer user_data)
 {
 	GAtPPP *ppp = user_data;
@@ -708,6 +764,11 @@ void g_at_ppp_set_acfc_enabled(GAtPPP *ppp)
 	lcp_turn_on_acfc(ppp->lcp);
 }
 
+void g_at_ppp_set_pfc_enabled(GAtPPP *ppp)
+{
+	lcp_turn_on_pfc(ppp->lcp);
+}
+
 static GAtPPP *ppp_init_common(gboolean is_server, guint32 ip)
 {
 	GAtPPP *ppp;
diff --git a/gatchat/gatppp.h b/gatchat/gatppp.h
index 428f08f..57185b5 100644
--- a/gatchat/gatppp.h
+++ b/gatchat/gatppp.h
@@ -80,6 +80,7 @@ void g_at_ppp_set_server_info(GAtPPP *ppp, const char *remote_ip,
 				const char *dns1, const char *dns2);
 
 void g_at_ppp_set_acfc_enabled(GAtPPP *ppp);
+void g_at_ppp_set_pfc_enabled(GAtPPP *ppp);
 
 #ifdef __cplusplus
 }
diff --git a/gatchat/ppp.h b/gatchat/ppp.h
index 9d118b0..8ab031b 100644
--- a/gatchat/ppp.h
+++ b/gatchat/ppp.h
@@ -27,6 +27,8 @@
 #define PPP_IP_PROTO	0x0021
 #define MD5		5
 
+#define PPP_IP_COMPRESSED_PROTO 0x21
+
 #define DBG(p, fmt, arg...) do {				\
 	char *str = g_strdup_printf("%s:%s() " fmt, __FILE__,	\
 					__FUNCTION__ , ## arg); \
@@ -96,6 +98,7 @@ struct pppcp_data *lcp_new(GAtPPP *ppp, gboolean dormant);
 void lcp_free(struct pppcp_data *lcp);
 void lcp_protocol_reject(struct pppcp_data *lcp, guint8 *packet, gsize len);
 void lcp_turn_on_acfc(struct pppcp_data *pppcp);
+void lcp_turn_on_pfc(struct pppcp_data *pppcp);
 
 /* IPCP related functions */
 struct pppcp_data *ipcp_new(GAtPPP *ppp, gboolean is_server, guint32 ip);
@@ -133,4 +136,5 @@ void ppp_set_recv_accm(GAtPPP *ppp, guint32 accm);
 void ppp_set_xmit_accm(GAtPPP *ppp, guint32 accm);
 void ppp_set_mtu(GAtPPP *ppp, const guint8 *data);
 void ppp_set_xmit_acfc(GAtPPP *ppp, gboolean acfc);
+void ppp_set_xmit_pfc(GAtPPP *ppp, gboolean pfc);
 struct ppp_header *ppp_packet_new(gsize infolen, guint16 protocol);
diff --git a/gatchat/ppp_lcp.c b/gatchat/ppp_lcp.c
index 9dfd534..40375d9 100644
--- a/gatchat/ppp_lcp.c
+++ b/gatchat/ppp_lcp.c
@@ -58,12 +58,13 @@ enum lcp_options {
 	ACFC			= 8,
 };
 
-/* Maximum size of all options, we only ever request ACCM, MRU and ACFC */
-#define MAX_CONFIG_OPTION_SIZE 12
+/* Maximum size of all options, we only ever request ACCM, MRU, ACFC and PFC */
+#define MAX_CONFIG_OPTION_SIZE 14
 
 #define REQ_OPTION_ACCM	0x1
 #define REQ_OPTION_MRU	0x2
 #define REQ_OPTION_ACFC	0x4
+#define REQ_OPTION_PFC	0x8
 
 struct lcp_data {
 	guint8 options[MAX_CONFIG_OPTION_SIZE];
@@ -108,6 +109,13 @@ static void lcp_generate_config_options(struct lcp_data *lcp)
 		len += 2;
 	}
 
+	if (lcp->req_options & REQ_OPTION_PFC) {
+		lcp->options[len] = PFC;
+		lcp->options[len + 1] = 2;
+
+		len += 2;
+	}
+
 	lcp->options_len = len;
 }
 
@@ -292,9 +300,15 @@ static enum rcr_result lcp_rcr(struct pppcp_data *pppcp,
 			ppp_set_mtu(ppp, ppp_option_iter_get_data(&iter));
 			break;
 		case MAGIC_NUMBER:
-		case PFC:
 			/* don't care */
 			break;
+		case PFC:
+		{
+			struct lcp_data *lcp = pppcp_get_data(pppcp);
+			if (lcp->req_options & REQ_OPTION_PFC)
+				ppp_set_xmit_pfc(ppp, TRUE);
+			break;
+		}
 		case ACFC:
 		{
 			struct lcp_data *lcp = pppcp_get_data(pppcp);
@@ -361,3 +375,13 @@ void lcp_turn_on_acfc(struct pppcp_data *pppcp)
 
 	pppcp_set_local_options(pppcp, lcp->options, lcp->options_len);
 }
+
+void lcp_turn_on_pfc(struct pppcp_data *pppcp)
+{
+	struct lcp_data *lcp = pppcp_get_data(pppcp);
+
+	lcp->req_options |= REQ_OPTION_PFC;
+	lcp_generate_config_options(lcp);
+
+	pppcp_set_local_options(pppcp, lcp->options, lcp->options_len);
+}
-- 
1.7.1

[PATCH_v3 3/4] emulator: Activate ACFC and PFC options onto GAtPPP object

[Guillaume Zajac]

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

---
 src/emulator.c |    3 +++
 1 files changed, 3 insertions(+), 0 deletions(-)

diff --git a/src/emulator.c b/src/emulator.c
index 906404d..c432b54 100644
--- a/src/emulator.c
+++ b/src/emulator.c
@@ -150,6 +150,9 @@ static void request_private_network_cb(
 	g_at_ppp_set_server_info(em->ppp, pns->peer_ip,
 					pns->primary_dns, pns->secondary_dns);
 
+	g_at_ppp_set_acfc_enabled(em->ppp);
+	g_at_ppp_set_pfc_enabled(em->ppp);
+
 	g_at_ppp_set_credentials(em->ppp, "", "");
 	g_at_ppp_set_debug(em->ppp, emulator_debug, "PPP");
 
-- 
1.7.1

[PATCH_v3 4/4] gsmdial: Activate ACFC and PFC options on GAtPPP object

[Guillaume Zajac]

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

---
 gatchat/gsmdial.c |    3 +++
 1 files changed, 3 insertions(+), 0 deletions(-)

diff --git a/gatchat/gsmdial.c b/gatchat/gsmdial.c
index 1116984..21df2f9 100644
--- a/gatchat/gsmdial.c
+++ b/gatchat/gsmdial.c
@@ -372,6 +372,9 @@ static void connect_cb(gboolean ok, GAtResult *result, gpointer user_data)
 
 	g_at_ppp_set_credentials(ppp, option_username, option_password);
 
+	g_at_ppp_set_acfc_enabled(ppp);
+	g_at_ppp_set_pfc_enabled(ppp);
+
 	/* set connect and disconnect callbacks */
 	g_at_ppp_set_connect_function(ppp, ppp_connect, NULL);
 	g_at_ppp_set_disconnect_function(ppp, ppp_disconnect, NULL);
-- 
1.7.1

Re: [PATCH_v3 1/4] GAtPPP: Add ACFC option support

[Denis Kenzior]

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

Hi Guillaume,

> @@ -658,6 +703,11 @@ void g_at_ppp_set_server_info(GAtPPP *ppp, const char *remote,
>  	ipcp_set_server_info(ppp->ipcp, r, d1, d2);
>  }
>  
> +void g_at_ppp_set_acfc_enabled(GAtPPP *ppp)

I really do want the signature to be g_at_ppp_set_acfc_enabled(GAtPPP
*ppp, gboolean enabled)

There are cases where we might re-use the PPP object with different
parameters.

> +{
> +	lcp_turn_on_acfc(ppp->lcp);
> +}
> +
>  static GAtPPP *ppp_init_common(gboolean is_server, guint32 ip)
>  {
>  	GAtPPP *ppp;

<snip>

> @@ -100,13 +101,19 @@ static void lcp_generate_config_options(struct lcp_data *lcp)
>  		len += 4;
>  	}
>  
> +	if (lcp->req_options & REQ_OPTION_ACFC) {
> +		lcp->options[len] = ACFC;
> +		lcp->options[len + 1] = 2;
> +
> +		len += 2;
> +	}
> +
>  	lcp->options_len = len;
>  }
>  
>  static void lcp_reset_config_options(struct lcp_data *lcp)
>  {
>  	/* Using the default ACCM */
> -

Please don't include random whitespace fixes as part of a larger patch,
send these separately.  And in this case the empty line is there on
purpose, since the comment does not refer to
lcp_generate_config_options, but to the fact that options by default are
empty.

>  	lcp_generate_config_options(lcp);
>  }
>  
> @@ -286,9 +293,15 @@ static enum rcr_result lcp_rcr(struct pppcp_data *pppcp,
>  			break;
>  		case MAGIC_NUMBER:
>  		case PFC:
> -		case ACFC:
>  			/* don't care */
>  			break;
> +		case ACFC:
> +		{
> +			struct lcp_data *lcp = pppcp_get_data(pppcp);

Please add a newline here

> +			if (lcp->req_options & REQ_OPTION_ACFC)
> +				ppp_set_xmit_acfc(ppp, TRUE);

And a newline here

> +			break;
> +		}
>  		}
>  	}
>  

Regards,
-Denis

Re: [PATCH_v3 1/4] GAtPPP: Add ACFC option support

[Denis Kenzior]

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

Hi Marcel,

On 06/28/2011 08:24 PM, Marcel Holtmann wrote:
> Hi Denis,
> 
>>> @@ -658,6 +703,11 @@ void g_at_ppp_set_server_info(GAtPPP *ppp, const char *remote,
>>>  	ipcp_set_server_info(ppp->ipcp, r, d1, d2);
>>>  }
>>>  
>>> +void g_at_ppp_set_acfc_enabled(GAtPPP *ppp)
>>
>> I really do want the signature to be g_at_ppp_set_acfc_enabled(GAtPPP
>> *ppp, gboolean enabled)
>>
>> There are cases where we might re-use the PPP object with different
>> parameters.
> 
> what is wrong with just g_at_ppp_set_acfc(GAtPPP *ppp, gboolean enabled)
> as function name. Duplicating enabled in the function name and as
> parameter seems to be bit odd.

For APIs I generally prefer:

_set_foo() when foo is a 'thing', e.g int/string/struct/etc

and _set_foo_enabled() when foo is on/off as that intent is clearer when
reading the code.

Regards,
-Denis

Re: [PATCH_v3 1/4] GAtPPP: Add ACFC option support

[Denis Kenzior]

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

Hi Marcel,

On 06/28/2011 08:38 PM, Marcel Holtmann wrote:
> Hi Denis,
> 
>>>>> @@ -658,6 +703,11 @@ void g_at_ppp_set_server_info(GAtPPP *ppp, const char *remote,
>>>>>  	ipcp_set_server_info(ppp->ipcp, r, d1, d2);
>>>>>  }
>>>>>  
>>>>> +void g_at_ppp_set_acfc_enabled(GAtPPP *ppp)
>>>>
>>>> I really do want the signature to be g_at_ppp_set_acfc_enabled(GAtPPP
>>>> *ppp, gboolean enabled)
>>>>
>>>> There are cases where we might re-use the PPP object with different
>>>> parameters.
>>>
>>> what is wrong with just g_at_ppp_set_acfc(GAtPPP *ppp, gboolean enabled)
>>> as function name. Duplicating enabled in the function name and as
>>> parameter seems to be bit odd.
>>
>> For APIs I generally prefer:
>>
>> _set_foo() when foo is a 'thing', e.g int/string/struct/etc
>>
>> and _set_foo_enabled() when foo is on/off as that intent is clearer when
>> reading the code.
> 
> fair enough.
> 
> However we have not been 100% consistent then here.
> 
> g_at_server_set_echo()
> g_at_hdlc_set_no_carrier_detect()
> 
> Both look at bit fishy with this API intention. Should we fix them as
> well then while at it?

Yes, I think set_echo definitely should become set_echo_enabled

set_no_carrier_detect_enabled would pretty long and I wouldn't mind
having a better name for this one...

Regards,
-Denis

Re: [PATCH_v3 1/4] GAtPPP: Add ACFC option support

[Marcel Holtmann]

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

Hi Denis,

> > @@ -658,6 +703,11 @@ void g_at_ppp_set_server_info(GAtPPP *ppp, const char *remote,
> >  	ipcp_set_server_info(ppp->ipcp, r, d1, d2);
> >  }
> >  
> > +void g_at_ppp_set_acfc_enabled(GAtPPP *ppp)
> 
> I really do want the signature to be g_at_ppp_set_acfc_enabled(GAtPPP
> *ppp, gboolean enabled)
> 
> There are cases where we might re-use the PPP object with different
> parameters.

what is wrong with just g_at_ppp_set_acfc(GAtPPP *ppp, gboolean enabled)
as function name. Duplicating enabled in the function name and as
parameter seems to be bit odd.

Regards

Marcel

Re: [PATCH_v3 1/4] GAtPPP: Add ACFC option support

[Marcel Holtmann]

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

Hi Denis,

> >>> @@ -658,6 +703,11 @@ void g_at_ppp_set_server_info(GAtPPP *ppp, const char *remote,
> >>>  	ipcp_set_server_info(ppp->ipcp, r, d1, d2);
> >>>  }
> >>>  
> >>> +void g_at_ppp_set_acfc_enabled(GAtPPP *ppp)
> >>
> >> I really do want the signature to be g_at_ppp_set_acfc_enabled(GAtPPP
> >> *ppp, gboolean enabled)
> >>
> >> There are cases where we might re-use the PPP object with different
> >> parameters.
> > 
> > what is wrong with just g_at_ppp_set_acfc(GAtPPP *ppp, gboolean enabled)
> > as function name. Duplicating enabled in the function name and as
> > parameter seems to be bit odd.
> 
> For APIs I generally prefer:
> 
> _set_foo() when foo is a 'thing', e.g int/string/struct/etc
> 
> and _set_foo_enabled() when foo is on/off as that intent is clearer when
> reading the code.

fair enough.

However we have not been 100% consistent then here.

g_at_server_set_echo()
g_at_hdlc_set_no_carrier_detect()

Both look at bit fishy with this API intention. Should we fix them as
well then while at it?

Regards

Marcel