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

Add Linux LibC with sysinfo #997

Merged
merged 3 commits into from
Aug 22, 2018

Conversation

dbwiddis
Copy link
Contributor

@dbwiddis dbwiddis commented Aug 7, 2018

Not sure if it's appropriate to put linux under unix. Could incur the wrath of GNU. :)

@dbwiddis dbwiddis force-pushed the linux-libc branch 3 times, most recently from 1dc3a25 to 1d8b366 Compare August 7, 2018 03:14
// but we are not allowed to declare an array of size zero. It's safe to
// declare extra padding in the structure; these bytes simply will not
// be written on 64-bit systems.
public byte[] _f = new byte[8]; // padding to 64 bytes
Copy link
Contributor

Choose a reason for hiding this comment

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

You could dynamically set this based on Native.LONG_SIZE, although I'm not entirely certain zero-length arrays handling is bug-free.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Would it work to put a conditional into the FieldOrder and omit the _f if the padding is 0? It presently throws an exception if the padding calculation results in "new byte[0]"

Copy link
Member

Choose a reason for hiding this comment

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

zero-length arrays are not supported by JNA. There is a guard in Native#getNativeSize that rejects zero length arrays. From what I read, C does not allow for zero length arrays, so the JNA bindings is in line.

But in this case the array is just padding - what I would suggest:

  • allow com.sun.jna.Structure.calculateSize(boolean force, boolean avoidFFIType) to be overriden by subclasses
  • override it in Sysinfo like so:
        @Override
        protected int calculateSize(boolean force, boolean b) {
            int superValue = super.calculateSize(force, b);
            if(superValue == CALCULATE_SIZE) {
                return superValue;
            } else {
                return superValue + (20 - 2 * NativeLong.SIZE - 4);
            }
        }

@twall what do you think? I aggree, that for a single structure you could just over allocate, but if for a reason the structure is in an array, this might break.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Could you also do:

@FieldOrder(NativeLong.SIZE < 8 ? 
    { "uptime", "loads", "totalram", "freeram", "sharedram", "bufferram", "totalswap", 
             "freeswap", "procs", "totalhigh", "freehigh", "mem_unit", "_f" } :
    { "uptime", "loads", "totalram", "freeram", "sharedram", "bufferram", "totalswap",
             "freeswap", "procs", "totalhigh", "freehigh", "mem_unit" })

?

Copy link
Contributor

Choose a reason for hiding this comment

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

Is the field ever used? If not, I'd say go with @matthiasblaesing 's suggestion.

I don't think the field order tracking and public fields were ever designed to potentially have different lists, so dynamically setting the field order would require extra testing.

Copy link
Contributor

@twall twall Aug 12, 2018

Choose a reason for hiding this comment

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

I don't think you need to expose calculateSize(boolean, boolean); calculateSize(boolean) is already exposed for overriding.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The only concern about allocation size is if this structure was in an array. Structure.toArray() calls ensureAllocated() which then calls (private) ensureAllocated(false) which eventually calls (package private) calculateSize(true, avoidFFIType). So I can override calculateSize(boolean) but it is never called in toArray().

Copy link
Contributor

Choose a reason for hiding this comment

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

I see what you're saying. I can't think of a different refactor that would solve it, so calculateSize(boolean,boolean) should be protected instead of package private.

// but we are not allowed to declare an array of size zero. Here we
// declare the array but will override calculateSize to ensure the
// correct C side allocation occurs. These bytes are never read.
public byte[] _f = new byte[8]; // padding to 64 bytes
Copy link
Member

Choose a reason for hiding this comment

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

This is not correct. The target size on 32 bit is 64bytes. But is now 72. With the _f member the structure is 8 bytes to large, as the padding happens via the calculateSize and the member.

I would remove the _f member. The memory needs to allocated and be present, but it is not needed to be mapped.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oops. I just copied your block of code without checking it. The conditional should be based on NativeLong.SIZE rather than CALCULATE_SIZE. But also the array calculation, starting in the superclass, would stay in the superclass. We would have to override toArray() for that approach to work.

Leaving _f off would leave the size 8 bytes too short on 32-bit systems without the toArray() override.

Perhaps the best answer is two separate structures, a SysInfo and SysInfo64 and have the caller do a size check to decide which one to use...

@dbwiddis dbwiddis force-pushed the linux-libc branch 6 times, most recently from 56e34c5 to 6c39b7d Compare August 15, 2018 00:59
@matthiasblaesing
Copy link
Member

Ok - overriding calculateSize is useless, as the value is the already padded size. deriveLayout would need to be modified. But instead of modifying JNA core, I implemented @dbwiddis already mentioned: I override getFieldOrder and getFieldList to return a filtered list.

This should do it:

    @FieldOrder({ "uptime", "loads", "totalram", "freeram", "sharedram", "bufferram", "totalswap", "freeswap", "procs",
            "totalhigh", "freehigh", "mem_unit" })
    class Sysinfo extends Structure {
        private static final int PADDING_SIZE = 20 - 2 * NativeLong.SIZE - 4;

        public NativeLong uptime; // Seconds since boot
        // 1, 5, and 15 minute load averages
        public NativeLong[] loads = new NativeLong[3];
        public NativeLong totalram; // Total usable main memory size
        public NativeLong freeram; // Available memory size
        public NativeLong sharedram; // Amount of shared memory
        public NativeLong bufferram; // Memory used by buffers
        public NativeLong totalswap; // Total swap space size
        public NativeLong freeswap; // swap space still available
        public short procs; // Number of current processes
        public NativeLong totalhigh; // Total high memory size
        public NativeLong freehigh; // Available high memory size
        public int mem_unit; // Memory unit size in bytes
        public byte[] _f = new byte[PADDING_SIZE];

        /*
         getFieldList and getFieldOrder are overridden because PADDING_SIZE
         might be 0 - that is a GCC only extension and not supported by JNA

         The dummy field at the end of the structure is just padding and so if
         the field is the zero length array, it is stripped from the fields
         and field order.
         */
        @Override
        protected List<Field> getFieldList() {
            List<Field> fields = new ArrayList<Field>(super.getFieldList());
            if(PADDING_SIZE == 0) {
                Iterator<Field> fieldIterator = fields.iterator();
                while(fieldIterator.hasNext()) {
                    Field field = fieldIterator.next();
                    if("_f".equals(field.getName())) {
                        fieldIterator.remove();
                    }
                }
            }
            return fields;
        }

        @Override
        protected List<String> getFieldOrder() {
            List<String> fieldOrder = new ArrayList<String>(super.getFieldOrder());
            if(PADDING_SIZE > 0) {
                fieldOrder.add("_f");
            }
            return fieldOrder;
        }
    }

@dbwiddis
Copy link
Contributor Author

dbwiddis commented Aug 19, 2018

I thought of doing exactly that, but saw notes in the JNA source about possibly deprecating getFieldOrder() in the future, so decided it wasn't a good direction to go.

However, I think I did find an elegant solution, have you seen my latest commit? I just declare the 64 bit version without the padding field, and extend that class, adding the padding field, to create a 32-bit variant of the structure. As a subclass its fields can still be accessed using the superclass type.

@matthiasblaesing
Copy link
Member

getFieldOrder is deprecated, to get people to think about its use (the new annotation is preferred), I don't see it going away in the near future.

I saw your solution and indeed it is creative. However it leaks implementation details to the user. The suggested solution above has the benefit, that if JNA at some point supports zero length arrays, removing the two method implementations is trivial.

@matthiasblaesing
Copy link
Member

Thank you.

@matthiasblaesing matthiasblaesing merged commit 0aacf71 into java-native-access:master Aug 22, 2018
@dbwiddis dbwiddis deleted the linux-libc branch August 23, 2018 00:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants