Skip to content

aix: add netmask, mac address into net interfaces#1410

Closed
gireeshpunathil wants to merge 1 commit intolibuv:v1.xfrom
gireeshpunathil:aix-port-netmask-macaddr
Closed

aix: add netmask, mac address into net interfaces#1410
gireeshpunathil wants to merge 1 commit intolibuv:v1.xfrom
gireeshpunathil:aix-port-netmask-macaddr

Conversation

@gireeshpunathil
Copy link
Copy Markdown
Contributor

@gireeshpunathil gireeshpunathil commented Jul 8, 2017

uv_interface_addresses API extracts the network interface entries.
In AIX, this was not fully implemented. retrieve the network mask and
the mac addresses.

Fixes: nodejs/node#14119

ok 284 - thread_mutex
ok 285 - thread_rwlock
ok 286 - thread_rwlock_trylock
ok 287 - thread_create
ok 288 - thread_equal
ok 289 - dlerror
ok 290 - ip4_addr
ok 291 - ip6_addr_link_local
ok 292 - queue_foreach_delete
PASS: test/run-tests
=============
1 test passed
=============

bnoordhuis
bnoordhuis approved these changes Jul 8, 2017
View reviewed changes
Copy link
Copy Markdown
Member

@bnoordhuis bnoordhuis left a comment

Choose a reason for hiding this comment

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

LGTM modulo nits.

Comment thread src/unix/aix.c Outdated
continue;

if (p->ifr_addr.sa_family == AF_INET6)
inet6 = 1;
Copy link
Copy Markdown
Member

@bnoordhuis bnoordhuis Jul 8, 2017

Choose a reason for hiding this comment

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

Simplify to inet6 = (p->ifr_addr.sa_family == AF_INET6)? That also lets you drop the assignment on line 1081.

Copy link
Copy Markdown
Contributor Author

@gireeshpunathil gireeshpunathil Jul 8, 2017

Choose a reason for hiding this comment

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

thanks, done.

Comment thread src/unix/aix.c Outdated
}

/* TODO: Retrieve netmask using SIOCGIFNETMASK ioctl */
memset(address->phys_addr, 0, sizeof(address->phys_addr));
Copy link
Copy Markdown
Member

@bnoordhuis bnoordhuis Jul 8, 2017

Choose a reason for hiding this comment

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

Dead memset.

Copy link
Copy Markdown
Contributor Author

@gireeshpunathil gireeshpunathil Jul 8, 2017

Choose a reason for hiding this comment

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

removed.

Comment thread src/unix/aix.c
sa_addr = (struct sockaddr_dl*) &p->ifr_addr;
memcpy(address->phys_addr, LLADDR(sa_addr), sizeof(address->phys_addr));

if (ioctl(sockfd, SIOCGIFNETMASK, p) == -1) {
Copy link
Copy Markdown
Member

@bnoordhuis bnoordhuis Jul 8, 2017

Choose a reason for hiding this comment

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

Is there a plausible scenario where this could fail?

Copy link
Copy Markdown
Contributor Author

@gireeshpunathil gireeshpunathil Jul 8, 2017

Choose a reason for hiding this comment

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

There is no documented evidence about ioctl on SIOCGIFNETMASK could fail in any AIX versions due to known functional limitations but based on the runtime characteristics, the usual suspects apply:
EAGAIN: Unable to allocate buffers for the output (p).
EBADF: Bad file descriptor (sockfd)
EINVAL: invalid pointer (in p)
EINTR: system call was interrupted
...

@gireeshpunathil gireeshpunathil force-pushed the aix-port-netmask-macaddr branch from c523e1a to de4e15a Compare July 8, 2017 14:59
santigimeno
santigimeno approved these changes Jul 9, 2017
View reviewed changes
Copy link
Copy Markdown
Member

@santigimeno santigimeno left a comment

Choose a reason for hiding this comment

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

LGTM with a couple of nits

Comment thread src/unix/aix.c Outdated
address->address.address6 = *((struct sockaddr_in6*) &p->ifr_addr);
} else {
address->address.address4 = *((struct sockaddr_in*) &p->ifr_addr);
}
Copy link
Copy Markdown
Member

@santigimeno santigimeno Jul 9, 2017

Choose a reason for hiding this comment

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

style: while you're here you could remove the braces

Copy link
Copy Markdown
Contributor Author

@gireeshpunathil gireeshpunathil Jul 9, 2017

Choose a reason for hiding this comment

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

sure, thanks. removed.

Comment thread src/unix/aix.c Outdated
address->netmask.netmask6 = *((struct sockaddr_in6*) &p->ifr_addr);
} else {
address->netmask.netmask4 = *((struct sockaddr_in*) &p->ifr_addr);
}
Copy link
Copy Markdown
Member

@santigimeno santigimeno Jul 9, 2017

Choose a reason for hiding this comment

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

And here

Copy link
Copy Markdown
Contributor Author

@gireeshpunathil gireeshpunathil Jul 9, 2017

Choose a reason for hiding this comment

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

done.

@gireeshpunathil
aix: add netmask, mac address into net interfaces
d6a4994 
uv_interface_addresses API extracts the network interface entries.
In AIX, this was not fully implemented. retrieve the network mask and
the mac addresses.

Fixes: nodejs/node#14119
@gireeshpunathil gireeshpunathil force-pushed the aix-port-netmask-macaddr branch from de4e15a to d6a4994 Compare July 9, 2017 17:18
cjihrig
cjihrig reviewed Jul 11, 2017
View reviewed changes
Copy link
Copy Markdown
Contributor

@cjihrig cjihrig left a comment

Choose a reason for hiding this comment

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

LGTM pending CI.

CI: https://ci.nodejs.org/view/libuv/job/libuv-test-commit/330/

bnoordhuis pushed a commit that referenced this pull request Jul 13, 2017
@gireeshpunathil @bnoordhuis
aix: add netmask, mac address into net interfaces
810377f 
uv_interface_addresses API extracts the network interface entries.
In AIX, this was not fully implemented. retrieve the network mask and
the mac addresses.

Fixes: nodejs/node#14119
PR-URL: #1410
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com
Reviewed-By: Santiago Gimeno <santiago.gimeno@gmail.com>
@bnoordhuis
Copy link
Copy Markdown
Member

bnoordhuis commented Jul 13, 2017

Cheers Gireesh, landed in 810377f.

@bnoordhuis bnoordhuis closed this Jul 13, 2017
@gireeshpunathil
Copy link
Copy Markdown
Contributor Author

gireeshpunathil commented Jul 14, 2017

thank you!

@viktor-ku
Copy link
Copy Markdown

viktor-ku commented Sep 14, 2017

Could someone help me testing this?
I am here nodejs/node#14098

@bnoordhuis
Copy link
Copy Markdown
Member

bnoordhuis commented Sep 14, 2017

@kuroljov Can you be more specific? What do you need help with?

@viktor-ku
Copy link
Copy Markdown

viktor-ku commented Sep 14, 2017

nodejs/node@1c142ff#diff-c7afac570407fa8ca5c2298e0b7ac4b9R173
There is aix case which we skipped because it wasn't implemented. But now it seems like it is. So is there any aix specific output from interfaces that should be tested?

@bnoordhuis
Copy link
Copy Markdown
Member

bnoordhuis commented Sep 14, 2017

No, it should work like other platforms now.

@viktor-ku
Copy link
Copy Markdown

viktor-ku commented Sep 14, 2017

@bnoordhuis thanks

@richardlau richardlau mentioned this pull request Sep 24, 2018
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@cjihrig cjihrig cjihrig left review comments

@bnoordhuis bnoordhuis bnoordhuis approved these changes

@santigimeno santigimeno santigimeno approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@gireeshpunathil @bnoordhuis @viktor-ku @santigimeno @cjihrig