Change in libosmocore[master]: ns2: add support for frame relay

[lynxis lazus]

lynxis lazus has posted comments on this change. ( https://gerrit.osmocom.org/c/libosmocore/+/21243 )

Change subject: ns2: add support for frame relay
......................................................................


Patch Set 1:

(16 comments)

https://gerrit.osmocom.org/c/libosmocore/+/21243/1/include/osmocom/gprs/frame_relay.h 
File include/osmocom/gprs/frame_relay.h:

https://gerrit.osmocom.org/c/libosmocore/+/21243/1/include/osmocom/gprs/frame_relay.h@1 
PS1, Line 1: /*! \file frame_relay.h */
> missing copyright / license statement (yes, my fault in the initial code, but I don't want to mess w […]
Ack


https://gerrit.osmocom.org/c/libosmocore/+/21243/1/src/gb/frame_relay.c 
File src/gb/frame_relay.c:

https://gerrit.osmocom.org/c/libosmocore/+/21243/1/src/gb/frame_relay.c@14 
PS1, Line 14: 
> missing copyright / license statement (yes, my fault in the initial code, but I don't want to mess w […]
Ack


https://gerrit.osmocom.org/c/libosmocore/+/21243/1/src/gb/frame_relay.c@119 
PS1, Line 119: //		.default_val = 10,
> ?
Ack


https://gerrit.osmocom.org/c/libosmocore/+/21243/1/src/gb/frame_relay.c@222 
PS1, Line 222: 		ie[2] |= 0x02;
> They are the bits we need to use for the "active" "add" and "del" situations. […]
Ack


https://gerrit.osmocom.org/c/libosmocore/+/21243/1/src/gb/frame_relay.c@276 
PS1, Line 276: 
> line can be dropped.
can :). I like a newline here because there is an if () without {} there.


https://gerrit.osmocom.org/c/libosmocore/+/21243/1/src/gb/frame_relay.c@310 
PS1, Line 310: 		LOGPFRL(link, LOGL_ERROR, "STATUS-ENQ aren't support for role user\n");
> "are not supported"
Ack


https://gerrit.osmocom.org/c/libosmocore/+/21243/1/src/gb/frame_relay.c@366 
PS1, Line 366: 	link->succeed = link->succeed << 1;
> works either way, there is no rule that your version is superior or preferred, IMHO. […]
Ack


https://gerrit.osmocom.org/c/libosmocore/+/21243/1/src/gb/frame_relay.c@370 
PS1, Line 370: 	/* count the bits */
> It would be great to explain a bit better what exactly are you doing here or provide some spec refer […]
Ack


https://gerrit.osmocom.org/c/libosmocore/+/21243/1/src/gb/frame_relay.c@461 
PS1, Line 461: 				LOGPFRL(link, LOGL_ERROR, "Could not create DLC %d\n", dlci);
> why? all the other DLCs in the message might just as well have been created?  This function is execu […]
i'll continue in this case.


https://gerrit.osmocom.org/c/libosmocore/+/21243/1/src/gb/frame_relay.c@657 
PS1, Line 657: 	tlv_parse2(tp, MAX_SUPPORTED_PVC + 1, &q933_att_tlvdef, msgb_l3(msg) + sizeof(*qh), msgb_l3len(msg) - sizeof(*qh), 0, 0);
> check return code.
Ack


https://gerrit.osmocom.org/c/libosmocore/+/21243/1/src/gb/frame_relay.c@889 
PS1, Line 889: /* TODO: osmo_fr_dlc_alloc with deregistering it from the link in fr-net */
> you probably mean _free here?
Ack


https://gerrit.osmocom.org/c/libosmocore/+/21243/1/src/gb/gprs_ns2.c 
File src/gb/gprs_ns2.c:

https://gerrit.osmocom.org/c/libosmocore/+/21243/1/src/gb/gprs_ns2.c@262 
PS1, Line 262: 		snprintf(buf, buf_len, "fr)netif: %s dlci: %s", "hdlcX", "unsupported");
> this needs work to print correct variables right?
Ack


https://gerrit.osmocom.org/c/libosmocore/+/21243/1/src/gb/gprs_ns2_fr.c 
File src/gb/gprs_ns2_fr.c:

https://gerrit.osmocom.org/c/libosmocore/+/21243/1/src/gb/gprs_ns2_fr.c@86 
PS1, Line 86: struct iphdr
> We don't really support FreeBSD, Apple or Cygwin platforms, so best to remove it altogether. […]
Ack


https://gerrit.osmocom.org/c/libosmocore/+/21243/1/src/gb/gprs_ns2_fr.c@119 
PS1, Line 119: 	char netif[IF_NAMESIZE + 1];
> IFNAMSIZ. That's the maximum size including the null char.
Ack


https://gerrit.osmocom.org/c/libosmocore/+/21243/1/src/gb/gprs_ns2_vty.c 
File src/gb/gprs_ns2_vty.c:

https://gerrit.osmocom.org/c/libosmocore/+/21243/1/src/gb/gprs_ns2_vty.c@86 
PS1, Line 86: 	char netif[IF_NAMESIZE + 1];
> remove +1
Ack


https://gerrit.osmocom.org/c/libosmocore/+/21243/1/src/gb/gprs_ns2_vty.c@484 
PS1, Line 484: 	uint16_t dlci = atoi(argv[2]);
> This looks wrong, I only see 2 arguments in this vty cmd.
I'm still unsure if I want this in the change.



-- 
To view, visit https://gerrit.osmocom.org/c/libosmocore/+/21243
To unsubscribe, or for help writing mail filters, visit https://gerrit.osmocom.org/settings

Gerrit-Project: libosmocore
Gerrit-Branch: master
Gerrit-Change-Id: Id3b49f93d33c271f77cd9c9db03cde6b727a4d30
Gerrit-Change-Number: 21243
Gerrit-PatchSet: 1
Gerrit-Owner: lynxis lazus <lynxis at fe80.eu>
Gerrit-Reviewer: Jenkins Builder
Gerrit-Reviewer: pespin <pespin at sysmocom.de>
Gerrit-CC: laforge <laforge at osmocom.org>
Gerrit-Comment-Date: Tue, 24 Nov 2020 02:37:04 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Comment-In-Reply-To: laforge <laforge at osmocom.org>
Comment-In-Reply-To: pespin <pespin at sysmocom.de>
Gerrit-MessageType: comment
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.osmocom.org/pipermail/gerrit-log/attachments/20201124/9a8dabbe/attachment.htm>