Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add a way to report rpc-errors from a plugin #562

Open
wants to merge 12 commits into
base: master
Choose a base branch
from

Conversation

cminyard
Copy link
Contributor

Add plugin_rpc_err(), which works something like clixon_err, but it saves error information from a plugin for reporting rpc-errors that need to be returned.

This is an RFC, would this be ok?

@cminyard cminyard force-pushed the add-plugin-rpc-err branch 3 times, most recently from 9dc4b28 to 71baf6f Compare November 25, 2024 17:18
Add plugin_rpc_err(), which works something like clixon_err, but it
saves error information from a plugin for reporting rpc-errors that
need to be returned.

Signed-off-by: Corey Minyard <corey@minyard.net>
The error return wasn't properly being generated in the statedata case.

Signed-off-by: Corey Minyard <corey@minyard.net>
An XML version was needed, and it had much in common with the
netconf_common_xml() function, so make a common XML function for
generating rpc-error output.

Signed-off-by: Corey Minyard <corey@minyard.net>
The handling of an error from validate_commit() for the rpc_err changes
was incorrect.  If cbret has data in it, use that for the return error.

Signed-off-by: Corey Minyard <corey@minyard.net>
There was a lot of duplicated code, consolidate the error handling
into a couple of functions.  This simplifies things some, and makes
the rpc_err changes less intrusive.

Signed-off-by: Corey Minyard <corey@minyard.net>
Tack it on to the other transaction error tests and use the same
infrastructure.

Signed-off-by: Corey Minyard <corey@minyard.net>
Copy link
Member

@olofhagsand olofhagsand left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is very useful, thank you for your contribution.
A final comment is that it needs documentation and examples.
Please add PR either in https://clixon-docs.readthedocs.io/en/latest/plugins.html or https://clixon-docs.readthedocs.io/en/latest/errors.html

lib/src/clixon_netconf_lib.c Show resolved Hide resolved
example/main/example_backend_nacm.c Outdated Show resolved Hide resolved
apps/backend/backend_commit.c Outdated Show resolved Hide resolved
apps/backend/backend_commit.c Outdated Show resolved Hide resolved
apps/backend/backend_commit.c Outdated Show resolved Hide resolved
apps/backend/backend_commit.c Show resolved Hide resolved
apps/backend/backend_commit.c Outdated Show resolved Hide resolved
_plugin_rpc_err_tag = n_tag;
_plugin_rpc_err_info = n_info;
_plugin_rpc_err_severity = n_severity;
_plugin_rpc_err_message = n_message;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How is these memory freed on exit?
If you run pattern=./test_transaction.sh ./mem.sh backend this memory is still active.
Yes, one could say that it is freed on exit anyway, but for cleanness, and detecting other memory errors, and in a remote case that one runs several handles in different threads, all heap memory should be freed before exit.
The most basic way is to make a separate free function and call it in backend_terminate().
But if you add the memory to the handle or make it generic to all plugins, it will be slightly different.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's now handled with clicon_data_cvec_xxx(), so it should be ok now, right? I don't see how that's freed, though.

The netconf_common_xml() was a strict subset of
netconf_common_rpc_err_xml(), so just replace it.

Signed-off-by: Corey Minyard <corey@minyard.net>
Move all parameter declarations to their own line.

Add some missing comments.

Rename cbret to xret for an cxobj.

Signed-off-by: Corey Minyard <corey@minyard.net>
Get ready for moving the data into clixon_handle.

Signed-off-by: Corey Minyard <corey@minyard.net>
Store it in a cvec in the clixon_data.

Signed-off-by: Corey Minyard <corey@minyard.net>
It was in the backend, but it could be useful for other plugins, so
move it to clixon_plugin.c

Signed-off-by: Corey Minyard <corey@minyard.net>
It was checking the wrong variable.

Signed-off-by: Corey Minyard <corey@minyard.net>
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.

2 participants