Attention is currently required from: jolly.
pespin has posted comments on this change by jolly. ( https://gerrit.osmocom.org/c/libosmo-sigtran/+/43667?usp=email )
Change subject: WIP: Cisco like DSCP configuration support ......................................................................
Patch Set 1:
(14 comments)
This change is ready for review.
Patchset:
PS1: TODO: figure out if it makes sense to provide a default qos, and with with ip-dscp, etc.
File include/osmocom/sigtran/osmo_ss7.h:
https://gerrit.osmocom.org/c/libosmo-sigtran/+/43667/comment/b87f878f_eb6b2c... : PS1, Line 155: * QoS Class Better create a new private ss7_qos_class.h + ss7_qos_class.c inside src/, and avoid prefixing it with osmo_ to avoid making them public until there's a real need for them to be public. This way we can extend without breaking the API.
https://gerrit.osmocom.org/c/libosmo-sigtran/+/43667/comment/afd2067c_a1e1be... : PS1, Line 167: osmo_ss7_qos_class_find(struct osmo_ss7_instance *inst, uint8_t qos_class); This one can go in ss7_instance.h + ss7_instance.c
File src/Makefile.am:
https://gerrit.osmocom.org/c/libosmo-sigtran/+/43667/comment/ed222008_955d41... : PS1, Line 92: ss7_qos_class_vty.c \ good, you need to do the same with ss7_qos_class.c and .h.
File src/ss7_asp_vty.c:
https://gerrit.osmocom.org/c/libosmo-sigtran/+/43667/comment/6a792eb9_260a68... : PS1, Line 414: "qos-class " IP_QOS_CLASS_RANGE_STR, SS7_QOS_CLASS_RANGE_STR
https://gerrit.osmocom.org/c/libosmo-sigtran/+/43667/comment/0e0613f2_81db4c... : PS1, Line 423: asp->cfg.ip_dscp = 0; why do you keep asp->cfg.ip_dscp? IMHO it should be removed now that we have the qos-class stuff.
https://gerrit.osmocom.org/c/libosmo-sigtran/+/43667/comment/a1562528_b29917... : PS1, Line 443: DEFUN_ATTR(asp_ip_dscps, asp_ip_dscp_cmd, This should be deprecated in some way imho.
File src/ss7_instance.c:
https://gerrit.osmocom.org/c/libosmo-sigtran/+/43667/comment/3837f739_7db64d... : PS1, Line 363: llist_for_each_entry(xua, &inst->xua_servers, list) { Cisco ITP doesn't have qos-class node for "listen" node, but I think it's actually a good idea to use it here.
https://gerrit.osmocom.org/c/libosmo-sigtran/+/43667/comment/ae9c485b_bcc2f2... : PS1, Line 392: void osmo_ss7_qos_class_update(struct osmo_ss7_qos_class *qos) This can go into ss7_qos_class.c too. btw I don't recall seeing it declared in the header file?
File src/ss7_qos_class_vty.c:
https://gerrit.osmocom.org/c/libosmo-sigtran/+/43667/comment/bf895354_4d8f14... : PS1, Line 112: if (qos->ip_dscp) { I think is "qos" exists it should always print the "qos class" node, even if empty.
https://gerrit.osmocom.org/c/libosmo-sigtran/+/43667/comment/b1000191_f8c93f... : PS1, Line 114: if (qos->ip_dscp) this is always true in this path?
File src/ss7_vty.c:
https://gerrit.osmocom.org/c/libosmo-sigtran/+/43667/comment/baaa5a1e_d2538e... : PS1, Line 1409: /* then dump ASPs, as ASs reference them */ I think you can simplify and drop all the connectors "first, then, now, finally" ;)
https://gerrit.osmocom.org/c/libosmo-sigtran/+/43667/comment/ef4efdbd_1854a2... : PS1, Line 1442: return ss7_vty_node_qos_class_go_parent(vty); Are you sure the func is needed? or simply apply it directly here like others?
https://gerrit.osmocom.org/c/libosmo-sigtran/+/43667/comment/823a30d6_ac7f52... : PS1, Line 1548: remove this empty line.