9:55 p.m.
Hi,
Those 2 patches, are implementing:
- Address and Control Field Compression option
- Protocol Field Compression option
It has been tested with gsmdial that do not negotiate any options and Win7
that negotiate both option.
An improvement of gsmdial and some ppp_lcp APIs could be done to test options
separately e.g. only PFC negotiated and only ACFC negotiated
Guillaume Zajac (2):
GAtPPP: Add ACFC option support
GAtPPP: Add PFC option support
gatchat/gatppp.c | 169 ++++++++++++++++++++++++++++++++++++++++++++++-------
gatchat/ppp.h | 10 +++
gatchat/ppp_lcp.c | 49 ++++++++++++++-
3 files changed, 203 insertions(+), 25 deletions(-)
9:55 p.m.
New subject: [PATCH 1/2] GAtPPP: Add ACFC option support
---
gatchat/gatppp.c | 116 ++++++++++++++++++++++++++++++++++++++++++----------
gatchat/ppp.h | 7 +++
gatchat/ppp_lcp.c | 27 +++++++++++-
3 files changed, 124 insertions(+), 26 deletions(-)
diff --git a/gatchat/gatppp.c b/gatchat/gatppp.c
index 5fb4146..a505db1 100644
--- a/gatchat/gatppp.c
+++ b/gatchat/gatppp.c
@@ -83,6 +83,9 @@ struct _GAtPPP {
int fd;
guint guard_timeout_source;
gboolean suspended;
+ gboolean acfc;
+ guint8 address;
+ guint8 control;
};
void ppp_debug(GAtPPP *ppp, const char *str)
@@ -168,17 +171,39 @@ 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;
+ /*
+ * RFC 1661:
+ * If we received PPP packets with Adress and Control Field Compressed
+ * although ACFC had not been negociated, we silently discard them.
+ */
+ if (!ppp->acfc && acfc_frame)
+ return;
+
switch (protocol) {
case PPP_IP_PROTO:
ppp_net_process_packet(ppp->net, packet);
break;
case LCP_PROTOCOL:
+ /* Those values will be tested while negociating ACFC option */
+ ppp->address = header->address;
+ ppp->control = header->control;
pppcp_process_packet(ppp->lcp, packet);
break;
case IPCP_PROTO:
@@ -196,37 +221,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 +268,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->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 +443,23 @@ void ppp_set_mtu(GAtPPP *ppp, const guint8 *data)
ppp->mtu = mtu;
}
+gboolean ppp_set_acfc_enabled(GAtPPP *ppp, gboolean acfc)
+{
+ /*
+ * RFC 1662:
+ * When using the standard HDLC-like framing, the Address and Control
+ * fields contain the hexadecimal values 0xff and 0x03 respectively.
+ * When other Address or Control field values are in use, Address-
+ * and-Control-Field-Compression MUST NOT be negotiated.
+ */
+ if (ppp->address == 0xFF && ppp->control == 0x03)
+ ppp->acfc = acfc;
+ else
+ ppp->acfc = FALSE;
+
+ return ppp->acfc;
+}
+
static void io_disconnect(gpointer user_data)
{
GAtPPP *ppp = user_data;
diff --git a/gatchat/ppp.h b/gatchat/ppp.h
index 023d779..3cf11f7 100644
--- a/gatchat/ppp.h
+++ b/gatchat/ppp.h
@@ -85,6 +85,12 @@ 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);
@@ -125,4 +131,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);
+gboolean ppp_set_acfc_enabled(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..24e7bae 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,6 +101,13 @@ 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;
}
@@ -110,6 +118,12 @@ static void lcp_reset_config_options(struct lcp_data *lcp)
lcp_generate_config_options(lcp);
}
+static void lcp_set_acfc_config_option(struct lcp_data *lcp)
+{
+ lcp->req_options |= REQ_OPTION_ACFC;
+ lcp_generate_config_options(lcp);
+}
+
/*
* signal the Up event to the NCP
*/
@@ -286,9 +300,16 @@ static enum rcr_result lcp_rcr(struct pppcp_data *pppcp,
break;
case MAGIC_NUMBER:
case PFC:
- case ACFC:
/* don't care */
break;
+ case ACFC:
+ if (ppp_set_acfc_enabled(ppp, TRUE)) {
+ struct lcp_data *lcp = pppcp_get_data(pppcp);
+ lcp_set_acfc_config_option(lcp);
+ pppcp_set_local_options(pppcp, lcp->options,
+ lcp->options_len);
+ }
+ break;
}
}
--
1.7.1
4:58 a.m.
New subject: [PATCH 1/2] GAtPPP: Add ACFC option support
Hi Guillaume,
On 06/16/2011 06:55 AM, Guillaume Zajac wrote:
---
gatchat/gatppp.c | 116 ++++++++++++++++++++++++++++++++++++++++++----------
gatchat/ppp.h | 7 +++
gatchat/ppp_lcp.c | 27 +++++++++++-
3 files changed, 124 insertions(+), 26 deletions(-)
diff --git a/gatchat/gatppp.c b/gatchat/gatppp.c
index 5fb4146..a505db1 100644
--- a/gatchat/gatppp.c
+++ b/gatchat/gatppp.c
@@ -83,6 +83,9 @@ struct _GAtPPP {
int fd;
guint guard_timeout_source;
gboolean suspended;
+ gboolean acfc;
+ guint8 address;
+ guint8 control;
I'm lost as to the purpose of the address and control variables in the
GAtPPP structure?
};
void ppp_debug(GAtPPP *ppp, const char *str)
@@ -168,17 +171,39 @@ 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);
+ }
Looks good to me
if (ppp_drop_packet(ppp, protocol))
return;
+ /*
+ * RFC 1661:
+ * If we received PPP packets with Adress and Control Field Compressed
+ * although ACFC had not been negociated, we silently discard them.
+ */
+ if (!ppp->acfc && acfc_frame)
+ return;
+
Please use ppp->acfc == FALSE here
switch (protocol) {
case PPP_IP_PROTO:
ppp_net_process_packet(ppp->net, packet);
break;
case LCP_PROTOCOL:
+ /* Those values will be tested while negociating ACFC option */
+ ppp->address = header->address;
+ ppp->control = header->control;
pppcp_process_packet(ppp->lcp, packet);
break;
case IPCP_PROTO:
@@ -196,37 +221,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 +268,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->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;
+ }
+}
+
Looks fine to me
static inline void ppp_enter_phase(GAtPPP *ppp, enum ppp_phase
phase)
{
DBG(ppp, "%d", phase);
@@ -390,6 +443,23 @@ void ppp_set_mtu(GAtPPP *ppp, const guint8 *data)
ppp->mtu = mtu;
}
+gboolean ppp_set_acfc_enabled(GAtPPP *ppp, gboolean acfc)
+{
+ /*
+ * RFC 1662:
+ * When using the standard HDLC-like framing, the Address and Control
+ * fields contain the hexadecimal values 0xff and 0x03 respectively.
+ * When other Address or Control field values are in use, Address-
+ * and-Control-Field-Compression MUST NOT be negotiated.
+ */
+ if (ppp->address == 0xFF && ppp->control == 0x03)
+ ppp->acfc = acfc;
There is no need for this check. The sender is responsible for not
compressing the packets if the address / control do not correspond to
the well known ones.
+ else
+ ppp->acfc = FALSE;
+
+ return ppp->acfc;
+}
+
So this seems wrong to me. The purpose of ppp_set_acfc_enabled is twofold:
- Whether we should send the ACFC option to the peer (e.g. we can accept
ACFC packets)
- Whether we should try to send ACFC packets (assuming the peer tells us
it is capable of doing so)
So what this function should be doing is setting lcp->req_options
accordingly (somehow).
ppp->acfc should not really change 'per packet', but a global setting
for the GAtPPP object
static void io_disconnect(gpointer user_data)
{
GAtPPP *ppp = user_data;
diff --git a/gatchat/ppp.h b/gatchat/ppp.h
index 023d779..3cf11f7 100644
--- a/gatchat/ppp.h
+++ b/gatchat/ppp.h
@@ -85,6 +85,12 @@ 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);
@@ -125,4 +131,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);
+gboolean ppp_set_acfc_enabled(GAtPPP *ppp, gboolean acfc);
This function should be g_at_ppp_set_acfc_enabled() and be in gatppp.h
struct ppp_header *ppp_packet_new(gsize infolen, guint16 protocol);
diff --git a/gatchat/ppp_lcp.c b/gatchat/ppp_lcp.c
index ce9dae2..24e7bae 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,6 +101,13 @@ 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;
}
@@ -110,6 +118,12 @@ static void lcp_reset_config_options(struct lcp_data *lcp)
lcp_generate_config_options(lcp);
}
+static void lcp_set_acfc_config_option(struct lcp_data *lcp)
+{
+ lcp->req_options |= REQ_OPTION_ACFC;
+ lcp_generate_config_options(lcp);
+}
+
/*
* signal the Up event to the NCP
*/
@@ -286,9 +300,16 @@ static enum rcr_result lcp_rcr(struct pppcp_data *pppcp,
break;
case MAGIC_NUMBER:
case PFC:
- case ACFC:
/* don't care */
break;
+ case ACFC:
+ if (ppp_set_acfc_enabled(ppp, TRUE)) {
+ struct lcp_data *lcp = pppcp_get_data(pppcp);
+ lcp_set_acfc_config_option(lcp);
+ pppcp_set_local_options(pppcp, lcp->options,
+ lcp->options_len);
+ }
+ break;
Again, this seems wrong. What is happening here is that the peer just
told us that 'Yes, I can accept ACFC packets'.
What you're trying to do is set whether our end can accept ACFC packets
based on whether the peer can. In fact, they are completely
independent. The relationship can be asymmetric, e.g. client that can
send ACFC packets, but not accept them, and vice versa. You need to
account for this.
}
}
Regards,
-Denis
6:41 p.m.
New subject: [PATCH 1/2] GAtPPP: Add ACFC option support
Hi Denis,
On 23/06/2011 20:58, Denis Kenzior wrote:
Hi Guillaume,
On 06/16/2011 06:55 AM, Guillaume Zajac wrote:
> ---
> gatchat/gatppp.c | 116 ++++++++++++++++++++++++++++++++++++++++++----------
> gatchat/ppp.h | 7 +++
> gatchat/ppp_lcp.c | 27 +++++++++++-
> 3 files changed, 124 insertions(+), 26 deletions(-)
>
> diff --git a/gatchat/gatppp.c b/gatchat/gatppp.c
> index 5fb4146..a505db1 100644
> --- a/gatchat/gatppp.c
> +++ b/gatchat/gatppp.c
> @@ -83,6 +83,9 @@ struct _GAtPPP {
> int fd;
> guint guard_timeout_source;
> gboolean suspended;
> + gboolean acfc;
> + guint8 address;
> + guint8 control;
I'm lost as to the purpose of the address and control variables in the
GAtPPP structure?
It was just for testing those values while negociating ACFC but as peet
is reponsible from them, I will delete them.
> };
>
> void ppp_debug(GAtPPP *ppp, const char *str)
> @@ -168,17 +171,39 @@ 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);
> + }
>
Looks good to me
> if (ppp_drop_packet(ppp, protocol))
> return;
>
> + /*
> + * RFC 1661:
> + * If we received PPP packets with Adress and Control Field Compressed
> + * although ACFC had not been negociated, we silently discard them.
> + */
> + if (!ppp->acfc&& acfc_frame)
> + return;
> +
Please use ppp->acfc == FALSE here
Ok
> switch (protocol) {
> case PPP_IP_PROTO:
> ppp_net_process_packet(ppp->net, packet);
> break;
> case LCP_PROTOCOL:
> + /* Those values will be tested while negociating ACFC option */
> + ppp->address = header->address;
> + ppp->control = header->control;
> pppcp_process_packet(ppp->lcp, packet);
> break;
> case IPCP_PROTO:
> @@ -196,37 +221,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 +268,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->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;
> + }
> +}
> +
Looks fine to me
> static inline void ppp_enter_phase(GAtPPP *ppp, enum ppp_phase phase)
> {
> DBG(ppp, "%d", phase);
> @@ -390,6 +443,23 @@ void ppp_set_mtu(GAtPPP *ppp, const guint8 *data)
> ppp->mtu = mtu;
> }
>
> +gboolean ppp_set_acfc_enabled(GAtPPP *ppp, gboolean acfc)
> +{
> + /*
> + * RFC 1662:
> + * When using the standard HDLC-like framing, the Address and Control
> + * fields contain the hexadecimal values 0xff and 0x03 respectively.
> + * When other Address or Control field values are in use, Address-
> + * and-Control-Field-Compression MUST NOT be negotiated.
> + */
> + if (ppp->address == 0xFF&& ppp->control == 0x03)
> + ppp->acfc = acfc;
There is no need for this check. The sender is responsible for not
compressing the packets if the address / control do not correspond to
the well known ones.
> + else
> + ppp->acfc = FALSE;
> +
> + return ppp->acfc;
> +}
> +
So this seems wrong to me. The purpose of ppp_set_acfc_enabled is twofold:
- Whether we should send the ACFC option to the peer (e.g. we can accept
ACFC packets)
- Whether we should try to send ACFC packets (assuming the peer tells us
it is capable of doing so)
So what this function should be doing is setting lcp->req_options
accordingly (somehow).
ppp->acfc should not really change 'per packet', but a global setting
for the GAtPPP object
> static void io_disconnect(gpointer user_data)
> {
> GAtPPP *ppp = user_data;
> diff --git a/gatchat/ppp.h b/gatchat/ppp.h
> index 023d779..3cf11f7 100644
> --- a/gatchat/ppp.h
> +++ b/gatchat/ppp.h
> @@ -85,6 +85,12 @@ 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);
> @@ -125,4 +131,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);
> +gboolean ppp_set_acfc_enabled(GAtPPP *ppp, gboolean acfc);
This function should be g_at_ppp_set_acfc_enabled() and be in gatppp.h
> struct ppp_header *ppp_packet_new(gsize infolen, guint16 protocol);
> diff --git a/gatchat/ppp_lcp.c b/gatchat/ppp_lcp.c
> index ce9dae2..24e7bae 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,6 +101,13 @@ 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;
> }
>
> @@ -110,6 +118,12 @@ static void lcp_reset_config_options(struct lcp_data *lcp)
> lcp_generate_config_options(lcp);
> }
>
> +static void lcp_set_acfc_config_option(struct lcp_data *lcp)
> +{
> + lcp->req_options |= REQ_OPTION_ACFC;
> + lcp_generate_config_options(lcp);
> +}
> +
> /*
> * signal the Up event to the NCP
> */
> @@ -286,9 +300,16 @@ static enum rcr_result lcp_rcr(struct pppcp_data *pppcp,
> break;
> case MAGIC_NUMBER:
> case PFC:
> - case ACFC:
> /* don't care */
> break;
> + case ACFC:
> + if (ppp_set_acfc_enabled(ppp, TRUE)) {
> + struct lcp_data *lcp = pppcp_get_data(pppcp);
> + lcp_set_acfc_config_option(lcp);
> + pppcp_set_local_options(pppcp, lcp->options,
> + lcp->options_len);
> + }
> + break;
Again, this seems wrong. What is happening here is that the peer just
told us that 'Yes, I can accept ACFC packets'.
What you're trying to do is set whether our end can accept ACFC packets
based on whether the peer can. In fact, they are completely
independent. The relationship can be asymmetric, e.g. client that can
send ACFC packets, but not accept them, and vice versa. You need to
account for this.
Right, so if we always set by default ACFC options into LCP configure
request.
I could dot into
static void lcp_reset_config_options(struct lcp_data *lcp)
{
/* Using the default ACCM */
lcp->req_options |= REQ_OPTION_ACFC;
lcp_generate_config_options(lcp);
}
On reception, we always say we can receive ACFC packet then it is up to
the peer to transmit or not ACFC packets.
GAtPPP object only needs to know if it has to transmit or not ACFC packets.
That is the purpose of ppp->acfc.
Kind regards,
Guillaume
12:42 a.m.
New subject: [PATCH 1/2] GAtPPP: Add ACFC option support
Hi Guillaume,
> Again, this seems wrong. What is happening here is that the peer
just
> told us that 'Yes, I can accept ACFC packets'.
>
> What you're trying to do is set whether our end can accept ACFC packets
> based on whether the peer can. In fact, they are completely
> independent. The relationship can be asymmetric, e.g. client that can
> send ACFC packets, but not accept them, and vice versa. You need to
> account for this.
>
Right, so if we always set by default ACFC options into LCP configure
request.
I could dot into
static void lcp_reset_config_options(struct lcp_data *lcp)
{
/* Using the default ACCM */
lcp->req_options |= REQ_OPTION_ACFC;
lcp_generate_config_options(lcp);
}
On reception, we always say we can receive ACFC packet then it is up to
the peer to transmit or not ACFC packets.
GAtPPP object only needs to know if it has to transmit or not ACFC packets.
That is the purpose of ppp->acfc.
I'd like to have the option of turning off ACFC completely, so it should
control:
1. Whether we advertise that we can receive ACFC packets
2. Whether we should try to send ACFC packets
Regards,
-Denis
1:20 a.m.
New subject: [PATCH 1/2] GAtPPP: Add ACFC option support
Hi Denis,
On 24/06/2011 16:42, Denis Kenzior wrote:
Hi Guillaume,
>> Again, this seems wrong. What is happening here is that the peer just
>> told us that 'Yes, I can accept ACFC packets'.
>>
>> What you're trying to do is set whether our end can accept ACFC packets
>> based on whether the peer can. In fact, they are completely
>> independent. The relationship can be asymmetric, e.g. client that can
>> send ACFC packets, but not accept them, and vice versa. You need to
>> account for this.
>>
> Right, so if we always set by default ACFC options into LCP configure
> request.
> I could dot into
>
> static void lcp_reset_config_options(struct lcp_data *lcp)
> {
> /* Using the default ACCM */
> lcp->req_options |= REQ_OPTION_ACFC;
> lcp_generate_config_options(lcp);
> }
>
>
> On reception, we always say we can receive ACFC packet then it is up to
> the peer to transmit or not ACFC packets.
>
> GAtPPP object only needs to know if it has to transmit or not ACFC packets.
> That is the purpose of ppp->acfc.
I'd like to have the option of turning off ACFC completely, so it should
control:
1. Whether we advertise that we can receive ACFC packets
2. Whether we should try to send ACFC packets
Fine I will add it.
I guess we will need same kind of options for PFC.
Kind regards,
Guillaume
1:21 a.m.
New subject: [PATCH 1/2] GAtPPP: Add ACFC option support
Hi Guillaume,
On 06/24/2011 10:20 AM, Guillaume Zajac wrote:
Hi Denis,
On 24/06/2011 16:42, Denis Kenzior wrote:
> Hi Guillaume,
>
>>> Again, this seems wrong. What is happening here is that the peer just
>>> told us that 'Yes, I can accept ACFC packets'.
>>>
>>> What you're trying to do is set whether our end can accept ACFC packets
>>> based on whether the peer can. In fact, they are completely
>>> independent. The relationship can be asymmetric, e.g. client that can
>>> send ACFC packets, but not accept them, and vice versa. You need to
>>> account for this.
>>>
>> Right, so if we always set by default ACFC options into LCP configure
>> request.
>> I could dot into
>>
>> static void lcp_reset_config_options(struct lcp_data *lcp)
>> {
>> /* Using the default ACCM */
>> lcp->req_options |= REQ_OPTION_ACFC;
>> lcp_generate_config_options(lcp);
>> }
>>
>>
>> On reception, we always say we can receive ACFC packet then it is up to
>> the peer to transmit or not ACFC packets.
>>
>> GAtPPP object only needs to know if it has to transmit or not ACFC
>> packets.
>> That is the purpose of ppp->acfc.
> I'd like to have the option of turning off ACFC completely, so it should
> control:
> 1. Whether we advertise that we can receive ACFC packets
> 2. Whether we should try to send ACFC packets
>
Fine I will add it.
I guess we will need same kind of options for PFC.
Yes please ;)
Regards,
-Denis
9:55 p.m.
New subject: [PATCH 2/2] GAtPPP: Add PFC option support
---
gatchat/gatppp.c | 57 ++++++++++++++++++++++++++++++++++++++++++++++++++++-
gatchat/ppp.h | 3 ++
gatchat/ppp_lcp.c | 28 +++++++++++++++++++++++--
3 files changed, 84 insertions(+), 4 deletions(-)
diff --git a/gatchat/gatppp.c b/gatchat/gatppp.c
index a505db1..6704536 100644
--- a/gatchat/gatppp.c
+++ b/gatchat/gatppp.c
@@ -86,6 +86,7 @@ struct _GAtPPP {
gboolean acfc;
guint8 address;
guint8 control;
+ gboolean pfc;
};
void ppp_debug(GAtPPP *ppp, const char *str)
@@ -174,6 +175,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;
@@ -185,6 +187,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 (ppp->pfc && 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;
@@ -277,6 +293,32 @@ static void ppp_send_acfc_frame(GAtPPP *ppp, guint8 *packet,
if (ppp->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->acfc && ppp->pfc)
+ offset = 3;
+ else if (ppp->acfc)
+ offset = 2;
+ else if (ppp->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)
@@ -298,9 +340,17 @@ 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);
}
}
@@ -460,6 +510,11 @@ gboolean ppp_set_acfc_enabled(GAtPPP *ppp, gboolean acfc)
return ppp->acfc;
}
+void ppp_set_pfc_enabled(GAtPPP *ppp, gboolean pfc)
+{
+ ppp->pfc = pfc;
+}
+
static void io_disconnect(gpointer user_data)
{
GAtPPP *ppp = user_data;
diff --git a/gatchat/ppp.h b/gatchat/ppp.h
index 3cf11f7..5efda31 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); \
@@ -132,4 +134,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);
gboolean ppp_set_acfc_enabled(GAtPPP *ppp, gboolean acfc);
+void ppp_set_pfc_enabled(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 24e7bae..2b62d25 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;
}
@@ -124,6 +132,12 @@ static void lcp_set_acfc_config_option(struct lcp_data *lcp)
lcp_generate_config_options(lcp);
}
+static void lcp_set_pfc_config_option(struct lcp_data *lcp)
+{
+ lcp->req_options |= REQ_OPTION_PFC;
+ lcp_generate_config_options(lcp);
+}
+
/*
* signal the Up event to the NCP
*/
@@ -299,9 +313,17 @@ 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);
+ ppp_set_pfc_enabled(ppp, TRUE);
+ lcp_set_pfc_config_option(lcp);
+ pppcp_set_local_options(pppcp, lcp->options,
+ lcp->options_len);
+ break;
+ }
case ACFC:
if (ppp_set_acfc_enabled(ppp, TRUE)) {
struct lcp_data *lcp = pppcp_get_data(pppcp);
--
1.7.1
4104
days inactive
4112
days old
7 comments
2 participants
participants (2)
-
Denis Kenzior -
Guillaume Zajac