Fix stuct packing in build_bcm_header on 32-bit

[karlding]

Previously build_bcm_header relied upon adding a "0q" at the end of the
struct format string in order to force a 8-byte wide alignment on the
struct, in order to interop with the structure defined in the Linux
headers:

struct bcm_msg_head {
  __u32 opcode;
  __u32 flags;
  __u32 count;
  struct bcm_timeval ival1, ival2;
  canid_t can_id;
  __u32 nframes;
  struct can_frame frames[0];
};

The Python code assumed that alignof(long long) == 8 bytes in order to
accomplish this. However, on certain 32-bit platforms, this assumption
is not true, and alignof(long long) == 4.

As the struct module does not provide logic to derive alignments of its
types, this changes the packing logic to use ctypes. This exposes the
alignment of the struct members, allowing us to determine whether padding
bytes are necessary.

Fixes #470

[codecov]

Codecov Report

Merging #573 into develop will increase coverage by 0.17%.
The diff coverage is 90.9%.

@@             Coverage Diff             @@
##           develop     #573      +/-   ##
===========================================
+ Coverage    63.99%   64.16%   +0.17%     
===========================================
  Files           63       63              
  Lines         5699     5718      +19     
===========================================
+ Hits          3647     3669      +22     
+ Misses        2052     2049       -3

[karlding]

I've tested this by installing can-utils and running the examples/cyclic.py demo and observing SocketCAN with candump on the following:

• Ubuntu 18.04 LTS (64-bit)
• Debian Stretch (32-bit)
• Ubuntu 16.04 LTS (32-bit)
• Raspbian Stretch

[karlding]

Oh, I also had tried writing a unit test by mocking the return values of ctypes.sizeof and ctypes.alignment to emulate what we'd see on these platforms, and then checking the size and alignment of the final structure (as returned from the factory function), but I was having some trouble getting it to correctly work after patching the functions.

I ended up hacking around it by poking at the _fields_ class attribute and comparing the _fields_ to an expected result. I feel like that's not ideal though, but I couldn't seem to get ctypes.sizeof and ctypes.alignof to work otherwise.

It looks something like this:

import unittest

try:
    from unittest.mock import Mock
    from unittest.mock import patch
    from unittest.mock import call
except ImportError:
    from mock import Mock
    from mock import patch
    from mock import call

import ctypes

from can.interfaces.socketcan.socketcan import bcm_header_factory


class BcmMsgHeadTest(unittest.TestCase):
    def setUp(self):
        self._ctypes_sizeof = ctypes.sizeof
        self._ctypes_alignment = ctypes.alignment

    @patch("ctypes.sizeof")
    @patch("ctypes.alignment")
    def test_bcm_header_factory_32_bit_sizeof_long_4_alignof_long_4(
        self, ctypes_sizeof, ctypes_alignment
    ):
        def side_effect_ctypes_sizeof(value):
            if value == ctypes.c_long:
                return 4
            return self._ctypes_sizeof(value)

        def side_effect_ctypes_alignment(value):
            if value == ctypes.c_long:
                return 4
            return self._ctypes_alignment(value)

        ctypes_sizeof.side_effect = side_effect_ctypes_sizeof
        ctypes_alignment.side_effect = side_effect_ctypes_alignment

        fields = [
            ("opcode", ctypes.c_uint32),
            ("flags", ctypes.c_uint32),
            ("count", ctypes.c_uint32),
            ("ival1_tv_sec", ctypes.c_long),
            ("ival1_tv_usec", ctypes.c_long),
            ("ival2_tv_sec", ctypes.c_long),
            ("ival2_tv_usec", ctypes.c_long),
            ("can_id", ctypes.c_uint32),
            ("nframes", ctypes.c_uint32),
        ]
        BcmMsgHead = bcm_header_factory(fields)

        # TODO: Figure out if we can just use ctypes.sizeof here?
        expected_fields = [
            ("opcode", ctypes.c_uint32),
            ("flags", ctypes.c_uint32),
            ("count", ctypes.c_uint32),
            ("ival1_tv_sec", ctypes.c_long),
            ("ival1_tv_usec", ctypes.c_long),
            ("ival2_tv_sec", ctypes.c_long),
            ("ival2_tv_usec", ctypes.c_long),
            ("can_id", ctypes.c_uint32),
            ("nframes", ctypes.c_uint32),

            # We expect 4 bytes of padding
            ("pad_0", ctypes.c_uint8),
            ("pad_1", ctypes.c_uint8),
            ("pad_2", ctypes.c_uint8),
            ("pad_3", ctypes.c_uint8),
        ]
        self.assertEqual(expected_fields, BcmMsgHead._fields_)

    # And then a similar test for 64-bit results

if __name__ == "__main__":
    unittest.main()

Would you want me to add this somewhere?

[hardbyte]

Consider calling the factory once at import time or socketcan bus instantiation time.

[karlding]

Done

[hardbyte]

Your test would be a good addition too. I assume your testing was with Python 2 and 3?

We could also see if we can carry out testing on 32bit systems on travis.

[karlding]

Sure!

AFAIK, Travis only supports 64-bit, so presumably the only way to get 32-bit runners is to run the tests in a 32-bit container via Docker. Not sure if that's something we want to do as part of this change?

[karlding]

Re-tested using the cyclic example on the same platforms using a variant of Python 2 and 3 for each, and it doesn't seem like I've broken anything. 😂

Would you like me to squash my commits? Or would you rather do that when merging (or not at all)?

[hardbyte]

Thanks for fixing this 👍