Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Add GetIfEntry, GetNetworkParams, and supporting IPHlpAPI structures #983

Closed
wants to merge 2 commits into from

Conversation

dbwiddis
Copy link
Contributor

@dbwiddis dbwiddis commented Jul 1, 2018

No description provided.

@dbwiddis dbwiddis force-pushed the iphlpapi branch 4 times, most recently from 6d925b3 to 482b0d3 Compare July 2, 2018 15:53
"InUcastOctets", "InMulticastOctets", "InBroadcastOctets", "OutOctets", "OutUcastPkts", "OutNUcastPkts",
"OutDiscards", "OutErrors", "OutUcastOctets", "OutMulticastOctets", "OutBroadcastOctets", "OutQLen" })
class MIB_IF_ROW2 extends Structure {
public long InterfaceLuid; // 64-bit union
Copy link
Member

Choose a reason for hiding this comment

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

Is this safe to use the raw value here instead of the union?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's safe, as it's 64-bits. The alternative would be a multi-layer NET_LUID union in JNA with one alternative being a 64-bit Value (the intended return) and the actual information encoded in 5 of the 8 bytes. See https://msdn.microsoft.com/library/windows/hardware/ff568747 . Happy to try to add that NET_LUID structure if I can figure out how; but at this layer it's possible for it to be returned and then parsed by the user with shifting and bitmasks.

Copy link
Member

Choose a reason for hiding this comment

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

I agree, retrieving the correct value can be implemented as a helper method on the structure, if needed.

"OutDiscards", "OutErrors", "OutUcastOctets", "OutMulticastOctets", "OutBroadcastOctets", "OutQLen" })
class MIB_IF_ROW2 extends Structure {
public long InterfaceLuid; // 64-bit union
public ULONG InterfaceIndex;
Copy link
Member

Choose a reason for hiding this comment

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

Would int be an alternative? (applies to other definitions as well).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Also applies to some of my other PRs. As a structure mapping it would work, however, I have tried (not always consistently) to use the mapped type when variables are unsigned. I've gone back and forth a few times on many of these... using int, we require the user to know to interpret the unsigned value, while ULONG or DWORD offer longvalue() options.

Copy link
Member

Choose a reason for hiding this comment

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

Your argument is sound - the question was really meant as a question, so if you think it would be better to bind it as a wrapped value, I'm ok with it too. The benefit I see in the int is, that it is the "natural" integral number type in java and thus easier to deal with.
If you want to switch back to DWORD, feel free to do so - please indicate your preference, I'll hold of merging till then.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's ok to leave it as int here, because the javadoc makes it clear what the underlying value is and there's likely a tiny performance improvement to avoiding the extra object creation. As I said I went back and forth on this (I literally did search-and-replace, and then undo, at least twice) before submitting.

I might suggest a Global JNA util method that converts unsigned values, though, so users don't have to always reinvent the wheel.

*/
@FieldOrder({ "Next", "IpAddress", "IpMask", "Context" })
class IP_ADDR_STRING extends Structure {
public ByReference Next;
Copy link
Member

Choose a reason for hiding this comment

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

Please qualify this to IP_ADDR_STRING.ByReference I had to look multiple times.

@dbwiddis
Copy link
Contributor Author

dbwiddis commented Jul 3, 2018

Made the changes requested except left the NET_LUID union as a long. I'm open to a byte[8] instead if it'd be more clear. I don't think there's much value in coding up the NET_LUID union itself.

@matthiasblaesing
Copy link
Member

matthiasblaesing commented Jul 4, 2018

I merged the three changesets via:

2287b92

Thank you for your contribution.

# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants