Attention is currently required from: pespin.
jolly has posted comments on this change by jolly. ( https://gerrit.osmocom.org/c/libosmo-sigtran/+/43667?usp=email )
Change subject: Add Cisco like DSCP configuration support ......................................................................
Patch Set 2:
(13 comments)
File include/osmocom/sigtran/osmo_ss7.h:
https://gerrit.osmocom.org/c/libosmo-sigtran/+/43667/comment/f6efde96_c75a93... : PS1, Line 155: * QoS Class
Better create a new private ss7_qos_class.h + ss7_qos_class. […]
Done
https://gerrit.osmocom.org/c/libosmo-sigtran/+/43667/comment/2d8004b1_da5ca6... : 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. […]
functions moved to ss7_qos_class.c/h
File src/Makefile.am:
https://gerrit.osmocom.org/c/libosmo-sigtran/+/43667/comment/75abf957_e6ec64... : PS1, Line 92: ss7_qos_class_vty.c \
good, you need to do the same with ss7_qos_class.c and .h.
Done
File src/ss7_asp_vty.c:
https://gerrit.osmocom.org/c/libosmo-sigtran/+/43667/comment/57ebe6bd_c62548... : PS1, Line 414: "qos-class " IP_QOS_CLASS_RANGE_STR,
SS7_QOS_CLASS_RANGE_STR
Done
https://gerrit.osmocom.org/c/libosmo-sigtran/+/43667/comment/56c169d8_c5a3b9... : 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.
You were mentioning in the ticket OS#6753:
"Since we already released libosmo-sigtran with the "ip-dscp" command, it is fine to keep it as an alternative way to configure the DSCP per ASP."
https://gerrit.osmocom.org/c/libosmo-sigtran/+/43667/comment/ac3fba11_1d6f81... : PS1, Line 443: DEFUN_ATTR(asp_ip_dscps, asp_ip_dscp_cmd,
This should be deprecated in some way imho.
Done
File src/ss7_instance.c:
https://gerrit.osmocom.org/c/libosmo-sigtran/+/43667/comment/3c9d3198_0bc0c5... : 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 us […]
Yes, it is essential, if we want DSCP for handshake packet.
https://gerrit.osmocom.org/c/libosmo-sigtran/+/43667/comment/e0ccd007_307e43... : PS1, Line 392: void osmo_ss7_qos_class_update(struct osmo_ss7_qos_class *qos)
This can go into ss7_qos_class.c too. […]
Done
File src/ss7_qos_class_vty.c:
https://gerrit.osmocom.org/c/libosmo-sigtran/+/43667/comment/410f0763_99050d... : PS1, Line 112: if (qos->ip_dscp) {
I think is "qos" exists it should always print the "qos class" node, even if empty.
Done
https://gerrit.osmocom.org/c/libosmo-sigtran/+/43667/comment/0fab36f3_29c54b... : PS1, Line 114: if (qos->ip_dscp)
this is always true in this path?
Fixed
File src/ss7_vty.c:
https://gerrit.osmocom.org/c/libosmo-sigtran/+/43667/comment/c9392dc1_c06842... : PS1, Line 1409: /* then dump ASPs, as ASs reference them */
I think you can simplify and drop all the connectors "first, then, now, finally" ;)
Done
https://gerrit.osmocom.org/c/libosmo-sigtran/+/43667/comment/3388f33f_879765... : 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?
Done
https://gerrit.osmocom.org/c/libosmo-sigtran/+/43667/comment/bf2eb826_19e86e... : PS1, Line 1548:
remove this empty line.
Done
-
jolly