This is merely a historical archive of years 2008-2021, before the migration to mailman3.
A maintained and still updated list archive can be found at https://lists.osmocom.org/hyperkitty/list/gerrit-log@lists.osmocom.org/.
Neels Hofmeyr gerrit-no-reply at lists.osmocom.orgNeels Hofmeyr has posted comments on this change. ( https://gerrit.osmocom.org/12446 )
Change subject: add BSC/MSC neighbor VTY commands for inter-MSC HO
......................................................................
Patch Set 2: Code-Review-1
(13 comments)
meta: Thanks for preparing so far! I can also take this patch over from you, if you like.
There are a lot of copy-paste errors in the vty code, needs a vty transcript test to uncover all.
https://gerrit.osmocom.org/#/c/12446/2/include/osmocom/msc/neighbor_ident.h
File include/osmocom/msc/neighbor_ident.h:
https://gerrit.osmocom.org/#/c/12446/2/include/osmocom/msc/neighbor_ident.h@26
PS2, Line 26: } a;
a as in "A-interface"??
I learned recently that one can actually have unnamed unions...
https://gerrit.osmocom.org/#/c/12446/2/src/libmsc/neighbor_ident.c
File src/libmsc/neighbor_ident.c:
https://gerrit.osmocom.org/#/c/12446/2/src/libmsc/neighbor_ident.c@35
PS2, Line 35: #define NEIGHBOR_IDENT_ADDR_STRING_MAX 64
hmm, if we have a maximum length for this, then rather use a fixed-size char[] array in neighbor_ident_addr.
It also clarifies ownership and memory management.
I was going to approve talloc strings there to provide arbitrary length names, but if we limit it then let's array.
https://gerrit.osmocom.org/#/c/12446/2/src/libmsc/neighbor_ident.c@39
PS2, Line 39: static char buf[NEIGHBOR_IDENT_ADDR_STRING_MAX + 4];
+4 for "BSC ". +1 for '\0'?
https://gerrit.osmocom.org/#/c/12446/2/src/libmsc/neighbor_ident.c@106
PS2, Line 106: * replace an existing entry, first call neighbor_ident_del(nil, cell_id).
neighbor_ident_del(nil, addr)
https://gerrit.osmocom.org/#/c/12446/2/src/libmsc/neighbor_ident.c@109
PS2, Line 109: * return -ENOMEM when the list is not initialized, -ERANGE when the BSIC value is too large. */
BSIC??
https://gerrit.osmocom.org/#/c/12446/2/src/libmsc/neighbor_ident.c@159
PS2, Line 159: struct gsm0808_cell_id *cell_id)
It should be possible to detect multiple matches.
Suggest arg match_idx as in gsm_bts_by_cell_id() in osmo-bsc/src/osmo-bsc/gsm_data.c.
(I now think we should also add an exact_match argument as well; that would also have to be added to gsm0808_cell_id_matches_list())
https://gerrit.osmocom.org/#/c/12446/2/src/libmsc/neighbor_ident_vty.c
File src/libmsc/neighbor_ident_vty.c:
https://gerrit.osmocom.org/#/c/12446/2/src/libmsc/neighbor_ident_vty.c@39
PS2, Line 39: #define NEIGHBOR_DOC "Manage neighbor BSS cells\n"
"Manage local- and remote-MSC BSS cells for inter-BSC and inter-MSC handover\n"
https://gerrit.osmocom.org/#/c/12446/2/src/libmsc/neighbor_ident_vty.c@134
PS2, Line 134: " "
no " " in DOC part, the docs are separated by '\n'; same N times below
https://gerrit.osmocom.org/#/c/12446/2/src/libmsc/neighbor_ident_vty.c@140
PS2, Line 140: vty_out(vty, "Could not parse point code '%s'%s", argv[0], VTY_NEWLINE);
wrong argv[idx] for point code arg.
preferably use a local const char *pc_str = &argv[1];
same N times below
https://gerrit.osmocom.org/#/c/12446/2/src/libmsc/neighbor_ident_vty.c@146
PS2, Line 146: return add_neighbor(vty, &addr, neighbor_ident_vty_parse_lac(vty, argv + 1));
+ 1 seems wrong; same N times below
https://gerrit.osmocom.org/#/c/12446/2/src/libmsc/neighbor_ident_vty.c@147
PS2, Line 147: }
Please add a .vty transcript test to verify that all of these commands work as expected.
See for example osmo-bsc/tests/neighbor_ident.vty
https://gerrit.osmocom.org/#/c/12446/2/src/libmsc/neighbor_ident_vty.c@261
PS2, Line 261: SHOW_STR "Delete a neighbor MSC\n" "MSC ipa-nam\n"
SHOW_STR???
https://gerrit.osmocom.org/#/c/12446/2/src/libmsc/neighbor_ident_vty.c@373
PS2, Line 373: SHOW_STR "Display information about a neighbor MSC\n" "MSC ipa-name\n"
No, you can't overload the same command token with two differing doc strings (above, the same one said "BSC").
Only one of them will be visible on the VTY interface.
Instead you need to "Display information about a neighbor MSC or BSS\n" and make the distinction only with the actually differing cmd token ('msc-*' or 'bsc-*').
Define one single NEIGHBOR_SHOW_DOC and use in both instances.
--
To view, visit https://gerrit.osmocom.org/12446
To unsubscribe, or for help writing mail filters, visit https://gerrit.osmocom.org/settings
Gerrit-Project: osmo-msc
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ia0dd08b087bfd4aa22e234917669d003150a4cd4
Gerrit-Change-Number: 12446
Gerrit-PatchSet: 2
Gerrit-Owner: Stefan Sperling <stsp at stsp.name>
Gerrit-Reviewer: Jenkins Builder (1000002)
Gerrit-Reviewer: Neels Hofmeyr <nhofmeyr at sysmocom.de>
Gerrit-Reviewer: Stefan Sperling <stsp at stsp.name>
Gerrit-Comment-Date: Mon, 14 Jan 2019 20:24:33 +0000
Gerrit-HasComments: Yes
Gerrit-HasLabels: Yes
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.osmocom.org/pipermail/gerrit-log/attachments/20190114/d7434a57/attachment.htm>