* [PATCH 0/2] ACFC and PFC options implementation @ 2011-06-16 11:55 Guillaume Zajac 2011-06-16 11:55 ` [PATCH 1/2] GAtPPP: Add ACFC option support Guillaume Zajac 2011-06-16 11:55 ` [PATCH 2/2] GAtPPP: Add PFC " Guillaume Zajac 0 siblings, 2 replies; 8+ messages in thread From: Guillaume Zajac @ 2011-06-16 11:55 UTC (permalink / raw) To: ofono [-- Attachment #1: Type: text/plain, Size: 684 bytes --] 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(-) ^ permalink raw reply [flat|nested] 8+ messages in thread
* [PATCH 1/2] GAtPPP: Add ACFC option support 2011-06-16 11:55 [PATCH 0/2] ACFC and PFC options implementation Guillaume Zajac @ 2011-06-16 11:55 ` Guillaume Zajac 2011-06-23 18:58 ` Denis Kenzior 2011-06-16 11:55 ` [PATCH 2/2] GAtPPP: Add PFC " Guillaume Zajac 1 sibling, 1 reply; 8+ messages in thread From: Guillaume Zajac @ 2011-06-16 11:55 UTC (permalink / raw) To: ofono [-- Attachment #1: Type: text/plain, Size: 7640 bytes --] --- 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 ^ permalink raw reply related [flat|nested] 8+ messages in thread
* Re: [PATCH 1/2] GAtPPP: Add ACFC option support 2011-06-16 11:55 ` [PATCH 1/2] GAtPPP: Add ACFC option support Guillaume Zajac @ 2011-06-23 18:58 ` Denis Kenzior 2011-06-24 8:41 ` Guillaume Zajac 0 siblings, 1 reply; 8+ messages in thread From: Denis Kenzior @ 2011-06-23 18:58 UTC (permalink / raw) To: ofono [-- Attachment #1: Type: text/plain, Size: 9551 bytes --] 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 ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH 1/2] GAtPPP: Add ACFC option support 2011-06-23 18:58 ` Denis Kenzior @ 2011-06-24 8:41 ` Guillaume Zajac 2011-06-24 14:42 ` Denis Kenzior 0 siblings, 1 reply; 8+ messages in thread From: Guillaume Zajac @ 2011-06-24 8:41 UTC (permalink / raw) To: ofono [-- Attachment #1: Type: text/plain, Size: 10606 bytes --] 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 ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH 1/2] GAtPPP: Add ACFC option support 2011-06-24 8:41 ` Guillaume Zajac @ 2011-06-24 14:42 ` Denis Kenzior 2011-06-24 15:20 ` Guillaume Zajac 0 siblings, 1 reply; 8+ messages in thread From: Denis Kenzior @ 2011-06-24 14:42 UTC (permalink / raw) To: ofono [-- Attachment #1: Type: text/plain, Size: 1227 bytes --] 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 ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH 1/2] GAtPPP: Add ACFC option support 2011-06-24 14:42 ` Denis Kenzior @ 2011-06-24 15:20 ` Guillaume Zajac 2011-06-24 15:21 ` Denis Kenzior 0 siblings, 1 reply; 8+ messages in thread From: Guillaume Zajac @ 2011-06-24 15:20 UTC (permalink / raw) To: ofono [-- Attachment #1: Type: text/plain, Size: 1399 bytes --] 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 ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH 1/2] GAtPPP: Add ACFC option support 2011-06-24 15:20 ` Guillaume Zajac @ 2011-06-24 15:21 ` Denis Kenzior 0 siblings, 0 replies; 8+ messages in thread From: Denis Kenzior @ 2011-06-24 15:21 UTC (permalink / raw) To: ofono [-- Attachment #1: Type: text/plain, Size: 1527 bytes --] 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 ^ permalink raw reply [flat|nested] 8+ messages in thread
* [PATCH 2/2] GAtPPP: Add PFC option support 2011-06-16 11:55 [PATCH 0/2] ACFC and PFC options implementation Guillaume Zajac 2011-06-16 11:55 ` [PATCH 1/2] GAtPPP: Add ACFC option support Guillaume Zajac @ 2011-06-16 11:55 ` Guillaume Zajac 1 sibling, 0 replies; 8+ messages in thread From: Guillaume Zajac @ 2011-06-16 11:55 UTC (permalink / raw) To: ofono [-- Attachment #1: Type: text/plain, Size: 5640 bytes --] --- 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 ^ permalink raw reply related [flat|nested] 8+ messages in thread
end of thread, other threads:[~2011-06-24 15:21 UTC | newest] Thread overview: 8+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2011-06-16 11:55 [PATCH 0/2] ACFC and PFC options implementation Guillaume Zajac 2011-06-16 11:55 ` [PATCH 1/2] GAtPPP: Add ACFC option support Guillaume Zajac 2011-06-23 18:58 ` Denis Kenzior 2011-06-24 8:41 ` Guillaume Zajac 2011-06-24 14:42 ` Denis Kenzior 2011-06-24 15:20 ` Guillaume Zajac 2011-06-24 15:21 ` Denis Kenzior 2011-06-16 11:55 ` [PATCH 2/2] GAtPPP: Add PFC " Guillaume Zajac
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.