-
Notifications
You must be signed in to change notification settings - Fork 77
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
Added changes to permit pppoe interfaces #38
base: master
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have not done review of code yet, just quick look and ...
src/config.c
Outdated
@@ -396,7 +396,7 @@ struct SubnetList *parseSubnetAddress(char *addrstr) { | |||
} | |||
|
|||
tmpSubnet = (struct SubnetList*) malloc(sizeof(struct SubnetList)); | |||
tmpSubnet->subnet_addr = addr; | |||
tmpSubnet->subnet_addr = (addr & mask); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why is this change needed?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Because in ifvc.c are now handling subnets on the form addr & mask so it must be changed in order to work properly.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I just would like to note that this specific change was reverted on pfSense
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok, so this change must be reverted....
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You are right, yesterday night i had compiled the source code and tested it. It is working perfectly without this change, and i can confirm that igmpproxy is working with a vlan tagged with pppoe interface.
What are next steps?. Do I cancel this pull request and open another one without config.c change or Do I revert the change in this pull request?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please update this pull request. There is no need to create a new one.
@@ -274,6 +281,21 @@ void buildIfVc(void) { | |||
mask = s_addr_from_sockaddr(&IfReq.ifr_addr); // Do not use ifr_netmask as it is not available on freebsd | |||
subnet = addr & mask; | |||
|
|||
dp = getIfByName(IfPt->ifr_name, 1); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What is the purpose of function parameter which is always constant?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It is to be able to take advantage of the function so that it also works in the same way as it did in the past.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
But that function is not called from other places. Such change introduce a dead code as parameter is never false.
@@ -201,8 +201,10 @@ void rebuildIfVc () { | |||
** | |||
*/ | |||
void buildIfVc(void) { | |||
struct ifreq IfVc[ sizeof( IfDescVc ) / sizeof( IfDescVc[ 0 ] ) ]; | |||
struct ifreq IfVc[ MAX_IF * 2 ]; /* allocate in double in order to get as much information as possible */ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why *2
? Maximal number is maximal
. So either defined constant is not so large or there is another hidden problem?
*/ | ||
if ( IfPt->ifr_addr.sa_family != AF_INET ) { | ||
my_log(LOG_DEBUG, 0, "Interface is not AF_INET, skipping %s (family %d)", IfPt->ifr_name, IfPt->ifr_addr.sa_family); | ||
continue; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Skipping interfaces without IPv4 address would mean igmpproxy itself would not see this interface. Why such change is needed? Previously these interfaces was normally processed just marked as non-IPv4.
while (currsubnet->next != NULL) | ||
currsubnet = currsubnet->next; | ||
currsubnet->next = allowednet; | ||
continue; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This completely skip code which is below. Why?
Looks like this is a hack which just hide the fact that there is a probably bug in code below and try to invisibly skip it. This change is non-intuitive and introduce (potentially) dead code. Also probably may introduce another bugs as parts like IfDescEp++;
are not executed...
@ljpaff Could you please address issues above? |
Workaround for:
Igmpproxy is not starting if upstream interface is a pppoe interface #28