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

Load-plugins related issues fixes #177

Merged
merged 13 commits into from
Dec 10, 2021

Conversation

dulive
Copy link
Contributor

@dulive dulive commented Nov 10, 2021

Fixed a memory leak inserted with the load-plugins options, where the plugins_to_load queue of the sys_config variable was not being destroyed on the mptcpd_config_create function.

Added a missing const qualifier to the input argument plugins of the set_plugins_to_load function.

Removed an extra space and dot on the man page.

@coveralls
Copy link

coveralls commented Nov 10, 2021

Pull Request Test Coverage Report for Build 1546713033

Warning: This coverage report may be inaccurate.

This pull request's base commit is no longer the HEAD commit of its target branch. This means it includes changes from outside the original pull request, including, potentially, unrelated coverage changes.

Details

  • 35 of 38 (92.11%) changed or added relevant lines in 2 files are covered.
  • 324 unchanged lines in 3 files lost coverage.
  • Overall coverage decreased (-1.7%) to 47.719%

Changes Missing Coverage Covered Lines Changed/Added Lines %
lib/plugin.c 32 35 91.43%
Files with Coverage Reduction New Missed Lines %
lib/plugin.c 35 68.35%
src/configuration.c 64 55.3%
lib/network_monitor.c 225 41.63%
Totals Coverage Status
Change from base Build 1418473626: -1.7%
Covered Lines: 1004
Relevant Lines: 2104

💛 - Coveralls

@dulive
Copy link
Contributor Author

dulive commented Nov 10, 2021

Also, on the load_plugins function of the lib/plugin.h, since DIR *const ds = fdopendir(fd) is only used if it will load all the plugins it probably should be inside of the else statement right?

@dulive dulive marked this pull request as draft November 10, 2021 13:43
@ossama-othman ossama-othman added the bug Something isn't working label Nov 10, 2021
@ossama-othman ossama-othman added this to the Q4-2021 milestone Nov 10, 2021
man/mptcpd.8.in Outdated Show resolved Hide resolved
src/configuration.c Outdated Show resolved Hide resolved
src/configuration.c Show resolved Hide resolved
src/configuration.c Show resolved Hide resolved
src/configuration.c Outdated Show resolved Hide resolved
src/configuration.c Outdated Show resolved Hide resolved
dulive and others added 5 commits December 1, 2021 10:49
Co-authored-by: Ossama Othman <ossama.othman@gmail.com>
Co-authored-by: Ossama Othman <ossama.othman@gmail.com>
Co-authored-by: Ossama Othman <ossama.othman@gmail.com>
Co-authored-by: Ossama Othman <ossama.othman@gmail.com>
@ossama-othman
Copy link
Member

Also, on the load_plugins function of the lib/plugin.h, since DIR *const ds = fdopendir(fd) is only used if it will load all the plugins it probably should be inside of the else statement right?

Yes, that's correct.

@dulive dulive marked this pull request as ready for review December 6, 2021 21:55
@dulive
Copy link
Contributor Author

dulive commented Dec 9, 2021

Do you want me to comment some parts to explain why things are not needed or why they are done that way?

@ossama-othman
Copy link
Member

Do you want me to comment some parts to explain why things are not needed or why they are done that way?

Sure, that would be great. Thanks!

@ossama-othman
Copy link
Member

@dulive I pushed two additional fixes to this pull request.

lib/plugin.c Outdated Show resolved Hide resolved
@dulive
Copy link
Contributor Author

dulive commented Dec 9, 2021

I completely forgot about the tests, sorry.

There is no need to call close() on the file on the file descriptor
after a call to fdopendir().  closedir() is sufficient since
fdopendir() basically claims onwership of the file descriptor.
Calling close() after closedir() in this case actually results in a
EBADF error.

This reverts commit c6762cd.
@ossama-othman
Copy link
Member

I completely forgot about the tests, sorry.

No worries. Thanks for your contributions! :)

@ossama-othman
Copy link
Member

This looks good. Do you have any other changes pending? If not, I'll squash and merge. Thanks so much!

@dulive
Copy link
Contributor Author

dulive commented Dec 10, 2021

This looks good. Do you have any other changes pending? If not, I'll squash and merge. Thanks so much!

That's all the changes I had.

@ossama-othman ossama-othman merged commit 504e8c5 into multipath-tcp:master Dec 10, 2021
@dulive dulive deleted the load_plugins_fix branch December 10, 2021 17:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants