Skip to content

Conversation

@rhaberkorn
Copy link
Contributor

Here are some assorted commits, mostly for getting mand to compile on Yocto 2.5.
The daemonization sequence is also improved and one serious bug regarding config serialization has also been fixed.

rhaberkorn added 30 commits June 2, 2018 12:15
 * the check in dm_walk_table_cb() is unnecessary since the
   arguments are declared as non-null.
 * the signature of +sessionTimeoutEvent() was wrong
 * the former is available on both 5.2, as well as 5.3
 * there are actually two incompatible versions around:
   the Perl-based one we we're assuming and an implementation from util-linux.
 * instead of trying to check for the correct behaviour, we simply rename files
   using a POSIX shell loop now.
 * The Perl rename is also apparently not available on Yocto
 * one purpose of daemonizing is to "signal" a startup system like
   systemd that the service has initialized all of its resources.
   (If you don't want to add dependencies on DBUS or Systemd libs)
 * e.g. a system service of mand should not become active before the
   dmconfig socket has been initialized, since otherwise systemd services
   depending on mand (usually dmconfig clients) might fail to open
   a dmconfig connection, resulting in unnecessary restarts.
 * NOTE: When daemonizing, mand returns exit code 2 as it uses daemon()

A systemd job for mand could/should thus contain the following service-statements:

Type=forking
GuessMainPID=yes
ExecStart=/sbin/mand -d
SuccessExitStatus=2
* duplicate `const` identifiers are avoided (they are pointless anyway)
* every case-statement without a `break` needs a `/* fallthrough */`
  comment because of -Wimplicit-fallthrough.
This adds support for empty selectors in dm_set_notify_by_selector_recursive().
It may actually be better to extend dm_get_element_ref().
* For instance, "dmctrl set", "dmctrl get" etc. all crashed.
* This was not critical since these commands are not supposed to do anything but fail
  without arguments.
  They will now fail more gracefully.
* Perhaps, there should be dedicated error messages when detecting missing parameters (TODO).
* some parameters were set as T_UINT while they are in reality T_UINT64
* Has been broken in 5ff31d9 when
  I added support for 64-bit integers in Yang schemes.
* It does not make sense IMHO to notify everybody.
  These functions are only used in callbacks/actions (see dm_dmconfig.c).
  * When a dmconfig client lists/gets a parameter with a callback, it would
    also be notified - this is unnecessary since it gets all the information
    in the LIST or GET.
  * Other dmconfig clients would spuriously receive notifications only because
    somebody else listed these parameters.
    On the other hand, they cannot count on timely notifications.
  So disabling all notifications does not bring any disadvantages.
  If you want to be informed about changes in passively-updated parameters
  like in interfaces-state, you have to poll them regularily.
* Python 2 is deprecated and no longer (easily) supported in Yocto hardknott.
  There is little sense in supporting both Python 2 and 3.
* OpenCPE.py was ported using the `modernize` package.
* fixed some return type mismatches
* chdir() and asprintf() return values should be checked
* avoid multiple definitions of notify_attr
* one pointless if-statement in dm_deserialize.c has been
  commented out since I am not sure about its purpose
…ted strings

* this only resulted in warnings (and therefore build errors) when building
  with optimizations (ie. when functions are inlined).
* This affects e.g. libdmconfig set/get requests (or dmctrl set/get) with invalid paths.
  It's useful to detect mistakes in selector paths.
…when a "-state" role (config agent implementing the rpc_interface_state) is missing
* The sync wrappers around server-to-client requests recurse the event loop.
  Since the SAVE request will include fetching the interfaces state
  (calls rpc_get_interface_state()), the SAVE request could be in certain corner cases
  executed again, resulting in a deadlock because of the non-recursive mutex lock.
* This is only a temporary workaround.
  The event loop recursion itself is broken by design.
  For most server-to-client requests, it could be easily avoided.
  Avoiding event loop recusion in rpc_get_interface_state() would require significant
  refactoring, though.
  It will be easier to reintegrate the RPC implementation (e.g. from mand-metropolisd)
  back into mand - the implementation is OS agnostic, so there is no real reason to have
  this as a RPC.
…een refactored while update_interface_state() directly uses libnl now

* ev_loop recursion is unsafe and caused crashes
* the firmware_download, firmware_commit and set_boot_order requests will now use the main ev_run()
* Unfortunately, it would have been a lot of refactoring to keep the CMD_CLIENT_GET_INTERFACE_STATE
  while avoiding ev_loop recursion.
  update_interface_state() therefore no longer calls a server-to-client request but uses libnl directly to
  retrieve all necessary data.
  It is assumed that there is no need to actually customize the interface-state retrieval.
  CMD_CLIENT_GET_INTERFACE_STATE has been removed.
…erialized with

* This used to be the SVN revision or was a manually managed constant tied to the
  mand sources.
* This is actually a necessity for using Lua upgrade scripts as the CFG_VERSION was hardcoded to 1.
  This no longer made any sense since the data models are managed outside of the mand source tree.
* Theoretically, every Yang scheme could have its own version, but that would have to be stored
  somewhere. A single revision is still practically sufficient since while it cannot have a globally
  significant meaning, it can be managed across all firmware variants/products of a given Linux
  distribution.
* The Lua scripts from lua/ are no longer installed since they still refer to the old TR069 data model
  and Travelping-specific products. The files are kept for reference (for the time being).
* Useful improvments to the Lua function hooks (TODO):
  * The current/new versions could be stored into global variables instead of passing "arguments" to the
    script.
  * There should be an arbitrary number of scripts for "event", so you can install scripts on a per-feature
    basis instead of matching against firmware names and the like.
    This cannot easily be emulated in Lua since we would need a way of listing directories (not part of the
    C or Lua runtimes).
* dm.list() still only supports the old-school non-recursive API.
  This may have to be changed but for the time being, it's sufficient for
  what we do in fncPostVersionCheck.lua.
* There are some remnants of "configure" sessions in the Lua interface.
  I don't know what their initial motivation was but they can probably
  be removed altogether.
* Objects were passed into dm_enqueue(), apparently with the intention
  that it takes ownership of the object.
  dm_enqueue() only reparented DM2_REQUEST::packet though.
* Effectively, we leaked a few bytes for every dmconfig request sent.
…g the currently active gateways

* especially useful when DHCP-assigned
* this analyzes all default routes
* In the future, we might want to support the configuration and querying of all routes.
  There probably is an existing IETF model for that.
* This allows fncStartup.lua to access the database (eg. to
  generate parts of the base config) and also
  makes sure it can make use of already loaded base config.
…="escaped"`) across block boundaries

* string_unescape() assumed that all octal escapes are fully contained in the data it gets passed in.
  This however is not guaranteed since the character data handler (charElement()) could be invoked with
  any number of bytes by the SAX parser.
  If it gets passed in e.g. a single slash at the end of the string and the remainig 3 bytes only
  in the next invocation, the slash would be lost and the 3 octal digits would be deserialized verbatim.
* Caused real-life data corruptions during repeated serializations and reboots.
* The state necessary by string_unescape() is now part of struct XMLstate.
It was reading two unrelated bytes from the beginning of
struct sockaddr and in turn was cutting off the last two
bytes of the actual MAC address.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant