28 Apr
2020
28 Apr
'20
5:17 p.m.
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
28 Apr
28 Apr
7 p.m.
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.
--
- Harald Welte <laforge(a)osmocom.org> http://laforge.gnumonks.org/
============================================================================
"Privacy in residential applications is a desirable marketing option."
(ETSI EN 300 175-7 Ch. A6)
9:25 p.m.
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
30 Apr
30 Apr
7:43 p.m.
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
1 May
1 May
4:54 p.m.
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 <laforge(a)osmocom.org> http://laforge.gnumonks.org/
============================================================================
"Privacy in residential applications is a desirable marketing option."
(ETSI EN 300 175-7 Ch. A6)
1461
days inactive
1464
days old
4 comments
3 participants
participants (3)
-
Harald Welte -
Keith -
Neels Hofmeyr