Hi Jeremy,<div><br></div><div>I have looked at your patch and we will not include it as it is now.</div><div><br></div><div>My comments:</div><div><br></div><div>The task to check whether a data packet in this case encoded as PER or BER is complete should be done on a higher level instead of</div>
<div>cluttering all the code with these checks. I think that this is already done in most implementations of protocols. What else would you do? Take a chance and </div><div>try to decode and try again if you get error, incomplete? I don't think so.</div>
<div><br></div><div>I think it is always possible to check that you have got the whole PDU before starting to decode the data. Then the decoding routines can just assume</div><div>that you have all the data.</div><div><br>
</div><div>I am not even sure that it is possible to determine in all places if a badmatch is because of incomplete data or faulty data.</div><div><br></div><div>Another note is that the module asn1rt_ber_bin will soon be obsoleted and asn1rt_ber_bin_v2 will be the default. In that module it is possible to do a generic check </div>
<div>for incomplete, and there are also NIF's involved which can get out of data and should the return incomplete.</div><div>But you could not know that asn1rt_ber_bin will be obsoleted soon.</div><div><br></div><div>
Even if I am not convinced of the need of this check I might accept it if it has no negative impact on performance and does not increase the size of</div><div>runtime modules and generated code significantly, and if it really is safe to tell that it failed because of incomplete data.</div>
<div><br></div><div>/Regards Kenneth, Erlang/OTP Ericsson<br><br><div class="gmail_quote">On Tue, Jul 24, 2012 at 5:15 PM, Jeremy Heater <span dir="ltr"><<a href="mailto:jeremy.heater@mobile-devices.fr" target="_blank">jeremy.heater@mobile-devices.fr</a>></span> wrote:<br>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">Hello all,<br>
<br>
We had already broached the subject a few years back (cf.<br>
<a href="http://erlang.org/pipermail/erlang-patches/2009-December/000627.html" target="_blank">http://erlang.org/pipermail/erlang-patches/2009-December/000627.html</a>) and<br>
we modified our approach to take the comments made at the time into<br>
account. Here follows a description of the changes:<br>
<br>
This patch's goal is to allow the user to determine if an incoming<br>
asn.1 packet is incomplete.<br>
To that end, the decode/2 functions of generated asn.1 files were<br>
modified so as to return {error, incomplete} when the packet to be<br>
decoded is incomplete instead of the generic<br>
{error, {asn1, {badmatch, _}}} (or badarg) which is not distinguishable<br>
from other errors.<br>
<br>
This would seem to us to be a necessary feature so as to be able to<br>
determine if a packet received from a socket is complete (since overly<br>
large messages are split).<br>
It is also complementary to the undec_rest option where when decode/2<br>
is called on a packet containing more than one message, the function<br>
returns the first message decoded and the rest of the packet<br>
undecoded.<br>
<br>
The solution was implemented for all encoding rules, even though BER<br>
encoding allows for simpler checking by manually parsing the first<br>
bytes of the packet, for the sake of having a consistent API whatever<br>
the encoding.<br>
<br>
The proposed patch can be found at:<br>
git fetch git://<a href="http://github.com/Kwisatx/otp.git" target="_blank">github.com/Kwisatx/otp.git</a> incomplete_packet_asn1<br>
<br>
<a href="https://github.com/Kwisatx/otp/compare/master...incomplete_packet_asn1" target="_blank">https://github.com/Kwisatx/otp/compare/master...incomplete_packet_asn1</a><br>
<a href="https://github.com/Kwisatx/otp/compare/master...incomplete_packet_asn1.patch" target="_blank">https://github.com/Kwisatx/otp/compare/master...incomplete_packet_asn1.patch</a><br>
<br>
<br>
Since the added tests inevitably add some overhead, we also tried to<br>
link the changes to the undec_rest option<br>
(incomplete_packet_asn1_duplicate_files branch), but since most of the<br>
changes are in the runtime, it made duplicating the code necessary (as<br>
the name of the branch suggests), which complicates the code.<br>
<br>
What are your thoughts on this change? Any ideas for how to improve it?<br>
<br>
Best regards,<br>
<br>
Jeremy Heater.<br>
_______________________________________________<br>
erlang-patches mailing list<br>
<a href="mailto:erlang-patches@erlang.org">erlang-patches@erlang.org</a><br>
<a href="http://erlang.org/mailman/listinfo/erlang-patches" target="_blank">http://erlang.org/mailman/listinfo/erlang-patches</a><br>
</blockquote></div><br></div>