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

afsocket: Remove invalid assertion from afsocket_dd_restore_writer. #1723

Merged
merged 1 commit into from
Oct 26, 2017

Conversation

swstephenson
Copy link
Contributor

Asserting that self->writer is null seems to be incorrect.

If the config being initialized is new, self->writer is guaranteed
to be null, however if we are re-initializing an old config after initializing
a new configuration failed, an existing writer may be present if keep-alive
is off (old_config self->writer is set to null if the connection is saved)

Fixes #1722

Signed-off-by: Sam Stephenson sam.stephenson@alliedtelesis.co.nz

@kira-syslogng
Copy link
Contributor

This user does not have permission to start the build. Can one of the admins verify this patch and start the build? (admin please type: ok to test)

@Kokan
Copy link
Collaborator

Kokan commented Oct 19, 2017

@kira-syslogng ok to test

@kira-syslogng
Copy link
Contributor

Build SUCCESS, the tests were executed on test branch: master and test suite: functions

Copy link
Collaborator

@bazsi bazsi left a comment

Choose a reason for hiding this comment

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

The diagnosis is correct, that assert is not true anymore, however we should probably check that cfg_persist_config_fetch() returns NULL in this case, otherwise we would be leaking the self->writer instance.

So exactly one of self->writer or cfg_persist_config_fetch() must be non-NULL but not both.

@kira-syslogng
Copy link
Contributor

Build SUCCESS, the tests were executed on test branch: master and test suite: functions

Asserting that self->writer is null seems to be incorrect.

If the config being initialized is new, self->writer is guaranteed
to be null, however if we are restoring an old config after initializing
a new configuration failed, an existing writer may be present if keep-alive
is off (old_config self->writer is set to null if the connection is saved)

Instead check that self->writer is non NULL before attempting to restore one.
If an existing writer is present, the stored writer should be freed by
_reload_store_item_free().

Fixes syslog-ng#1722

Signed-off-by: Sam Stephenson <sam.stephenson@alliedtelesis.co.nz>
@kira-syslogng
Copy link
Contributor

Build SUCCESS, the tests were executed on test branch: master and test suite: functions

1 similar comment
@kira-syslogng
Copy link
Contributor

Build SUCCESS, the tests were executed on test branch: master and test suite: functions

@lbudai
Copy link
Collaborator

lbudai commented Oct 25, 2017

LGTM.
Just a note (nothing to do with this in the PR):

  • the if statement is a little bit complicated:
 if (!self->writer && item && !_is_protocol_type_changed_during_reload(self, item))

it says that when

  • we don't have a writer instance
    • because we set it to NULL when we stored it in the reload cache (aka persist_config) which can happen only when keepalive(yes)
    • OR it is the first time when we try to initialize the writer
  • AND we have an item stored in the reload cache (which says that this is not the first time when we try to intialize the writer...)
  • AND the protocol is still compatible with the writer,
    then we re-use the writer instance stored in the reload cache...

@lbudai lbudai merged commit 21ca5c7 into syslog-ng:master Oct 26, 2017
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

5 participants