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

Fix license cmd #2351

Merged
merged 4 commits into from Oct 12, 2018

Conversation

Projects
None yet
6 participants
@lbudai
Contributor

lbudai commented Oct 11, 2018

@czanik : FYI

Show outdated Hide outdated lib/control/control-commands.c Outdated
@kira-syslogng

This comment has been minimized.

Show comment
Hide comment
@kira-syslogng

kira-syslogng Oct 11, 2018

Contributor

Build SUCCESS

Contributor

kira-syslogng commented Oct 11, 2018

Build SUCCESS

1 similar comment
@kira-syslogng

This comment has been minimized.

Show comment
Hide comment
@kira-syslogng

kira-syslogng Oct 11, 2018

Contributor

Build SUCCESS

Contributor

kira-syslogng commented Oct 11, 2018

Build SUCCESS

{
msg_warning("Trying to replace a non-existent command. Command will be registered as a new command.",
evt_tag_str("command", command_name));
control_register_command(command_name, description, function, user_data);

This comment has been minimized.

@bazsi

bazsi Oct 11, 2018

Member

Do we need the warning here?

If we do, I think we need a bit more context, like mentioning that this was a control socket command, but I like the idea that modules can register such stuff.

But all in all I would make this msg_debug().

@bazsi

bazsi Oct 11, 2018

Member

Do we need the warning here?

If we do, I think we need a bit more context, like mentioning that this was a control socket command, but I like the idea that modules can register such stuff.

But all in all I would make this msg_debug().

This comment has been minimized.

@lbudai

lbudai Oct 12, 2018

Contributor

changed to debug

@lbudai

lbudai Oct 12, 2018

Contributor

changed to debug

@furiel

From security point if view, I would sleep better if modules cannot change behavior of existing commands. I would probably have deleted the option if it is not needed, and allow modules to register their own commands as new commands. Other than that, the changeset looks good to me.

@gaborznagy gaborznagy self-requested a review Oct 12, 2018

@gaborznagy

This comment has been minimized.

Show comment
Hide comment
@gaborznagy

gaborznagy Oct 12, 2018

Contributor

I think nothing protects from re-registering a command with control_register_command, although you cannot override an already registered due to GList find.

We should rethink the control_register_command behaviour.

Contributor

gaborznagy commented Oct 12, 2018

I think nothing protects from re-registering a command with control_register_command, although you cannot override an already registered due to GList find.

We should rethink the control_register_command behaviour.

@lbudai

This comment has been minimized.

Show comment
Hide comment
@lbudai

lbudai Oct 12, 2018

Contributor

@gaborznagy : fixed

Contributor

lbudai commented Oct 12, 2018

@gaborznagy : fixed

@lbudai lbudai dismissed stale reviews from furiel and bazsi via 47bf99c Oct 12, 2018

@kira-syslogng

This comment has been minimized.

Show comment
Hide comment
@kira-syslogng

kira-syslogng Oct 12, 2018

Contributor

Build SUCCESS

Contributor

kira-syslogng commented Oct 12, 2018

Build SUCCESS

lbudai added some commits Oct 11, 2018

control-commands: add support to replace existing commands
Signed-off-by: Laszlo Budai <stentor.bgyk@gmail.com>
control-commands: add valid LICENSE command handler for OSE
Signed-off-by: Laszlo Budai <stentor.bgyk@gmail.com>
control-commands: register as new cmd when try to replace a non-exist…
…ent one

Signed-off-by: Laszlo Budai <stentor.bgyk@gmail.com>
control-commands: do not re-register an already registered command
Signed-off-by: Laszlo Budai <stentor.bgyk@gmail.com>
@bazsi

This comment has been minimized.

Show comment
Hide comment
@bazsi

bazsi Oct 12, 2018

Member
Member

bazsi commented Oct 12, 2018

@kira-syslogng

This comment has been minimized.

Show comment
Hide comment
@kira-syslogng

kira-syslogng Oct 12, 2018

Contributor

Build SUCCESS

Contributor

kira-syslogng commented Oct 12, 2018

Build SUCCESS

@szemere szemere merged commit 52844c7 into balabit:master Oct 12, 2018

2 checks passed

Kira-starter Build SUCCESS
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment