Hi Philipp,
while reading your comments to:
https://gerrit.osmocom.org/c/osmo-bsc/+/19670 https://gerrit.osmocom.org/c/osmo-mgw/+/20250
I realized that it would be better to discuss here.
My initial idea was that the absence of attributes (in the command definition and thus in the VTY reference) implicitly indicates that a given VTY command in the 'config' node comes into effect immediately. This way we would only need to add attributes to commands requiring program restart or reestablishment of some link(s) / connection(s).
However, in some applications *most* of the commands may require full program restart. Or the opposite: most commands may apply immediately. Adding same attributes to every command is annoying, so I came up with a concept of default 'applies when' policy: what if we add a new field to 'struct cmd_node' indicating default attributes of all commands belonging to it?
struct cmd_node foo_node = { .node = FOO_NODE, .prompt = "%s(config-net)# ", .vtysh = 1, .usrattr = (1 << FOO_VTY_ATTR_RESTART_FULL), // (!) };
This way a DEFUN() command definition would inherit foo_node.usrattr, and a DEFUN_USRATTR() command definition would override it.
dexter wrote:
I just wonder if there could be a VTY_ATTR_RESTART_FULL predefined in libosmore. Almost any of the osmo program will need it. Maybe also an VTY_ATTR_RESTART_IMMEDIATE?
ACK. I think they could be even moved to generic attributes:
/*! Attributes (flags) for \ref cmd_element */ enum { CMD_ATTR_DEPRECATED = (1 << 0), CMD_ATTR_HIDDEN = (1 << 1), CMD_ATTR_APPLY_IMMEDIATE = (1 << 2), CMD_ATTR_APPLY_FULL_RESTART = (1 << 3), };
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?
What do you think about these ideas? If you agree, and nobody has any objections, I can quickly implement them in libosmocore. Would be also nice to know what other developers think. Neels, Harald, Pau, Daniel?
Best regards, Vadim.
Hello Vadim.
My initial idea was that the absence of attributes (in the command definition and thus in the VTY reference) implicitly indicates that a given VTY command in the 'config' node comes into effect immediately...
I also think that this makes sense, because as far as I can see this predefinition would also affect interactive VTY commands. Those always have an immediate effect. Also we prefer if commands apply immediately when the implementation permits it.
struct cmd_node foo_node = { .node = FOO_NODE, .prompt = "%s(config-net)# ", .vtysh = 1, .usrattr = (1 << FOO_VTY_ATTR_RESTART_FULL), // (!) };
This way a DEFUN() command definition would inherit foo_node.usrattr, and a DEFUN_USRATTR() command definition would override it.
Makes sense I think, however, this will be more important in the future. Right now we need to look at each VTY command individually and check what changes it causes and when. I have now done the common part of osmo-bts, but for the moment I am using comments until the API is stable again.
ACK. We can also agree that generic (pre-defined) attributes use upper case letters, while the application specific ones use lower case?
Probably makes sense, there will also be never collisions should we add more predefined attributes later.
What do you think about these ideas? If you agree, and nobody has any objections, I can quickly implement them in libosmocore. Would be also nice to know what other developers think. Neels, Harald, Pau, Daniel?
I do not have any counter-arguments. I think your ideas make sense.
I still question myself if CMD_ATTR_APPLY_FULL_RESTART should be used on every CONFIGURE TERMINAL command. A full restart will always cause the changes to be applied, or is the policy only to set the attribute of the minimal condition that causes the change to apply?.
Best regards, Philipp
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.
dexter wrote:
I just wonder if there could be a VTY_ATTR_RESTART_FULL predefined in libosmore. Almost any of the osmo program will need it. Maybe also an VTY_ATTR_RESTART_IMMEDIATE?
ACK. I think they could be even moved to generic attributes:
/*! Attributes (flags) for \ref cmd_element */ enum { CMD_ATTR_DEPRECATED = (1 << 0), CMD_ATTR_HIDDEN = (1 << 1), CMD_ATTR_APPLY_IMMEDIATE = (1 << 2), CMD_ATTR_APPLY_FULL_RESTART = (1 << 3), };
libosmocore defined attrs would be useful. That would also require some _VTY_ATTR_LAST enum value so that programs can add own attributes that don't overlap.
Not so sure about these CMD attributes including the left shift. It would duplicate things, because we do need a plain integer enum to trivially hit the right attribute indexes. See also osmo_fsm state and event values.
I know that the VTY code disregards osmo_ completely, but I'd still favor prepending OSMO_* to new enum values.
{ btw, each and every osmo_fsm caller and now also each and every vty attribute defining code defines a left-shift macro like
#define S(x) (1 << (x))
it's a bit silly to repeat this definition all over the place. Added to libosmocore, it would have to be called OSMO_S()... makes everything longer. maybe nevermind, just thinking aloud. }
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?
My opinion is not strong here.
I guess the argument is that if we want to add a letter to libosmocore later, we have to check each and every osmo-* program to avoid collisions?
I think the effort to do this explicitly is minor, so we can do it like cmdline options: each program assigns its own letters completely? Then again, 'F' should mean the same thing in various osmo-* programs?
But consider, having attributes of upper case and lower case of the same letter could be somewhat confusing to users. Might as well just use numbers, like for additives on a fast food menu...
libosmocore assigns letters, applications assign numbers and special characters??
~N
Hi Neels,
On 9/23/20 8:58 PM, Neels Hofmeyr wrote:
libosmocore defined attrs would be useful. That would also require some _VTY_ATTR_LAST enum value so that programs can add own attributes that don't overlap.
it needs to be clarified that we have two kinds of attributes: global (the ones defined in libosmocore, like CMD_ATTR_DEPRECATED) and application specific. Both are stored in separate bit-masks, not together. So there can be no overlap between the flag values.
My proposal is to add CMD_ATTR_APPLY_{FULL_RESTART,IMMEDIATE} as global attributes, and still reserve 32 bits for application specific ones.
I know that the VTY code disregards osmo_ completely, but I'd still favor prepending OSMO_* to new enum values.
ACK, makes sense. We can also prepend 'OSMO_' to the existing symbols and add backwards-compatibility defines. Fortunately, only two attributes exist at the moment: CMD_ATTR_{DEPRECATED,HIDDEN}.
it's a bit silly to repeat this definition all over the place. Added to libosmocore, it would have to be called OSMO_S()... makes everything longer. maybe nevermind, just thinking aloud.
I don't think it's a big deal to add a one-liner define. AFAIR, Max tried to get such macros merged to libosmocore, but his patch was rejected.
My opinion is not strong here.
I guess the argument is that if we want to add a letter to libosmocore later, we have to check each and every osmo-* program to avoid collisions?
...
libosmocore assigns letters, applications assign numbers and special characters??
As I already mentioned, the attribute values themselves do not overlap, but if we assign flag letters to the globally defined attributes, then there is a chance that the letters would overlap.
What I also forgot to point out is that we can have up to 8 global attributes and up to 32 application specific attributes, because:
struct cmd_element { // ... unsigned char attr; /*!< Command attributes (global) */ unsigned int usrattr; /*!< Command attributes (program specific)*/ };
Given that we're not going to have more than 8 global attributes, and not going to assign anything to both CMD_ATTR_{DEPRECATED,HIDDEN}, I think reserving something like '*' and '!' would be enough.
Best regards, Vadim.
Sorry for the late response,
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...
libosmocore defined attrs would be useful.
ACK.
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.
btw, each and every osmo_fsm caller and now also each and every vty attribute defining code defines a left-shift macro like
#define S(x) (1 << (x))
it's a bit silly to repeat this definition all over the place. Added to libosmocore, it would have to be called OSMO_S()... makes everything longer. maybe nevermind, just thinking aloud. }
Yes, that is a known issue..
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
Then again, 'F' should mean the same thing in various osmo-* programs?
yes.
But consider, having attributes of upper case and lower case of the same letter could be somewhat confusing to users.
Well, given that in iptables, lowercase are matches and uppercase are targets, I guess it's clear I don't share such a concern. But maybe it is confusing to Windows users who are taught for decades that there is no distinction between upper and lower case? I'm willing to accept that there is a concern.
libosmocore assigns letters, applications assign numbers and special characters??
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, ...
Regards, Harald
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.
Hi all again,
this is a follow-up post continuing discussion on the VTY attributes. Yesterday Philipp reached me out regarding adding 'when it applies' attributes to VTY commands registered by our shared libraries, i.e. libosmo-*. The problem is that the current API does not allow to register description and flag letters for such commands.
My initial idea was to introduce a simple function that would merge a given set of library specific attributes into the application specific attributes provided by the API user (see struct vty_app_info). However, this is not a solution, because there can be multiple libraries an application dynamically links to, and there can be collisions not only between attributes defined by a library and the application itself, but what's even more problematic, between different libraries too.
+----------+ +---| LIB CORE |-----+ | +----------+ | | | | | +---x---+ +---x---+ | | LIB A | | LIB B | | +-------+ +-------+ | | | | +---x------------x---+ +---x APPLICATION | +--------------------+
This illustrates a typical situation when not only the 'APPLICATION' itself defines its own VTY commands, and thus its own VTY attributes, but also the libraries it is dynamically linked to optionally register VTY commands and the related attributes.
Given that a single VTY command may store up to 32 unique attributes (because it's a bit-mask based on unsigned int), we somehow need to ensure that there is no clashes between the attributes and their associated flag letters.
I also thought about reserving 16 LSBs of the bit-mask for attributes defined by libraries, so this way all libosmo-* would need to somehow share these reserved 16 bits, but... Given that a VTY command can either be an application's command *or* a library's command, splitting the bit-mask does not make that much sense.
After a bit of brainstorming, I came up with a proposal:
a) applications are allowed to register up to 32 unique attributes with optional flags: some symbols (to be discussed), numbers, or lowercase letters;
b) libraries get their own *global* bit-space for attributes with optional flags: some symbols (to be discussed) or uppercase letters;
c) all libraries *share* 32 unique attributes (and thus flags), because an application may be using VTY commands of several libraries at the same time; c1) in other words, if let's say libosmo-abis defines attribute (1 << 0), then none of the other libraries is allowed to redefine it;
d) all attributes belonging to libraries, together with their description and optional flag letters, to be defined in a centralized place - libosmocore (more precisely, libosmovty); d1) this way we ensure that there are no clashes: neither between the attribute values, nor between their flag letters.
A downside of this approach is that libosmovty would need to distinguish VTY commands belonging to an application from commands registered by the libraries it depends on. This way it would be possible to pick the correct attribute description (from struct vty_app_info or from libosmocore) when generating the VTY documentation. I propose:
e) to introduce one more global attribute CMD_ATTR_LIBCMD, and 'equip' all VTY commands registered by libraries with it; e1) in order to avoid mangling all existing DEFUNs, we can introduce special install_element_lib() / install_element_ve_lib() wrappers and use them.
I hope I have not forgotten anything. Looking forward to know your opinion.
Best regards, Vadim.
Hello Vadim,
I understand a,b,c,c1,d,d1 but I am a bit lost at e and e1.
In d you suggest to define the attributes in a centralized place to avoid clashes. I think this is fine. If we look at include/osmocom/vty/command.h than in enum node_type the node ids for libosmo-abis and libosmo-sccp are stored. Given that I think it would be acceptable to add the flags we need for libosmo-sccp and libosmo-abis in the enum next to CMD_ATTR_IMMEDIATE.
btw.: In libosmo-sccp there is one situation where the changes take effect when the VTY node is exited. At least this is generic enough to justify a CMD_ATTR_NODEEXIT.
best regards, Philipp
Hi Philipp,
On 10/5/20 10:17 PM, Philipp Maier wrote:
I understand a,b,c,c1,d,d1 but I am a bit lost at e and e1.
it's just an implementation detail that should not affect the process of adding attributes. I came up with the implementation, maybe it would be easier to understand looking at the code:
https://gerrit.osmocom.org/c/libosmocore/+/20440 vty/command: add CMD_ATTR_LIB_COMMAND and install() API wrappers https://gerrit.osmocom.org/c/libosmocore/+/20447 vty: introduce API for the library specific attributes
In other words, vty_dump_element() needs to know whether a given command is a library command or it belongs to an application. Thanks to CMD_ATTR_LIB_COMMAND, this function can print proper attribute description and flag letter (if present).
We don't want to equip each DEFUN() in libraries with this attribute *manually*, so that's why I came up with install_lib_element() and install_lib_element_ve(). Patches are in Gerrit.
btw.: In libosmo-sccp there is one situation where the changes take effect when the VTY node is exited. At least this is generic enough to justify a CMD_ATTR_NODEEXIT.
ACK, I'll add this one as a global attribute. I am pretty sure we can benefit from having this attribute globally some day.
Best regards, Vadim.
Hello Vadim,
it's just an implementation detail that should not affect the process of adding attributes. I came up with the implementation, maybe it would be easier to understand looking at the code:
Thanks for pointing at the code. I have read through and I think I understand it now. I have uploaded a patch on how I would add a new attribute:
https://gerrit.osmocom.org/c/libosmocore/+/20460
There is also still the open question on how we should tag commands that perform a node change, like this one here: https://gerrit.osmocom.org/c/osmo-trx/+/20319/1/CommonLibs/trx_vty.c#100
While going through all the commands of osmo-bsc, osmo-mgw, osmo-bts, osmo-trx and osmo-pcu I have always CMD_ATTR_IMMEDIATE on commands that change the VTY node, even when the command does nothing except changing the VTY node (which indeed applies immediately).
In my opinion the VTY commands that change the nodes should be excluded from the attributes, maybe by puting a CMD_ATTR_EXCLUDE on them. The reason for this is because those commands are used to walk through the VTY, they do not set any config value. This is comparable to a tree structure, where only the leafs are actually of interest.
When looking deeper into this one can find that the things are not as simple as they may seem. There are VTY commands out there that change the node and at the same time they allocate an object. Like the trunk command in osmo-mgw. What should we do with those. If the trunk already exists, then it is put in the VTY context and the node is changed, basically nothing happens. There may be commands behind the node where some apply immediately and where some do require a full restart. What kind of attribute would we assign the the trunk command? Als here I would argue to exclude it. In the end the commands perform the configuration and even if the object is allocated immediately, it still requires to get configured by the commands and here the final decision is made whether a full restart is required or not.
best regards. Philipp
On Sat, Oct 03, 2020 at 05:27:54PM +0700, Vadim Yanitskiy wrote:
[...] 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
Depends on how you read this. I thought this indicates the *minimum* requirement to get a config to take effect. If a config has the attribute "ATTR_RESTART_FULL" it means there is no way to apply this config unless i actually restart the program. From that aspect it could be more elegant to leave config items without attributes until someone looked at it and determined the minimum requirement. Otherwise we'd tag commands as "RESTART_FULL" which don't actually require a full restart.
I'm -as usual - in favor of consistency. So if the old code has no prefix, any additions should also not have prefixes.
And me as usual in favor of throwing out silly quirks that exist only "because we always did it that way". I'd favor the approach that everything in libosmocore should have the "osmo_"/"OSMO_" prefix. But I can accept that I disagree and think it's quirky and strange, but am not going to get my way there. See the recent bssmap_le patches...
~N
-
Harald Welte -
Neels Hofmeyr -
Philipp Maier -
Vadim Yanitskiy