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

cfg_rpc: extending the functionality of cfg.get command #1321

Closed
wants to merge 1 commit into from

Conversation

hdikme
Copy link
Contributor

@hdikme hdikme commented Nov 20, 2017

  • rpc_get function can be also called with only entering the group name information.
    Before it used to work only with group name and variable name and returning the value for a variable.
    With the enhancement, it is also possible to retrieve all variable values belonging to a specific group.
    e.g.: kamcmd cfg.get core.
    Above given example will print the values of all variables of the configuration group "core".

Pre-Submission Checklist

  • Commit message has the format required by CONTRIBUTING guide
  • Commits are split per component (core, individual modules, libs, utils, ...)
  • Each component has a single commit (if not, squash them into one commit)
  • No commits to README files for modules (changes must be done to docbook files
    in doc/ subfolder, the README file is autogenerated)

Type Of Change

  • Small bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds new functionality)
  • Breaking change (fix or feature that would change existing functionality)

Checklist:

  • PR should be backported to stable branches
  • Tested changes locally
  • Related to issue #XXXX (replace XXXX with an open issue number)

Description

rpc_get function can be also called with only entering the group name information.
Before it used to work only with group name and variable name and returning the value for a variable.
With the enhancement, it is also possible to retrieve all variable values belonging to a specific group.
e.g.: kamcmd cfg.get core.
Above given example will print the values of all variables of the configuration group "core".

- rpc_get function can be also called with only entering the group name information.
Before it used to work only with group name and variable name and returning the value for a variable.
With the enhancement, it is also possible to retrieve all variable values belonging to a specific group.
e.g.: kamcmd cfg.get core.
Above given example will print the values of all variables of the configuration group "core".
var.len = 0;
}
else return;
}
Copy link
Member

@miconda miconda Nov 23, 2017

Choose a reason for hiding this comment

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

I think that the right way is to read with optional specifier *:

n = rpc->scan(c, "S*S", &group, &var);

Then n is 2 if both were read or 1 if only group is read.

When n is -1, there can be other errors.

Otherwise, the patch is useful, thanks!

Can you do a new patch based on what I suggested? It can be a follow up of this one, we can squash from the web when merging.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thank you for the review, sure i'll send a new patch with the changes.

@miconda
Copy link
Member

miconda commented Dec 8, 2017

@hdikme - planning to have the new patch soon? Just to evaluate if this needs to be in 5.1 or not which is going to be out in few days...

@hdikme
Copy link
Contributor Author

hdikme commented Dec 8, 2017

@miconda - I have tested the code with the changes based on your review. First part is indeed more appropriate (S*S) but i have a problem with the value of n. If only one parameter (group name) is given , n is still assigned -1. Either way S*S or SS returns -1. What do you think?

@miconda
Copy link
Member

miconda commented Dec 8, 2017

What do you use to send the rpc commands - kamcmd? If yes, can you try also with kamctl rpc cfg.get ...? I want to see if it is some issue in ctl module. For kamctl you need jsonrpcs module to be loaded.

@hdikme
Copy link
Contributor Author

hdikme commented Dec 15, 2017

Yes, I use kamcmd as I think this is the new one and this should replace kamctl someday in the future. I tried with kamctl rpc, but it just prints out the help page of kamctl. It seems kamctl does not know rpc as a parameter. Maybe you can provide me a more detailed description how to test this with kamctl? Or maybe it's even faster you test it yourself using kamctl?

@miconda
Copy link
Member

miconda commented Dec 15, 2017

Are you testing with master branch (or 5.x)? kamctl rpc ... works there.

@miconda
Copy link
Member

miconda commented Dec 15, 2017

Btw, kamcmd is not a replacement for kamctl.

kamcmd is only a binrpc client application that connects to ctl module of kamailio.

kamcli might replace kamctl, but no timeline for that. kamctl is still very active maintained and enhanced with each release.

@hdikme
Copy link
Contributor Author

hdikme commented Dec 22, 2017

I have tested it with the version 5.0.1 using kamctl rpc ... and the result hasn't changed.
n = rpc->scan(c, "S*S", &group, &var);
n equals to -1 in case of using only the group name.

miconda added a commit that referenced this pull request Jan 23, 2018
- if only one parameter is given (group name), cfg.get returns a
structure with all variables and their values in the group
- patched enhaced from the PR #1321 by Huseyin Dikme <hueseyin.dikme@1und1.de>
miconda pushed a commit that referenced this pull request Jan 23, 2018
@miconda
Copy link
Member

miconda commented Jan 23, 2018

The issue with n = rpc->scan(c, "S*S", &group, &var); was due to a bug in ctl module, using kamcmd revealed that. Using kamctl with jsonrpcs module was not affected. The bug is fixed now in master and it will be backported.

Besides using the * specifier for optional params, your patch in this PR was adjusted so that the cfg.get for a group returns a structure with field names and values. All are in master now.

@miconda miconda closed this Jan 23, 2018
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.

None yet

2 participants