Hi all,
On 10/2/20 2:59 PM, Harald Welte wrote:
On Wed, Sep 23, 2020 at 03:58:31PM +0200, Neels Hofmeyr wrote:
On Wed, Sep 23, 2020 at 05:40:39PM +0700, Vadim Yanitskiy wrote:
struct cmd_node foo_node = { .usrattr = (1 << FOO_VTY_ATTR_RESTART_FULL), // (!) };
this way we could no longer see that a newly added command failed to set the correct user attribute. i.e. I add a new DEFUN, I fail to remember setting attributes, and thus it shows some attribute which is wrong. If each command gets explicit attributes, errors like that are unlikely: If I add a DEFUN and forget to set the right attribute, there will be no attribute, and a user reading the vty reference will ask us to fix, without the need to try and fail first.
we could have a checker script that issues warnings, similar to the value_string checker? We could indeed also deprecate DEFUN (make it print a warning for now) while things are migrated to those with attributes...
in general, I agree with Neels. This approach would also require us to break ABI, so osmo-* binaries compiled against an old libosmocore version might 'acquire' unexpected attributes due to mismatching size of struct cmd_node.
Deprecating DEFUN does not sound like a reasonable way to me, as we're just introducing a new feature. Imagine what compilation logs of unmaintained projects like osmocom-bb would turn into; they already contain lots of deprecation warnings :/
After a productive discussion with Philipp, we decided that it does not make sense to have FOO_VTY_ATTR_RESTART_FULL at all. It's obvious that all commands apply on full program restart, right? Instead, we took the opposite approach: let's state in the VTY reference that "all VTY commands take effect on full program restart, unless stated otherwise", and introduce a global attribute indicating that a given command applies immediately (CMD_ATTR_IMMEDIATE).
libosmocore defined attrs would be useful.
ACK.
See https://gerrit.osmocom.org/c/libosmocore/+/20309/1 and https://gerrit.osmocom.org/c/libosmocore/+/19577.
Here is a brief summary:
* if a given command applies on application specific event (e.g. OML/RSL link [re]establishment), then assign application specific attributes; * if a given command applies immediately, then assign global attribute CMD_ATTR_IMMEDIATE (defined in libosmocore); * if a given command applies on full program restart, then there is no need to assign attributes - it's implicit default.
I know that the VTY code disregards osmo_ completely, but I'd still favor prepending OSMO_* to new enum values.
I'm -as usual - in favor of consistency. So if the old code has no prefix, any additions should also not have prefixes.
ACK.
dexter wrote:
Maybe we could have some standard letters defined in libosmocore for standard situations: F = Full restart required I = Applied Immediately
ACK. We can also agree that generic (pre-defined) attributes use upper case letters, while the application specific ones use lower case?
I like that approach
This should eventually be stated in the VTY reference / documentation.
We can even add a check to libosmocore, so vty_init() would at least print a warning if an upper-case letter is used in the application specific attributes. I already submitted duplicate detection check:
https://gerrit.osmocom.org/c/libosmocore/+/20411
I would go for the other way around, as I would expect libosmocore to define fewer 'common' attributes, while applications may have more different flags. So as there are fewer numbers than lstters, I'd go for libosmocore=numbers.
Please note that in the end it's not just libosmcoore, but also libosmogb, libosmo-abis, libosmo-sccp, ...
This will be discussed in a follow-up E-mail.
Best regards, Vadim.