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.

Sign up for GitHub

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

Jump to bottom

Cleanup formatting and minor code changes #2

Closed
wants to merge 3 commits into from
Closed

Cleanup formatting and minor code changes #2

wants to merge 3 commits into from

Conversation

ViToni
Copy link
Contributor

@ViToni ViToni commented Apr 13, 2016

@pali
Copy link
Owner

pali commented Apr 20, 2016

Hi! Please add proper description into commit message and do not add external links... Also maybe you should squash them into one commit.

@ViToni ViToni force-pushed the next branch 4 times, most recently from fd66d90 to 866b4b5 Compare April 21, 2016 20:29
@ViToni
Copy link
Contributor Author

ViToni commented Apr 21, 2016

Could you please check again.

pali
pali reviewed Apr 29, 2016
View reviewed changes
src/ifvc.c
dp = getIfByName(IfPt->ifr_name, 1);
if (dp != NULL && dp->allowednets != NULL) {
allowednet = (struct SubnetList *)malloc(sizeof(struct SubnetList));
if (allowednet == NULL) my_log(LOG_ERR, 0, "Out of memory !");
Copy link
Owner

Choose a reason for hiding this comment

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

this is broken code

Copy link
Owner

Choose a reason for hiding this comment

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

Please fix this part and also fix your email address in your git commit.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed the formatting related to this change.

The email adress is the same as used by changes done inside the GitHub UI. Which one is prefered for consistency (GitHub email or private email)?

Copy link
Owner

Choose a reason for hiding this comment

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

Null pointer dereference is still not fixed. Also rebase patch on top of branch.

Email address must be valid. Does not matter if private or public... Just valid address on which people can reach you (e.g. in case somebody would need to update your patch).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Maybe you could explain the problem in the above code to me, because my C is quite rusty and I clearly fail to see it.

Copy link
Owner

Choose a reason for hiding this comment

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

Null pointer dereference:

allowednet = ...
if (allowednet == NULL) my_log(...);
allowednet->...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I believe code above avoids NULL pointer dereference because logging an error via

if (allowednet == NULL) my_log(LOG_ERR, 0, "Out of memory !");

leads to 'src/syslog.c' and there it exits after logging:

if( Severity <= LOG_ERR ) exit( -1 );

@ViToni ViToni force-pushed the next branch 4 times, most recently from 2cb283d to 32c8241 Compare May 20, 2016 22:56
@ViToni ViToni changed the title Properly handle IP aliases in igmpproxy. (FreeBSD / pfSense related) Cleanup formatting and mirno code changes May 20, 2016
@ViToni ViToni changed the title Cleanup formatting and mirno code changes Cleanup formatting and minor code changes May 20, 2016
@ViToni
Copy link
Contributor Author

ViToni commented May 20, 2016 •
edited

Rewrote all email addresses so that only one email is used for changes by my
Removed the code in question. The puill request consists now mainly of cleanups, chnage of incompatible code und removal of code duplicates.
Especially the code formatting should ease further patches.

@pali
Copy link
Owner

pali commented May 21, 2016

"This branch has conflicts that must be resolved"

And please do different things in different commits/pull requests.
Also include description into commit message. People should understand it without need to open external links...

@ViToni
Adding support for fbsd 9/10 due to changes in struct ip.
c8bdcbb 
Original patch submitted by: yuri@rivera.ru
@ViToni
Minor change in wording of logging message.
2f167d9
@ViToni
Replace call to setpgrp with setpgid
0c5df49 
Linux and FreeBSD use a different signature for setpgrp.
Both support setpgid which replaces the now obsolete setpgrp and has the same signature on both systems.
@ViToni ViToni closed this May 21, 2016
@Mushoz Mushoz mentioned this pull request Mar 24, 2018
Open
@MattisLind MattisLind mentioned this pull request Mar 29, 2018
Open
@TheBR TheBR mentioned this pull request Sep 10, 2018
Closed
@stduong stduong mentioned this pull request Jul 24, 2019
Open
@Uglymotha Uglymotha mentioned this pull request May 8, 2020
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
2 participants
@ViToni @pali