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

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
Tue Jan 19 11:33:35 UTC 2021


neels 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 5:

(5 comments)

one important element of reading from a database is to limit the amount of entries, i.e. we wouldn't want to dump the list of all subscribers of an entire continent by accident. So I guess the db_subscr_get() should have a 'limit' argument, or alternatively the get_cb() should be able to terminate reading from the DB by returning false, for example. If the get_cb() also gets an argument indicating the row nr within the filtered result, then the get_cb() could decide to drop out after, say, 1000 entries. Thinking: vty dumping more than (say) 1000 entries should be prevented anyway to make sure osmo-hlr doesn't miss out on handling sockets in time.

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)


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 directly. So this function itself should not need to know what it is passing on to the callback function at all, and it also doesn't need to know whether the subscriber will be printed or what happens to it, i.e. there should be no mention of a 'struct vty' or 'vty' or 'print' here at all. instead, the usual pattern for this situation is to use a 'void *data' arg to be passed to the callback:

  int db_subscrs_get(struct db_context *dbc, [...],
                     void (*get_cb)(struct hlr_subscriber *subscr, void *data),
                     void *data)
  {
       ...
       get_cb(subscr, data);
  }

Then a get_cb implementation could use the vty like this:

 void subscr_print_vty(struct hlr_subscriber *subscr, void *data)
 {
     struct vty *vty = data;
     vty_out(vty, "...");
 }

 foo(struct vty *vty)
 {
     db_subscrs_get(dbc, [...], subscr_print_vty, vty);
 }

Like this the db_subscrs_get() is the most generally useful naming and parameter typing, and you happen to use it for printing to vty.


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

  sub = (struct hlr_subscriber){
       .id = sqlite3_column_int64(stmt, 0),
  }

which would both zero init and set the id member)


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 I'd suggest a simple shim function (which will most likely get optimized away by the compiler) to forward the callback:

  void print_vty_cb(struct hlr_subcsriber *subscr, void *data)
  {
         subscr_dump_summary_vty((struct vty*)data, subscr);
  }

and then

  db_subscrs_get(g_hlr->dbc, ....., print_vty_cb, vty, &count, &err);


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. think: if someone modifies the column headers and tests only with < 40 entries, and never notices that the footer names were forgotten to be updated...

Possibly that get_cb() could be passed an index number as well, and the headers could be printed before the first line and again every 40 lines?



-- 
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: 5
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 11:33:35 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Gerrit-MessageType: comment
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.osmocom.org/pipermail/gerrit-log/attachments/20210119/2b9ef0e8/attachment.htm>


More information about the gerrit-log mailing list