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/.
Pau Espin Pedrol gerrit-no-reply at lists.osmocom.orgPau Espin Pedrol has posted comments on this change. ( https://gerrit.osmocom.org/12761 ) Change subject: Add generic host config and related helpers ...................................................................... Patch Set 5: Code-Review-1 (6 comments) Without looking at next patches, I have the feeling most of these APIs are so simple they are really not needed. Having the struct would be enough. Also I don't like this way of splitting between commits where a bunch of APIs not being used anywhere are added in one commit. https://gerrit.osmocom.org/#/c/12761/5/contrib/jenkins.sh File contrib/jenkins.sh: https://gerrit.osmocom.org/#/c/12761/5/contrib/jenkins.sh@27 PS5, Line 27: export PATH="$inst/bin:$PATH" why is this line needed? https://gerrit.osmocom.org/#/c/12761/5/src/client.c File src/client.c: https://gerrit.osmocom.org/#/c/12761/5/src/client.c@34 PS5, Line 34: #define MATCH(a, b) (strcmp(a, b) != 0) ? false : true can be simplified: #define MATCH(a, b) (strcmp(a,b) == 0) which makes me think if we really need this MATCH statement... https://gerrit.osmocom.org/#/c/12761/5/src/client.c@38 PS5, Line 38: bool m_name = MATCH(match, cfg->name), m_host = MATCH(match, cfg->remote_host); separate lines please. https://gerrit.osmocom.org/#/c/12761/5/src/client.c@56 PS5, Line 56: struct host_cfg *make_config(void *ctx, const char *name, const char *host, uint16_t port) This is basically a constructor/allocator, so better name it like we usually do, like talloc_host_cfg or host_cfg_alloc. https://gerrit.osmocom.org/#/c/12761/5/src/client.c@79 PS5, Line 79: char *get_authority(void *ctx, const struct host_cfg *cfg) It's allocating something and the method is not const, so I'd better name it "create_authority" or "make_authority". https://gerrit.osmocom.org/#/c/12761/5/src/client.c@81 PS5, Line 81: if (!cfg->remote_host) Is this really needed? -- To view, visit https://gerrit.osmocom.org/12761 To unsubscribe, or for help writing mail filters, visit https://gerrit.osmocom.org/settings Gerrit-Project: osmo-sysmon Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ie321655a92cdbefbfaa056ac0d583397c83beccb Gerrit-Change-Number: 12761 Gerrit-PatchSet: 5 Gerrit-Owner: Max <msuraev at sysmocom.de> Gerrit-Reviewer: Jenkins Builder (1000002) Gerrit-Reviewer: Max <msuraev at sysmocom.de> Gerrit-Reviewer: Pau Espin Pedrol <pespin at sysmocom.de> Gerrit-Comment-Date: Fri, 01 Feb 2019 16:06:44 +0000 Gerrit-HasComments: Yes Gerrit-HasLabels: Yes -------------- next part -------------- An HTML attachment was scrubbed... URL: <http://lists.osmocom.org/pipermail/gerrit-log/attachments/20190201/59f793c0/attachment.htm>