socket.c improvements and unit tests #914
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
cfconrad
force-pushed
the
wip-memory-curruption-2
branch
from
55f0427
to
6474f77
Compare
When ever we do not care of a ni_socket_t object, we need to call the ni_socket_release(), otherwise the reference will kept forever.
cfconrad
force-pushed
the
wip-memory-curruption-2
branch
2 times, most recently
from
c2aaac6
to
6c0a08b
Compare
mtomaschewski
requested changes
May 30, 2022
There was a problem hiding this comment.
- The commit 6c0a08b says:
testing: Run tests within valgrind mem-check
To do so, we use the automake `test-driver` and adopt it
to the functionality of valgrind.
But it does not provide the test-driver with --valgrind support any more.
- After a
make checkrun, there are untracked files:
Untracked files:
(use "git add <file>..." to include in what will be committed)
testing/socket-mock-test.log
testing/socket-mock-test.trs
testing/test-suite.log
nothing added to commit but untracked files present (use "git add" to track)
In ni_socket_array_wait() we should not manipulating the same array what we use for iteration. The static socket array __ni_sockets should not be used for iteration, as we might manipulate the array via ni_socket_close() from other modules.
The close() callback was called unconditional, thus it got called even if the file descriptor is already unset.
If a `ni_socket_release()` is called with a refcount equal to `1`, we hit a assert in a recursive call of ni_socket_release(). Simply check that no one call `ni_socket_release()` on the last reference, while the socket is still active!
cfconrad
force-pushed
the
wip-memory-curruption-2
branch
from
6c0a08b
to
8d7df5f
Compare
Cover upcoming regressions and check for issues in corner cases which failed in old implementation.
cfconrad
force-pushed
the
wip-memory-curruption-2
branch
from
8d7df5f
to
d2e04e2
Compare
|
Good catch. |
mtomaschewski
previously approved these changes
May 31, 2022
There was a problem hiding this comment.
Hmm... almost. It works fine now... except of the fact, that autogen.sh (autoreconf) overwrites the test-driver with the --valgrind=yes/no addition...:
$ git status
On branch pull-914-3
Your branch is up to date with 'github/pull/914/head'.
Changes not staged for commit:
(use "git add <file>..." to update what will be committed)
(use "git restore <file>..." to discard changes in working directory)
modified: test-driver
no changes added to commit (use "git add" and/or "git commit -a")
$ git diff
diff --git a/test-driver b/test-driver
index db1a2378..d86cf694 100755
--- a/test-driver
+++ b/test-driver
@@ -1,8 +1,9 @@
-#!/bin/sh
+#! /bin/sh
# test-driver - basic testsuite driver script.
-#
+
+scriptversion=2016-01-11.22; # UTC
+
# Copyright (C) 2011-2017 Free Software Foundation, Inc.
-# Copyright (C) 2022 SUSE LLC
#
# This program is free software; you can redistribute it and/or modify
# it under the terms of the GNU General Public License as published by
[...]
|
@cfconrad Perhaps we should use the standard test-driver first and take a separate / deeper look at e.g.:
later? |
cfconrad
force-pushed
the
wip-memory-curruption-2
branch
from
d2e04e2
to
dde20d8
Compare
|
urgs, I didn't realized it, sorry! |
mtomaschewski
approved these changes
Jun 1, 2022
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This PR is the second outcome of (bsc#1192508) and the continues work started with
#912 .
The
src/socket.cmodule had some possible issues in special cases, which solved withthese changes. To avoid regressions, I created a unit test suit for that module, which show the issues in
the old implementation.
Also some code cleanup in dbus was added, where we had wrong refcount handling of
ni_socket_tobjects.