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

Added changes to permit pppoe interfaces #38

Open
wants to merge 3 commits into
base: master
Choose a base branch
from
Open
Changes from 2 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion src/config.c
Original file line number Diff line number Diff line change
@@ -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);
Copy link
Owner

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?

Copy link
Author

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.

Copy link

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

pfsense/FreeBSD-ports@009d60c

Copy link
Author

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....

Copy link
Author

@ljpaff ljpaff Aug 2, 2018

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?

Copy link
Owner

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.

tmpSubnet->subnet_mask = ntohl(mask);
tmpSubnet->next = NULL;

53 changes: 39 additions & 14 deletions src/ifvc.c
Original file line number Diff line number Diff line change
@@ -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 */
Copy link
Owner

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?

struct ifreq *IfEp;
struct IfDesc *dp;
struct SubnetList *allowednet, *currsubnet;
struct Config *config = getCommonConfig();

int Sock;
@@ -246,6 +248,19 @@ void buildIfVc(void) {
if (IfNext < IfPt + 1)
IfNext = IfPt + 1;

/* don't retrieve any further info if MAX_IF is reached
*/
if ( IfDescEp >= &IfDescVc[ MAX_IF ] ) {
my_log(LOG_DEBUG, 0, "Too many interfaces, skipping all since %s", IfPt->ifr_name);
break;
}

/* don't retrieve any info from non-IPv4 interfaces
*/
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;
Copy link
Owner

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.

}
strncpy( IfDescEp->Name, IfPt->ifr_name, sizeof( IfDescEp->Name ) );

// Currently don't set any allowed nets...
@@ -254,14 +269,6 @@ void buildIfVc(void) {
// Set the index to -1 by default.
IfDescEp->index = (unsigned int)-1;

/* don't retrieve more info for non-IP interfaces
*/
if ( IfPt->ifr_addr.sa_family != AF_INET ) {
IfDescEp->InAdr.s_addr = 0; /* mark as non-IP interface */
IfDescEp++;
continue;
}

// Get the interface adress...
IfDescEp->InAdr.s_addr = s_addr_from_sockaddr(&IfPt->ifr_addr);
addr = IfDescEp->InAdr.s_addr;
@@ -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);
Copy link
Owner

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?

Copy link
Author

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.

Copy link
Owner

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.

if (dp != NULL && dp->allowednets != NULL) {
allowednet = (struct SubnetList *)malloc(sizeof(struct SubnetList));
if (allowednet == NULL) my_log(LOG_ERR, 0, "Out of memory !");
allowednet->next = NULL;
allowednet->subnet_mask = mask;
allowednet->subnet_addr = subnet;
currsubnet = dp->allowednets;
while (currsubnet->next != NULL)
currsubnet = currsubnet->next;
currsubnet->next = allowednet;
continue;
Copy link
Owner

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...

}


/* get if flags
**
** typical flags:
@@ -333,13 +355,16 @@ void buildIfVc(void) {
** - NULL if no interface 'IfName' exists
**
*/
struct IfDesc *getIfByName( const char *IfName ) {
struct IfDesc *getIfByName( const char *IfName, int iponly ) {
struct IfDesc *Dp;

for ( Dp = IfDescVc; Dp < IfDescEp; Dp++ )
if ( ! strcmp( IfName, Dp->Name ) )
return Dp;

for ( Dp = IfDescVc; Dp < IfDescEp; Dp++ ) {
if (iponly && Dp->InAdr.s_addr == 0)
continue;
if ( ! strcmp( IfName, Dp->Name ) )
return Dp;
}

return NULL;
}

2 changes: 1 addition & 1 deletion src/igmpproxy.h
Original file line number Diff line number Diff line change
@@ -186,7 +186,7 @@ extern int upStreamIfIdx[MAX_UPS_VIFS];
*/
void rebuildIfVc( void );
void buildIfVc( void );
struct IfDesc *getIfByName( const char *IfName );
struct IfDesc *getIfByName( const char *IfName, int iponly );
struct IfDesc *getIfByIx( unsigned Ix );
struct IfDesc *getIfByAddress( uint32_t Ix );
struct IfDesc *getIfByVifIndex( unsigned vifindex );