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
Thu Jan 9 14:23:52 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 23:

(1 comment)

@pespin, you keep saying that separating is easy, and I keep trying to explain how it isn't: this patch has tight interdependence between the new items. You are welcome to try to separate. I have spent hours and days separating patches already, which did make sense in many places. I understand the point very much. But it is still necessary to review the combined result, and it makes little sense to spend more time on creating intermediate versions of osmo-hlr that will never be used. I know the review is harder than it should be, but separating would probably take longer than reviewing as-is. does that make sense?

https://gerrit.osmocom.org/c/osmo-hlr/+/16205/23/include/osmocom/gsupclient/ipa_name.h 
File include/osmocom/gsupclient/ipa_name.h:

https://gerrit.osmocom.org/c/osmo-hlr/+/16205/23/include/osmocom/gsupclient/ipa_name.h@29 
PS23, Line 29: struct osmo_ipa_name {
> I'm not really liking this kind of adhoc structures to keep a string specially since they are presen […]
the IPA name is a problem child, it is not entirely clear whether it is a string or arbitrary bytes with a length.
The discussion was that this should also support global titles (that could contain zero bytes),
and I assumed that we need to make all code paths treat it as binary data.
However, many code paths expect it to be nul terminated, especially the hlr.db stores as VARCHAR.
And then in code review quite recently, the request was to add a union that allows adding a global title separately, which I did (in the next patch "2/2" after this one).

In retrospect it might make sense to snap ipa_name back to a plain char[] that is always nul terminated?
Drop the separate len indicator? I think we should resolve this question before merging, feels like it indeed doesn't make sense like this.



-- 
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: 23
Gerrit-Owner: neels <nhofmeyr at sysmocom.de>
Gerrit-Reviewer: Jenkins Builder
Gerrit-Reviewer: laforge <laforge at osmocom.org>
Gerrit-Reviewer: neels <nhofmeyr at sysmocom.de>
Gerrit-Reviewer: osmith <osmith at sysmocom.de>
Gerrit-CC: pespin <pespin at sysmocom.de>
Gerrit-Comment-Date: Thu, 09 Jan 2020 14:23:52 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Comment-In-Reply-To: pespin <pespin at sysmocom.de>
Gerrit-MessageType: comment
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.osmocom.org/pipermail/gerrit-log/attachments/20200109/d7cbc0aa/attachment.htm>


More information about the gerrit-log mailing list