Change in osmo-hlr[master]: 1/2: refactor: add and use lu_fsm, osmo_gsup_req, osmo_ipa_name

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 gerrit-no-reply at lists.osmocom.org
Wed Jan 29 11:44:16 UTC 2020


neels has posted comments on this change. ( https://gerrit.osmocom.org/c/osmo-hlr/+/16205 )

Change subject: 1/2: refactor: add and use lu_fsm, osmo_gsup_req, osmo_ipa_name
......................................................................


Patch Set 26:

(meh, the new gerrit UI keeps making me miss comments. I don't get it, some comments are plain visible, others are greyed out in a single line and look like jenkins cruft.)

> 1- I didn't say it was easy, yet is something really desirable.
> 3- It's not only about reviewing for merge. It's a problem about discussing each change.
> It's a problem when reading the git log.
> It's a problem when bisecting.
> It's a problem when understanding why and how stuff was changed, and the implications it had.
> It's a problem when reverting.

I still agree completely. We don't need to debate the above points at all.

Instead we need to discuss the practical tradeoff, which is quite debatable. At least for sponsored work, a large part of the question is: How much budget do you want to spend on it and what is the actual practical positive impact you expect from it?

So let's say we do spend another week on separating this patch into three different ones. Then each of this infrastructure implemented on its own will need other (probably weird/ad hoc) inventions to make the intermediate code work. Instead of one large change that makes sense, you get three changes that each take a large step into weird no-mans-land of ad hoc solutions. I doubt that this would help during bisecting, likely some bugs appear in-between the patches to be fixed again right away, and reviewers / log readers need to understand ad-hoc shims just to see them being removed again right in the next patch. That's what I saw coming out of separating, so I stopped that. I really mean that you should try to separate it if you have the time, then it will become obvious how it is not trivial.

This seems to me a discussion whether we want large changes at all. It is not possible to trivially change osmo-hlr to achieve a big change like this.

BTW, we have used this code at 36c3 in december. OsmoHLR hasn't made any problems.

> 2- I agree with keeping ipa-name as a null-terminated string.

I started out thinking that we would use this struct for all kinds of identification, i.e. also as Global Title buffer, i.e. that we need to regard it as opaque blob of data. The code now seems to go a different direction, so I guess I don't know which way to take this. I don't want to pass it as an allocated string to avoid ownership. But keeping a separate length seems unnecessary... We did do a bunch of work previously to handle it as opaque blob, explicitly allowing both with nul and without nul; now I could use some help deciding what it should actually be...


-- 
To view, visit https://gerrit.osmocom.org/c/osmo-hlr/+/16205
To unsubscribe, or for help writing mail filters, visit https://gerrit.osmocom.org/settings

Gerrit-Project: osmo-hlr
Gerrit-Branch: master
Gerrit-Change-Id: I3a8dff3d4a1cbe10d6ab08257a0138d6b2a082d9
Gerrit-Change-Number: 16205
Gerrit-PatchSet: 26
Gerrit-Owner: neels <nhofmeyr at sysmocom.de>
Gerrit-Reviewer: Jenkins Builder
Gerrit-Reviewer: fixeria <axilirator at gmail.com>
Gerrit-Reviewer: laforge <laforge at osmocom.org>
Gerrit-Reviewer: neels <nhofmeyr at sysmocom.de>
Gerrit-Reviewer: osmith <osmith at sysmocom.de>
Gerrit-Reviewer: pespin <pespin at sysmocom.de>
Gerrit-Comment-Date: Wed, 29 Jan 2020 11:44:16 +0000
Gerrit-HasComments: No
Gerrit-Has-Labels: No
Gerrit-MessageType: comment
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.osmocom.org/pipermail/gerrit-log/attachments/20200129/7626bfc9/attachment.htm>


More information about the gerrit-log mailing list