Compilation fixes and memory corruption prevention #8
Conversation
… build, and fix a pile of incorrect strncpy() usage that expects it to behave saner than it actually does.
|
Should mention that this also fixes a few GCC warnings. |
|
Symlinks are fixed, and I should add that this code is tested as working and makes it possible to build the Linux driver from this repository. :^) |
|
@Subsentient Would you mind splitting your change for the memory corruption into a separate PR so our engineering team can review the proposed changes in isolation from the other changes suggested here? Thanks. Regarding the missing driver build files, I just pushed a branch I'd been working on this week which addresses this. With regards to building the kernel module with CMake, that work hadn't been finished yet but is something we do want to provide. I just hadn't gotten to adding the build target for it to the CMakeLists, and testing the resultant module. As it currently exists the |
|
After investigating actually building the kernel module via CMake, it is not as straightforward as it may seem. Kernel module builds use the Kbuild system. Trying to adapt the building of kernel modules to CMake effectively entails calling This was all new to me as I'm not the author of the existing AJA NTV2 kernel module, and am learning these details for the first time here. I found this very basic example which demonstrates this: https://gitlab.com/christophacham/cmake-kernel-module |
|
Sorry for the delay, busy with work for my employer. I can try that if absolutely necessary, but other than the added missing files, I think it should take the engineering team all of 5 minutes to read these commits. As a result, I'm requesting that you merge this anyways. Thanks! |
| @@ -990,7 +990,7 @@ bool CNTV2DriverInterface::BitstreamStatus (NTV2ULWordVector & outRegValues) | |||
| void CNTV2DriverInterface::FinishOpen (void) | |||
| { | |||
| // HACK! FinishOpen needs frame geometry to determine frame buffer size and number. | |||
| NTV2FrameGeometry fg; | |||
| NTV2FrameGeometry fg{}; | |||
There was a problem hiding this comment.
This initializer won't work if the library is built without C++11 compiler support.
There was a problem hiding this comment.
That's true, but I thought new versions of NTV2 were C++11+ only?
There was a problem hiding this comment.
Believe me, we'd love to pull that trigger, but we have a surprising number of customers that are stuck with older hardware and operating systems yet still need to use our newer hardware. In SDK 16.0, we changed to assume (by default) C++11 support, but the library can be built without it (by commenting out the lines defining the NTV2_USE_CPLUSPLUS11 and AJA_USE_CPLUSPLUS11 macros).
There was a problem hiding this comment.
Because of the reason Mr. Bill mentioned, in my own code I have preferred initializing NTV2 enum types to either their equivalent zero-value, or their UNKNOWN or INVALID value, ex:
NTV2FrameGeometry fg = NTV2_FG_1920x1080; // default 0-value for NTV2FrameGeometry
NTV2VideoFormat vf = NTV2_FORMAT_UNKNOWN; // default 0-value for NTV2VideoFormat
NTV2PixelFormat pf = NTV2_FBF_FIRST; // default 0-value for NTV2FrameBufferFormat/NTV2PixelFormat
There was a problem hiding this comment.
@mrbillaja Ahh, okay, I understand. That is unfortunate but understandable. In that case you can either do that or for other structs with less well-defined default values, it's often a safe bet to just do struct MyStruct Name = { 0 }; since 0 is also acceptable as an initializer for pointers and floats (though implicitly converted), and the compiler will zero out the rest as long as something is initialized explicitly.
| @@ -175,8 +175,10 @@ bool CNTV2Card::LoadDynamicDevice (const NTV2DeviceID inDeviceID) | |||
| currentBitfileVersion = 0xff; // ignores bitfile version | |||
| } | |||
|
|
|||
| if (!currentDesignID) | |||
| {DDFAIL("Current design ID is zero for " << oldDevName); return false;} | |||
| @@ -244,8 +244,12 @@ NTV2NubPkt * BuildNubBasePacket (NTV2NubProtocolVersion protocolVersion, | |||
| pPkt->hdr.dataLength = dataSize; | |||
|
|
|||
| char *p (reinterpret_cast<char*>(pPkt->data)); // Work around ISO C++ forbids zero-size array | |||
| ULWord len (ULWord(::strlen(queryRespStr)) + 1); | |||
There was a problem hiding this comment.
This code module is disappearing in 16.3.
| @@ -283,7 +283,7 @@ void ConvertLinetoRGB(UByte * ycbcrBuffer, | |||
| bool fUseSDMatrix, | |||
| bool fUseSMPTERange) | |||
| { | |||
| YCbCrAlphaPixel ycbcrPixel; | |||
| YCbCrAlphaPixel ycbcrPixel{}; | |||
There was a problem hiding this comment.
Willl this initializer work if the library is built without C++11 compiler support?
So this is rolled into one commit, I tried to keep the working tree as clean as possible.
The following is done here:
NOTE: Apparently CMake build of the kernel module is still fouled up, looking at the CMakeLists.txt it looks like you're missing the .ko target. So, in the meantime, the Makefile stuff works.
Thanks!