Page MenuHomeClusterLabs Projects

Handle output objects in pcmk_update_configured_schema()
Open, NormalPublic

Assigned To
Authored By
kgaillot
Dec 6 2023, 12:27 PM
Tags
  • Restricted Project
  • Restricted Project
  • Restricted Project
Referenced Files
None
Subscribers

Description

crm_verify calls pcmk_verify() which calls pcmk_update_configured_schema() with to_logs == false. That causes messages to be sent to stderr.

We need the messages to go to the output object so they are displayed in the user's requested format.

We should be able to reuse the config warn/error handlers used by pcmk__config_err() and pcmk__config_warn().

to_logs also filters down to pcmk__update_schema(), apply_upgrade(), and the validate_with() error handler; they might be affected as well. We'll have to come up with test cases that cause each function to generate a message.

pcmk_update_configured_schema() is public API, so we'll need a new internal function to take an output object argument, and the current function can become a wrapper for it.

Event Timeline

kgaillot triaged this task as Normal priority.Dec 6 2023, 12:27 PM
kgaillot created this task.
kgaillot created this object with edit policy "Restricted Project (Project)".
kgaillot renamed this task from Handle output objects in cli_config_update() to Handle output objects in pcmk_update_configured_schema().Jun 12 2024, 10:31 AM
kgaillot updated the task description. (Show Details)

Since this has turned out to be complicated, let's break it down into smaller pieces.

The types of error handlers involved are:

  • What I'll call "configured handlers": pcmk__config_error_handler set by pcmk__set_config_error_handler() and pcmk__config_warning_handler() set by pcmk__set_config_warning_handler() (these default to logging, but crm_verify sets them to the user's requested output format)
    • pcmk__config_err() and pcmk__config_warn() call the configured handlers
  • What I'll call "XML error handlers": xmlRelaxNGValidityErrorFunc, xmlRelaxNGValidityWarningFunc, and xmlGenericErrorFunc functions
    • libxml2 parsing and validation functions call the XML error handlers
    • Currently, the XML error handler is always either xml_log() with LOG_ERR as its argument, cib_upgrade_err() with either NULL or a log level as its argument, an output object's out->err() function with out as its argument, or NULL (which indicates stderr)
      • xml_log() usually calls PCMK__XML_LOG_BASE(priority, FALSE, 0, NULL, ...) (it should probably pass TRUE instead of FALSE like all other callers, which means that argument should just be dropped from PCMK__XML_LOG_BASE() so it can be simplified a bit); if silent_logging is true (set by pcmk__update_schema() -> validate_with_silent()) it does nothing
      • ultimately, xml_log() is passed to xmlRelaxNGSetParserErrors() and xmlRelaxNGSetValidErrors()
      • cib_upgrade_err(): set by apply_transformation() using xsltSetGenericErrorFunc(); generally logs errors using PCMK__XML_LOG_BASE(), but if a log level is passed (equivalent to apply_transformation()'s to_logs being false), errors much more severe than that log level will go to stderr

The functions that are involved are:

  • pcmk__verify(): takes an output object, which it uses for its own error messages (though it does have one call to pcmk__config_err()); called by pcmk_verify() which always creates an XML output object and by crm_verify.c:main() with the user's requested output format
    • pcmk__validate_xml(): takes an XML error handler and its argument (if NULL, messages should go to stderr); calls pcmk__config_err() for its only own error message (this should go to stderr instead if the XML error handler is NULL); pcmk__verify() is currently the only caller that passes a non-NULL XML error handler (using the user's requested output format), so all other callers currently send messages to stderr (including crm_simulate and cibadmin, which should use the user's requested output format instead)
      • validate_with(): takes an XML error handler and its argument; uses crm_err() for its only own error, but that would be an internal bug rather than related to the XML, so that's fine
        • validate_with_relaxng(): takes an XML error handler and its argument; as an aside, valid should be set to false when xmlRelaxNGValidateDoc() returns nonzero (it currently isn't if returning negative); calls crm_err() if xmlRelaxNGParse() returns NULL, which is likely redundant and should be dropped since the XML error handler should be called already (need to verify); also calls crm_err() if xmlRelaxNGValidateDoc() returns negative, which may or may not call the XML error handler (need to verify); sets its XML error handler (or fprintf() to stderr if NULL) using xmlRelaxNGSetParserErrors() and xmlRelaxNGSetValidErrors()
  • pcmk_update_configured_schema()/pcmk__update_configured_schema(): takes to_logs; uses pcmk__config_err()/pcmk__config_warn() (if to_logs is true) and stderr for its own errors
    • pcmk__update_schema(): takes to_logs; calls validate_with() using xml_log() as the XML error handler if to_logs is true (otherwise NULL which indicates stderr); also called for cibadmin --upgrade requests by pacemaker-based (or cib_process_upgrade() if CIB_file is in use), and by cibadmin.c:main() after cibadmin --upgrade failures
      • validate_with_silent(): sets silent_logging then calls validate_with() so that errors are ignored
      • apply_upgrade(): takes to_logs; calls validate_with() using xml_log() as the XML error handler if to_logs is true (otherwise NULL which indicates stderr)
        • apply_transformation(): takes to_logs; sets the XSLT error function to cib_upgrade_err() (passing the global log level if to_logs is false) and calls XSLT APIs
  • pcmk__configured_schema_validates(): calls pcmk__validate_xml() with xml_log()

Here's what I think we should do, in order (each point being at least one commit):

  1. Ensure that there are cts-cli validity tests that trigger the error messages we're interested in (T873)
  2. Try having xml_log() call PCMK__XML_LOG_BASE() with TRUE. If nothing breaks, drop that argument from PCMK__XML_LOG_BASE() (treat it as always true) to simplify that macro.
  3. Make cibadmin and crm_simulate set the configured handlers the same way crm_verify does. Verify that any changes in the test outputs are improvements. (This would be a good point to do a PR.)
  4. My general idea is that we should always call pcmk__config_err()/pcmk__config_warn() for CIB XML (parsing/validating/transforming) errors. That way they will go to logs or the configured output object as appropriate, without any special handling in the schema code. We can start with PCMK__XML_LOG_BASE()
    • pcmk__log_xmllib_err() (which calls PCMK__XML_LOG_BASE()) is used for XML other than the CIB, so we can't unconditionally change that one. Copy PCMK__XML_LOG_BASE() to its file under a new name and use it there, so it's unaffected by our changes. That does mean that XML parsing errors will always go to logs, but that's fine for now at least.
    • The remaining callers are in schemas.c, so move the macro there. It's not needed in any header.
    • While we're at it, there are a few comments referencing the macro in files that don't use it. Those comments can be deleted.
    • Make the schemas.c version call pcmk__config_err()/pcmk__config_warn() instead of qb_log_from_external_source() if priority is warning or error. This should make a lot of messages (anything called via xml_log(), and anything called via cib_upgrade_err() except the -VVVV) go to the appropriate place.
  5. cib_upgrade_err() should output to stderr only if pcmk__config_error_handler is NULL (and no longer needs its log_level argument since it can use this test instead, and reference crm_log_level directly). Otherwise it should call PCMK__XML_LOG_BASE() as usual. This should make the -VVVV output go to XML instead of stderr when --output-format=xml is used.
  6. At this point, apply_transformation() no longer needs its to_logs argument since it can always pass NULL for use with cib_upgrade_err(). (This would be another good point for a PR.)

There would be more after that, but that's a lot to start with, so we can see if that works as I expect.