Change in osmo-bsc[master]: paging: Add VTY options to calculate T3113 timeout dynamically

[Neels Hofmeyr]

Neels Hofmeyr has posted comments on this change. ( https://gerrit.osmocom.org/11654 )

Change subject: paging: Add VTY options to calculate T3113 timeout dynamically
......................................................................


Patch Set 4: Code-Review+1

(2 comments)

hmm, if stsp also agrees, maybe I should insist on my hunch that a generic T1234 dynamic setting will not make sense anyway. But I don't want to block this more than necessary.

Arguing for my earlier points... just a +1 from me for now because I think they are valid points.

https://gerrit.osmocom.org/#/c/11654/3/src/osmo-bsc/bsc_vty.c
File src/osmo-bsc/bsc_vty.c:

https://gerrit.osmocom.org/#/c/11654/3/src/osmo-bsc/bsc_vty.c@3916
PS3, Line 3916: 	"T-number, optionally preceded by 't' or 'T'.")
> I didn't completely get what you mean here regarding docs, but anyway that sounds confusing to me wh […]
Point 1 is, we often have #defines to re-use the exact same string again, because only one doc string survives in the online docs.

  DEFUN("foo one", "Foo to set one\n" "One arg")
  DEFUN("foo two", "Foo to set two\n" "Two")

  > list
  foo one
  foo two
  > foo?
  Foo to set one
  > foo ?
  one  One arg
  two  Two

So in that case, you want a FOO_STR that is always the same.

But when there is a 'no' before that, that's not an issue.
This example shows that, and also illustrates the other point:

  #define FOO_STR "Set a foo value\n"
  DEFUN("foo one", FOO_STR "One arg")
  DEFUN("no foo", NO_STR FOO_STR)

  > list
    foo one
    no foo
  > foo?
  Set a foo value
  > no?
  Negate a command or set its defaults
  > no ?
    foo  Set a foo value
  > no foo?
  Set a foo value

"no foo" now says that it sets a value. For me it makes more sense to describe what "no foo" would actually do:

  DEFUN("no foo", NO_STR "Disable foo")

  > no?
  Negate a command or set its defaults
  > no ?
    foo  Disable foo
  > no foo?
  Disable foo

So

  > no timer-dynamic?
  Calculate T3113 dynamically based on channel config and load

that's "WTF" to me.


https://gerrit.osmocom.org/#/c/11654/3/src/osmo-bsc/bsc_vty.c@3927
PS3, Line 3927: 		return CMD_WARNING;
> Sure, but anyway nowadays doesn't make sense, to let's better tell the user probably he's doing some […]
(still think it makes sense to say "has no dynamic" when the user tries to set a dynamic value; if you really need to say something here, maybe rather "T%d already is non-dynamic"?)



-- 
To view, visit https://gerrit.osmocom.org/11654
To unsubscribe, or for help writing mail filters, visit https://gerrit.osmocom.org/settings

Gerrit-Project: osmo-bsc
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I4fb2969b690151415038631fb6ad059aa6835c7f
Gerrit-Change-Number: 11654
Gerrit-PatchSet: 4
Gerrit-Owner: Pau Espin Pedrol <pespin at sysmocom.de>
Gerrit-Reviewer: Harald Welte <laforge at gnumonks.org>
Gerrit-Reviewer: Jenkins Builder (1000002)
Gerrit-Reviewer: Neels Hofmeyr <nhofmeyr at sysmocom.de>
Gerrit-Reviewer: Pau Espin Pedrol <pespin at sysmocom.de>
Gerrit-Reviewer: Stefan Sperling <ssperling at sysmocom.de>
Gerrit-Comment-Date: Tue, 27 Nov 2018 01:33:55 +0000
Gerrit-HasComments: Yes
Gerrit-HasLabels: Yes
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.osmocom.org/pipermail/gerrit-log/attachments/20181127/e3d8424e/attachment.htm>