Missing NULL test of NTNDArray::wrap()

[mdavidsaver]

NTNDArray::wrap() can return NULL. If for some reason it does, eg. I give it a somehow "incompatible" definition, then SIGSEGV follows.

https://github.com/areaDetector/pvaDriver/blob/e0819d25620eaf467b81a25bc16f0f639ae6ddb5/pvaDriverApp/src/pvaDriver.cpp#L244-L258

Also, it is bad form to swallow exception messages like this. I see no reason to catch anything other than std::exception. If you ever find that PVD or PVA throws something which doesn't derive from std::exception, then please report this as a bug.

[mdavidsaver]

So I found which of the ~30 conditions in epics::nt::NTNDArray::isCompatible() were causing it to return false...

As the (somewhat unwilling) maintainer of normativeTypesCPP i have to ask. Would you be willing to stop using NTNDArray in NTNDArrayConverter? Every time I look at the NTCPP code, I find my finger drifting towards Del.

[MarkRivers]

I don't know enough about NTNDArrayConverter to answer this. Hopefully @brunoseivam can respond.

[brunoseivam]

Hi Michael,

What would it be replaced with? "Hand"-built NTNDArray definitions? If that's the case, I worry that other users of normative types will have to keep track of the types by themselves.

As far as I remember, the code that you quoted assumes that wrap will succeed because isCompatible succeeded before. Is the type changing in between monitor updates? I never thought about this case.

[brunoseivam]

@MarkRivers could you please give me write access to this repo?

[MarkRivers]

I just gave the Developer team write access, so you should be able to write to it now.

[mdavidsaver]

What would it be replaced with? "Hand"-built NTNDArray definitions

In case there is any confusion I'd like to scrap NTNDArray, but not NTNDArrayBuilder which contains the Structure definition. I see the benefit of having a reference definition.

Is the type changing in between monitor updates?

Well it certainly can change. In this case though the issues turned out to be that 'codec' needs to be a sub-structure instead of an array of structures, and the 'dataTimeStamp' field was missing.

https://github.com/mdavidsaver/p4p/blob/d694a998bc24aaa6cf4c637212aef294ff5bedef/src/p4p/nt/ndarray.py#L101-L104

[brunoseivam]

I see what you're saying. Do you want to get rid of all wrapper classes? When I was writing the converters I thought the opposite: that the wrappers should be made more ergonomic, e.g. array.getUniqueId() instead of array.getUniqueId()->get().

But I get where you are coming from

[brunoseivam]

Fixed via #9

[mdavidsaver]

Do you want to get rid of all wrapper classes? When I was writing the converters I thought the opposite: that the wrappers should be made more ergonomic

I see what is there now as trap for the inexperienced, and "Del" is what I have time for. I think you are the only one to use NTNDArray in anger, which leaves you better positioned that I am to say what would make a helpful wrapper.

Technically, what I'd like to see is a move away from the ridged and opaque NTNDArray::isCompatible(), and instead have the individual accessors test for errors. In short, replace the dangerous getSubField(...)->get() with the safe getSubFieldT(...)->get(). Did you spot the 'T'?

This way errors, in the form of exceptions, are generated only when something essential is missing. And there will be a message to gives some clue as to what the missing piece may be.

[brunoseivam]

One aspect of having a wrapper / isCompatible that I like is checking for type compatibility only once, at the "network boundary". Currently, once an NTNDArray makes it into pvaDriver past isCompatible, it can be assumed to be valid one, so the subsequent code doesn't have to worry about exceptions when accessing its non-optional fields. However, I agree that the current design makes debugging a pain. Maybe wrap could throw instead of returning NULL (wrapT?), and/or maybe there could be a reporting function that lists the missing fields that it expected.

Of course, that's development effort, and I understand the position you're in right now.

[brunoseivam]

I am closing this issue since the specific problem has been addressed