[erlang-patches] Re: [Re] Patch for asn1 library

[Kenneth Lundin]

Hi Aude,

I did not notice your patch earlier possibly because I was not a
member of the erlang-patches mailing list at the time.
I am responsible for the ASN.1 application and have some comments, see below:

On Thu, Dec 17, 2009 at 10:16 AM, Aude Quintana
<aude.quintana@REDACTED> wrote:
> Hi !
> I received no acknowlegment concerning the patch I sent for the asn1
> library (sent in August)
> So in order to facilitate its treatment I have comitted it on github
> otp project clone. Hope you will (re)consider it, or at least, say the
> problem with it.
>
> You can get it with :
> git fetch git://github.com/titaude30/otp.git asn1_incomplete_packet
>
> To remind you the subject it is a patch which change decode/2
> functions generated from a file .asn1 : they return the error {error,
> incomplete} when the packet given to decode is incomplete instead of
> any error.
> It is necessary when implementing functions that decode packet
> received from a socket when it's not sure that they are complete (if
> messages are too big, they are are splitted).

I don't really agree with you here.
When receiving data on a socket you can never know for sure you get all data
together in one message (or revc) or if you have to collect and
concatenate data from many messages
until you have a complete PDU. In most protocols you have some header
with a length indicator which
tell you how many bytes you need to get a complete package i.e you
must handle this before you
call asn1rt:decode.
When you open a socket you have the packet mode which can help with
this, there is also a specific
asn.1 packet mode there.

When the decoder fails the rason can be any of:
- corrupt indata , for example incorrect length indication
- incomplete data i.e. the case you are after with missing data
- a bug in the Erlang asn1 decoding functions

You can not know which case of the above in runtime.

Another point is that even if you return {error,incomplete} , what can
the user do about that?
It is not a good idea to rely on that for regular use for testing if
more data is needed simply because it is not efficient,
you have fetch more data and then do the full decode again.
The right thing is as said above to first assure that a complete PDU
is at hand and then call decode.

> It 's the completary of another functionality : when .asn1 file are
> compiled with the undec_rest option and decode/2 functions are called
> with argument packet that contain more of one messages, then decode/2
> function return the decoded message plus the rest of the packet.
>
Yes i agree here that "incomplete2 is kind of a complement to
{Result,RestBytes} but
with the big difference that it makes much more sense to continue
computing something with the RestBytes
and that it makes sense that you get RestBytes for legitime reasons
and not becauseof an unknown type of error.


Above was a discussion regarding the need and usefulness of {error,incomplete}.

I also think that the tests you have added will cause a significant
degradation of performance for all users
even those that are not interested in {error,incomplete}.
If {error,incomplete} should be supported as return value (which I
don't think is a very good idea as said above)
I would implement it in another way with less performance penalty and
for example make it an option at
compile time for those who want the function.

There is also a big difference between BER and PER in this regard. In
BER the nested TLV structure is
decoded first and sometimes even in a driver (C-code) and the checking
can be done there (only).
In PER the length information is ofthen missing and the checking must
be done by other means.

/Regards Kenneth
> Best regards,
> Aude
>
> 2009/8/25 Aude Quintana <aude.quintana@REDACTED>:
>> Hello !
>>
>> After some other tests with the patch I made previously on asn1 library I
>> had to add some lines in the patch.
>> (it makes the decoding function to return {error,incomplete} when the buffer
>> passed in argument doesn't contain at least a full message)
>>
>> So, .diff file joined is the new patch and I hope it is complete now.
>>
>> Best Regards,
>> Aude.
>>
>
> --
> Aude Quintana
>
> ________________________________________________________________
> erlang-patches mailing list. See http://www.erlang.org/faq.html
> erlang-patches (at) erlang.org
>
>