Hi all,
we need to wrap up the DGSM work and get it to a state that can be merged to master.
There are still open issues that I am not sure how to solve, which I've mentioned on some occasions, but it seems to not have been loud enough. I would easily choose one way, but am not sure about others' opinions.
(1) One open point is the GSUP peer identification. I've added a comment explaining it in https://gerrit.osmocom.org/c/osmo-hlr/+/16459/9
Me personally, I would strip down basically all of that complexity again and go with the simplest solution, a nul terminated size limited char string for GSUP peer id. The patch became what it is because vague requirements were thrown in the mix and I tried to accomodate them, and now it ended up being a rather ugly shim around a simple char string, really.
(2) Another open question is the freeing behavior in osmo_gsup_req (for proper async handling of DGSM, and to ensure proper GSUP responses). I've added a comment explaining that in https://gerrit.osmocom.org/c/osmo-hlr/+/16205/29
The gist for both issues is that I could write patches that would have a large ripple effect throug many files and follow-up patches, but if we again disagree on the outcome, the work would multiply.
So, DGSM works and is ready, except that we need to agree on what will be accepted by review. I need opinions to be able to complete this (or possibly a "go" to do whatever I think is right and merge that).
Thanks!
~N
Hi Neels,
On Tue, Apr 28, 2020 at 05:17:04PM +0200, Neels Hofmeyr wrote:
we need to wrap up the DGSM work and get it to a state that can be merged to master.
Indeed.
(1) One open point is the GSUP peer identification. I've added a comment explaining it in https://gerrit.osmocom.org/c/osmo-hlr/+/16459/9
Me personally, I would strip down basically all of that complexity again and go with the simplest solution, a nul terminated size limited char string for GSUP peer id. The patch became what it is because vague requirements were thrown in the mix and I tried to accomodate them, and now it ended up being a rather ugly shim around a simple char string, really.
I defer to your judgement.
(2) Another open question is the freeing behavior in osmo_gsup_req (for proper async handling of DGSM, and to ensure proper GSUP responses). I've added a comment explaining that in https://gerrit.osmocom.org/c/osmo-hlr/+/16205/29
No strong opinion either way, slight preference towards the way the patch currently is.
Let's see if somebody has strong opinions otherwise.
On 28/04/2020 10:17, Neels Hofmeyr wrote:
There are still open issues that I am not sure how to solve, which I've mentioned on some occasions, but it seems to not have been loud enough. I would easily choose one way, but am not sure about others' opinions.
I guess this falls somewhat into my lap and I have to admit to not having looked at it enough.
I don't think it is that you are not loud enough. It's just this is still way way ahead of the speeds things seem to be able to move at the the world of "mañana..."
I can't even respond to your specific questions. (1) and (2). All I could say, for my part, at this point would be "go" with your way and then "somebody" will have to deal with any fallout later from not being up to speed with it now.
So, DGSM works and is ready, except that we need to agree on what will be accepted by review. I need opinions to be able to complete this (or possibly a "go" to do whatever I think is right and merge that).
So, if it is "just" questions which are to do with the tech of the implementation, then I guess my opinion is not very relevant.
k
On Tue, Apr 28, 2020 at 05:17:04PM +0200, Neels Hofmeyr wrote:
we need to wrap up the DGSM work and get it to a state that can be merged to master.
My latest summary on this is:
- keith had no time to look at this, says to go ahead and deal with the merged situation later.
- pespin would prefer an opaque data structure for peer id, for ABI compat reasons.
- I'm reluctant to add opaque data because that requires dynamic allocation. Concerning future ABI compatibility, I agree that it would be a nice thing but is not a hard requirement. IMHO it isn't worth adding dynamic allocation (and rewriting many patches on this branch) for it.
- laforge indicates slight preference of the patch staying as it is now.
- I would actually prefer a plain char string (the current real world usage). My gut feel is that the peer id will remain API bloat forever, but it gives the benefit of the doubt to being able to expand the API compatibly in the future.
Seems like everyone is pointing in a slightly different direction. But I think most of our concerns are rather scratching the surface rather than being substantial deep review on the core implementations of D-GSM.
In the last patch set, I only renamed osmo_gsup_peer_id to osmo_cni_peer_id, added some API doc and tweaked some commit log messages, added a const.
So, essentially, these patches are still the same that they have been for months
I've probed the patches for desirable changes for a while and seem to hit reasons to not change anything in every direction. So we either find agreement on how to change these patches, or we merge them as they are. I think there is agreement that we don't want to keep this on a branch for much longer, so I'd ask reviewers to converge on a verdict, ideally one that says CR+2...
On the level of maturity: osmith and I have tested the D-GSM setup a lot, and the substantial code changes in osmo-hlr (new LU FSM and osmo_gsup_req and peer id) have already been running at 36c3 in Leipzig without issues.
~N
Hi Neels,
On Thu, Apr 30, 2020 at 07:43:38PM +0200, Neels Hofmeyr wrote:
Seems like everyone is pointing in a slightly different direction. But I think most of our concerns are rather scratching the surface rather than being substantial deep review on the core implementations of D-GSM.
Agreed. I've now merged the entire patch series as it is so far, we cannot delay this another couple of months. This obviously doesn't meant that the code cannot change from how it is now. Anyone is welcome to improve it in any way.
I know this may not make everyone happy, but we had to do something to unblock the situation. I value the 5 different lines of thought, but it seemed we were unable to converge on one particular direction. I could have taken either of those directions (no real preference either way), but going for the patches as they are now obviously means we can move ahead without additional work now.
In hindsight I have the feeling that this development was kept in a private branch for too long before being submitted to gerrit for review, until a point it was very far developed and complex. If I'm right with that feeling, I would strongly encourage a different approach in future development projects.
Regards, Harald
-
Harald Welte -
Keith -
Neels Hofmeyr