Application specific VTY attributes

[Vadim Yanitskiy]

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.

-- 
- Vadim Yanitskiy <vyanitskiy at sysmocom.de>    http://www.sysmocom.de/
=======================================================================
* sysmocom - systems for mobile communications GmbH
* Alt-Moabit 93
* 10559 Berlin, Germany
* Sitz / Registered office: Berlin, HRB 134158 B
* Geschaeftsfuehrer / Managing Director: Harald Welte