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