Patch Set 2: Code-Review-2 (3 comments)
https://gerrit.osmocom.org/#/c/3722/2//COMMIT_MSG Commit Message: Line 31: - 'a5 1' (writing this last) Now I understand, and I think this is a no-go. A scenario is by nature imperative, and adding a scenario shall only add more restriction, never enlarge the scope of permitted options. If we want a scenario with both a5 0 and 1, we need an actual separate scenario file saying ciphers: - a5_0 - a5_1 If we allow adding a scenario to enhance options, we will break the semantics and pretty surely end up in chaos. To change this, we'd need some 'a OR b' operator, but so far adding a scenario is strictly 'AND' implicitly. We also potentially hit problems there with the logic that we need to think about: if a scenario says [a5_0, a5_1], it implies that a BTS must support *both* a5_0 and a5_1 at the same time, not that it may support either of the two. More fitting I guess is the argument of frequency bands as in another patch: if a scenario requests multiple bands, it would imply that a BTS supports all of them at the same time. We can't let the resolver pick one of them without breaking the rest of the logic. The same argument applies to the reverse case: a BTS supports multiple bands, but a scenario picks only one of them -- this is also potentially breaking the current resolver. The places where we need this have to be set apart from the rest somehow to work safely. Compare to the modem features where all have to be present, this cannot be implicit. We need to go back to the drawing board and discuss in person I believe. Something like 'AND', 'OR' keywords maybe, if we can't find a way to avoid this . Apologies for not spotting this earlier. I'm leaving the remaining comments for illustration... https://gerrit.osmocom.org/#/c/3722/2/src/osmo_gsm_tester/config.py File src/osmo_gsm_tester/config.py: Line 249: assert type(dest[i]) == type(src[i]) this is fragile: tuple and list are both iterable and can be combined, yet their type() differs. I guess this needs to be assert is_list(a) == is_list(b) and is_dict(a) == is_dict(b) ? Line 253: dest.append(src[i]) I need to think hard here ... maybe a comment would be good. ...so... dest and src are both lists. now we see whether each item in src matches the item in dest at the same index. but if dest[i] is only a single value and src[i] ... is nowhere else in dest ... we append to dest? I can't wrap my head around it, looks like a violation of matching up indexes in the lists. Also while we're iterating, appending to the list being iterated opens up all sorts of hell. -- To view, visit https://gerrit.osmocom.org/3722 To unsubscribe, visit https://gerrit.osmocom.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: Ib7a38f10eb9de338a77bf1fa3afceb9df1532015 Gerrit-PatchSet: 2 Gerrit-Project: osmo-gsm-tester Gerrit-Branch: master Gerrit-Owner: Pau Espin Pedrol <pes...@sysmocom.de> Gerrit-Reviewer: Jenkins Builder Gerrit-Reviewer: Neels Hofmeyr <nhofm...@sysmocom.de> Gerrit-Reviewer: Pau Espin Pedrol <pes...@sysmocom.de> Gerrit-HasComments: Yes
