Added support for AIX platform (for issue #5) #9

patrickhrastnik · 2019-02-13T11:45:13Z

The script aix.js is compatible with AIX on PPC64 architecture. If some time in future AIX on another architecture should be supported, then add separate code paths, depending on the architecture.

Solves issue #5

The script aix.js is compatible with AIX on PPC64 architecture. If some time in future AIX on another architecture should be supported, then add separate code paths, depending on the architecture.

silverwind · 2019-02-13T17:00:09Z

I'm really worried about adding a C++ dependency, even if optional because if I remember correctly package installation can under some circumstances fail if a optional dependency fails to build. I assume you looked around if there is a pure JS alternative? Can we be sure that a build it not attempted if platform is not AIX?

silverwind · 2019-02-13T17:06:48Z

aix.js

+
+const getSqlStatement = family => {
+  let sql =
+    "select NEXT_HOP from QSYS2.NETSTAT_ROUTE_INFO where ROUTE_TYPE='DFTROUTE'";


Does this table possibly also contain the name of the network interface of the default gateway? If so, we can fill the interface property of the result with it, if its present.

According to this it might be LOCAL_BINDING_INTERFACE but it says it will be null for IPv6, so not sure if worth to add if it only works for IPv4.

I‘m gonna have a look on it tomorrow. If it‘s null, I‘m gonna make sure it‘s at least an empty string, using coalesce, or maybe there‘s some other way round.

Yeah result should be non-empty string or null. Guess it does have some value to have it at least in the IPv4 case.

I checked the LOCAL_BINDING_INTERFACE, and it contains, indeed, the interface address. So I added it.

silverwind · 2019-02-13T17:20:29Z

aix.js

+
+      if (results && results[0] && results[0].NEXT_HOP) {
+        resolve({
+          gateway: results[0].NEXT_HOP


Documentation says that the special value *DIRECT for NEXT_HOP is "the next hop value of a route that is automatically created.".

Maybe this special value should be handled similar to the Linux case of default via dev? I realize that returning the matching interface address is not totally correct (practically, there is no default gateway address an route via interface), but it's better than failing, I suppose.

Alternatively, we could just fail if *DIRECT is encountered.

As far as I‘ve seen in the database, there is an entry with value *DIRECT, that‘s why I filter on ROUTE_TYPE=‘DFTROUTE‘

Maybe add NEXT_HOP!='*DIRECT' to the query as a safeguard, if it is necessary.

Added the NEXT_HOP filter

patrickhrastnik · 2019-02-13T17:28:17Z

I tried everything me and my boss could come up with to gather the information using vanilla js, but no other approach worked :/ I understand your worries. Would it be possible to manage this situation with a pre-install script that only installs idb-connector if it‘s aix on ppc64? I have no such experience, but I read somewhere there‘s an event hook for this...
Another possibility I have to figure out: It may be possible to require the package directly from the system. There‘s supposed to be some directory the package is installed globally.

silverwind · 2019-02-13T17:39:40Z

It may actually be fine. The package specifies os in package.json and if npm documentation is to be trusted, this means the package won't install on other platforms. Still, I will inspect their build conditions more closely. A shame that node packages can not have platform-specific dependencies easily.

silverwind · 2019-02-13T17:53:31Z

aix.js

+
+const getGatewayInformationAsync = async family => {
+  return new Promise((resolve, reject) => {
+    const idbConnector = require("idb-connector");


You should try ... catch these require statements and return the "Unable to determine default gateway" error on catch. Normally I'd say to let the error through to the consumer, but the module has been consistent in returning only this specific error.

I wrapped the connector functions as well

Changed sql statement to only return entries where NEXT_HOP!=*DIRECT Changed sql statement to return LOCAL_BINDING_INTERFACE as well Changed return values to contain "interface" as well (fallback: empty string) Wrapped all require's and database functions to throw especially "Unable to determine..." in error case

patrickhrastnik · 2019-02-14T10:03:13Z

In terms of the optional dependency on idb-connector:
According to npm, optional dependencies don't cause the installation to fail if they don't build.
And the idb-connector states in it's package.json "os": {"aix"} as well. So there shouldn't be an issue with this...
Referencing a global package may cause some trouble in case the relative path to the package changes, and it might depend on the system as well, where you can find the package. And, as well, I couldn't find the global package path on our system.
Is it okay for you if we leave it like this, or do you insist on finding another solution?

silverwind

Looking good. Is this latest version properly tested? Does it work for unpriviledged users?

silverwind · 2019-02-14T16:58:54Z

Referencing a global package may cause some trouble in case the relative path to the package changes

Not sure what you mean. We only have local dependencies (in node_modules), what could be the issue?

Is it okay for you if we leave it like this, or do you insist on finding another solution?

It's fine as it is.

patrickhrastnik · 2019-02-14T17:04:16Z

I tested it on our aix (as/400) system and on my windows machine. Didn‘t cause problems as far as I was able to test it. I assumed the Travis CI takes care of the other systems (Linux etc) as I have no Linux machine available.

silverwind · 2019-02-14T17:24:22Z

aix.js

+        if (results && results[0] && results[0].NEXT_HOP) {
+          resolve({
+            gateway: results[0].NEXT_HOP,
+            interface: results[0].LOCAL_BINDING_INTERFACE || ""


Does the query return a actual null for LOCAL_BINDING_INTERFACE in the IPv6 case?

Since it‘s a system table, it might be null. That‘s why I put it like this, just in case.

Yes, but I want to know if the null in the docs transform to a actual JavaScript null value or maybe 'null' or other weird things.

Oh, this is what you mean.
According to the source of idb-connector dbstmt.cc ist translates SQL_NULL_DATA to an actual null value
See dbstmt.cc lines 344-345

Thanks for digging that out!

silverwind · 2019-02-14T17:25:44Z

Travis isn't actually in use (e.g. tests are skipped) because last I checked, their VMs did not have network interfaces, but that may have changed recently.

patrickhrastnik · 2019-02-14T18:07:47Z

Okay, so we need to run separate tests on other systems manually as well?
Do you have systems you can test the package on?

There changes shouldn‘t interfer with the previous code at all, since I put it into a separate file... but ofc, a package used that much needs to be tested properly.

silverwind · 2019-02-14T19:20:38Z

No need to run tests elsewhere, the change can possibly only affect AIX. I will look into enabling tests on travis again later.

patrickhrastnik · 2019-02-15T12:39:29Z

Is there anything left to do for me?

silverwind · 2019-02-15T12:48:13Z

I merged it to master now, with additional tweaks in f7903f5. Can you please test current master on your machine?

patrickhrastnik · 2019-02-15T14:34:35Z

Tested -> works as expected

Added support for AIX platform

f32b881

The script aix.js is compatible with AIX on PPC64 architecture. If some time in future AIX on another architecture should be supported, then add separate code paths, depending on the architecture.

silverwind reviewed Feb 13, 2019

View reviewed changes

silverwind approved these changes Feb 14, 2019

View reviewed changes

silverwind reviewed Feb 14, 2019

View reviewed changes

silverwind merged commit 0680f5b into silverwind:master Feb 15, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Added support for AIX platform (for issue #5) #9

Added support for AIX platform (for issue #5) #9

patrickhrastnik commented Feb 13, 2019

silverwind commented Feb 13, 2019

silverwind Feb 13, 2019 •

edited

Loading

silverwind Feb 13, 2019 •

edited

Loading

patrickhrastnik Feb 13, 2019

silverwind Feb 13, 2019

patrickhrastnik Feb 14, 2019

silverwind Feb 13, 2019 •

edited

Loading

patrickhrastnik Feb 13, 2019

silverwind Feb 13, 2019 •

edited

Loading

patrickhrastnik Feb 14, 2019

patrickhrastnik commented Feb 13, 2019 •

edited

Loading

silverwind commented Feb 13, 2019 •

edited

Loading

silverwind Feb 13, 2019

patrickhrastnik Feb 14, 2019

patrickhrastnik commented Feb 14, 2019

silverwind left a comment

silverwind commented Feb 14, 2019

patrickhrastnik commented Feb 14, 2019

silverwind Feb 14, 2019

patrickhrastnik Feb 14, 2019

silverwind Feb 14, 2019

patrickhrastnik Feb 15, 2019

silverwind Feb 15, 2019

silverwind commented Feb 14, 2019

patrickhrastnik commented Feb 14, 2019

silverwind commented Feb 14, 2019 •

edited

Loading

patrickhrastnik commented Feb 15, 2019

silverwind commented Feb 15, 2019

patrickhrastnik commented Feb 15, 2019

Added support for AIX platform (for issue #5) #9

Added support for AIX platform (for issue #5) #9

Conversation

patrickhrastnik commented Feb 13, 2019

silverwind commented Feb 13, 2019

silverwind Feb 13, 2019 • edited Loading

Choose a reason for hiding this comment

silverwind Feb 13, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

silverwind Feb 13, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

silverwind Feb 13, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

patrickhrastnik commented Feb 13, 2019 • edited Loading

silverwind commented Feb 13, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

patrickhrastnik commented Feb 14, 2019

silverwind left a comment

Choose a reason for hiding this comment

silverwind commented Feb 14, 2019

patrickhrastnik commented Feb 14, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

silverwind commented Feb 14, 2019

patrickhrastnik commented Feb 14, 2019

silverwind commented Feb 14, 2019 • edited Loading

patrickhrastnik commented Feb 15, 2019

silverwind commented Feb 15, 2019

patrickhrastnik commented Feb 15, 2019

silverwind Feb 13, 2019 •

edited

Loading

silverwind Feb 13, 2019 •

edited

Loading

silverwind Feb 13, 2019 •

edited

Loading

silverwind Feb 13, 2019 •

edited

Loading

patrickhrastnik commented Feb 13, 2019 •

edited

Loading

silverwind commented Feb 13, 2019 •

edited

Loading

silverwind commented Feb 14, 2019 •

edited

Loading