Change in osmo-hlr[master]: Add vty command to show summary of all or filtered subscribers

[keith]

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

Change subject: Add vty command to show summary of all or filtered subscribers
......................................................................


Patch Set 6:

(7 comments)

https://gerrit.osmocom.org/c/osmo-hlr/+/22311/5/src/db_hlr.c 
File src/db_hlr.c:

https://gerrit.osmocom.org/c/osmo-hlr/+/22311/5/src/db_hlr.c@626 
PS5, Line 626: }
> (add a blank line)
Done


https://gerrit.osmocom.org/c/osmo-hlr/+/22311/5/src/db_hlr.c@638 
PS5, Line 638: int db_subscrs_get_print(struct vty *vty, struct db_context *dbc, const char *filter_type, const char *filter,
> Good solution: in fact this uses a callback function -- which is more elegant than using the vty dir […]
Done


https://gerrit.osmocom.org/c/osmo-hlr/+/22311/5/src/db_hlr.c@644 
PS5, Line 644: subscr
> Why do you need this pointer? I think you can use 'sub' directly.
The code I modelled this on had a pointer to a hlr_subscriber struct passed in, so I was mimicking that. 
I think it might be OK now, but I'm never 100% sure.


https://gerrit.osmocom.org/c/osmo-hlr/+/22311/5/src/db_hlr.c@689 
PS5, Line 689: 		subscr->id = sqlite3_column_int64(stmt, 0);
> (along with fixeria's comment: here you could do […]
Does it zero init the entire struct? I sort of depend on that later


https://gerrit.osmocom.org/c/osmo-hlr/+/22311/5/src/hlr_vty_subscr.c 
File src/hlr_vty_subscr.c:

https://gerrit.osmocom.org/c/osmo-hlr/+/22311/5/src/hlr_vty_subscr.c@229 
PS5, Line 229: 	rc = db_subscrs_get_print(vty, g_hlr->dbc, filter_type, filter, subscr_dump_summary_vty, &count, &err);
> to call the pre-existing subscr_dump_summary_vty(): to match the 'void *data' as I described before  […]
No objection, but can I ask why, just to understand?
Is it better to cast data to a vty struct this way rather than the declaration in:

subscr_dump_summary_vty(struct hlr_subscriber *subscr, void *data)
{
	struct vty *vty = data;
	.....
}


https://gerrit.osmocom.org/c/osmo-hlr/+/22311/5/src/hlr_vty_subscr.c@230 
PS5, Line 230: 	if (count > 40) {
> hm, this code dup is not so nice. […]
Yep... I was doubting it also, but given that the patch set was unlikely to attract +2 on first run, I left it for comments. 40 was just an arbitrary number - of course we have no idea what size the terminal is.
All the same, if somebody modifies the header without modifying the footer, then they get a slap on the wrist...?

I could just drop the footer, it's not like it's not obvious which column is which. 
I don't like reprinting the header, in can get in the way if one did want to copy rows from the terminal or screen scrape the vty (not suggesting I wanty to do this)


https://gerrit.osmocom.org/c/osmo-hlr/+/22311/5/tests/test_nodes.vty 
File tests/test_nodes.vty:

https://gerrit.osmocom.org/c/osmo-hlr/+/22311/5/tests/test_nodes.vty@17 
PS5, Line 17:  show subscribers filter (imsi|msisdn) FILTER
            :   show subscribers filter (cs|ps) (on|off)
> I would actually go for just "show subscribers (imsi|msisdn) FILTER" without the "filter' part. […]
Done



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

Gerrit-Project: osmo-hlr
Gerrit-Branch: master
Gerrit-Change-Id: I7f0573381a6d0d13841ac6d42d50f0e8389decf4
Gerrit-Change-Number: 22311
Gerrit-PatchSet: 6
Gerrit-Owner: keith <keith at rhizomatica.org>
Gerrit-Reviewer: Jenkins Builder
Gerrit-CC: fixeria <vyanitskiy at sysmocom.de>
Gerrit-CC: laforge <laforge at osmocom.org>
Gerrit-CC: neels <nhofmeyr at sysmocom.de>
Gerrit-Comment-Date: Tue, 19 Jan 2021 18:20:17 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Comment-In-Reply-To: neels <nhofmeyr at sysmocom.de>
Comment-In-Reply-To: laforge <laforge at osmocom.org>
Comment-In-Reply-To: fixeria <vyanitskiy at sysmocom.de>
Gerrit-MessageType: comment
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.osmocom.org/pipermail/gerrit-log/attachments/20210119/6c020d25/attachment.htm>