Extended viewer interface

[pmai]

This PR extends the viewer plugin interface, while fixing some bugs and problems, to generalize the interface for arbitrary formats, and extend plugin functionality for enhanced in GUI features for new formats.

• Fix life-cycle bugs in calls to CloseViewer
• Fix cursor handling during StartViewer calls with errors
• Remove XODR/XOSC-specific wording in UI and comments
• Let plugins handle decisions of which input files they support
• Let plugins handle decisions of which issues they can locate
• Allow for auto-start of viewers, with support for multiple viewers being active for different input file (formats)
• Implement auto-start in main GUI
• Properly support offset-based FileLocations
• Allow plugins to convert file content to file view text and map issue locations to points in the file view

[MatteoRagni]

I'm late to the party, since I have not reviewed the overall code. But who guarantees they are not null?

[pmai]

This follows the existing pattern in the code base, which seems built on the assumption that the caller (i.e. the user of the plugin) is responsible to not pass in null pointers. While I don't necessarily agree with leaving out defensive code checks here, I try to keep general renovation of the code base separate from the unrelated API changes to ease review.

I think going forward the esmini_viewer (and viewer_example code bases) could do with some renovations, but would propose to keep this to a seprate PR.

[MatteoRagni]

Again, late in the party, but there is something that is missing in the implementation of the interface, and is a container for the state or the context of the viewer implementation. This is a plugin so it is responsability of plugin caller (who calls Initialize) to keep an handle pointer (Bridge pattern) with the context and to call a appropriate Finalize method that frees the memory. I'm seeing from here that we are holding the context inside the memory space of the shared object and it is not fantastic:

1. We are forcing plugin implementer in Singleton (anti-)pattern (no multiple use)
2. No guarantees on thread safety can be provided and sync is on plugin developer (and we cannot trust a plugin with application stability)
3. No actual lifecycle control (it is abi-implementation specific)

There are several exploit that can developed based upon this premise.

I suggest to have Initialize to provide in output a hande pointer and to add a Finalize function that handle the desctruction of resources in the context. All other methods will take the plugin context as first parameter. Lifecycle is on plugin user, state is segregated and we can avoid race conditions.

Sample:

typedef void * viewer_plugin_t;

// LifeCycle
VIEWER viewer_plugin_t* Initialize(const char *inputPath);
VIEWER void Finalize(viewer_plugin_t * ctx);
// API
VIEWER bool CanSupportFormat(viewer_plugin_t * ctx, const char * inputPath);
//etc..

bool on initialize becomes a check on NULL for context.

in plugin implementation (pseudocode in C++)

extern "C" {
viewer_plugin_t* Initialize(const char *inputPath) {
  try {
    return reinterpret_cast<viewer_plugin_t*>(new Viewer(inputPath));
  } catch(...) {
    return nullptr;
  }
}

void Finalize(viewer_plugin_t * ctx) {
  if (ctx) {
    Viewer * viewer = reinterpret_cast<Viewer *>(ctx);
    delete viewer;
  }
}

// etc... on API
}

Without this implementation, I'm not too sure we should ship this.

[MatteoRagni]

Additionally, if we want to follow the "Python" pattern, we may consider to relax definition of function names and to force the plugin implementer to expose a unique symbol, which is a structure with a name that is derived by the shared object filename.

This avoid name clashes (depends on flags for dlopen, e.g. RTLD_NOW).

// In the interface file
struct PluginDefinition {
  const char * name;
  const char * version;
  const char * intialize_symbol; // or another structure for all symbols
  // ...
};

// For Example in ESMINI viewer plugin
// I actually do not remember if requires the export decorator
VIEWER static const struct PluginDefinition QcFrameworkViewerPlugin_esmini {
  "Esmini",
  "1.0.0",
  "esmini_viewer_Initialize",
  // ... etc
}

[pmai]

Again, late in the party, but there is something that is missing in the implementation of the interface, and is a container for the state or the context of the viewer implementation. This is a plugin so it is responsability of plugin caller (who calls Initialize) to keep an handle pointer (Bridge pattern) with the context and to call a appropriate Finalize method that frees the memory. I'm seeing from here that we are holding the context inside the memory space of the shared object and it is not fantastic:

No disagreements from me there in principle, but I would go further with this that the whole plugin API currently is not that well thought out; i.e. life-cycle handling vs. init handling, passing all issues via calls initially, while relying on C++ memory layout compatibility across the interface, etc.

I think a more thorough refactoring would be needed to fix this however, as this is heavily intertwined with the way the rest of the code base is structured, see next point:

1. We are forcing plugin implementer in Singleton (anti-)pattern (no multiple use)

That however is non-consequential right now, as there is no use for multi-instance plugins currently, as the whole code-base is not ready for e.g. more than one input file being handled. So I would propose to handle that kind of refactoring once we refactor the overall code-base to be multi-file capable/ready, where e.g. instantiaton/startup and file-selection should be merged into one call.

3. No guarantees on thread safety can be provided and sync is on plugin developer (and we cannot trust a plugin with application stability)

That is however generally true in this code base.

5. No actual lifecycle control (it is abi-implementation specific)> > There are several exploit that can developed based upon this premise.

Not sure what you mean here, as there is lifecycle control (StartViewer vs. CloseViewer), it just is not multi-instance capable. The fact that the esmini viewer code does not properly handle the loading/unloading of the esmini dll is just an implementation bug, I would say.

[jdsika]

Checkout a configuration example for the viewer example (lichtblick) to hand-over via argument. Try-out the mechanism with Simspector. Functionality was demostrated in the ASAM OSI project meeting