BytesIO used in BER (and CER, DER) decode #175
Conversation
Codecov Report
@@ Coverage Diff @@
## master #175 +/- ##
==========================================
- Coverage 86.08% 85.72% -0.37%
==========================================
Files 29 29
Lines 4270 4293 +23
==========================================
+ Hits 3676 3680 +4
- Misses 594 613 +19
Continue to review full report at Codecov.
|
|
Thank you for working on this matter! Once it's done, I believe it should improve things and overall design!
This is not a public feature. I think one of the use-cases for it is to optimize decoding fragments of bit/octet strings encoded in "chunked" mode. Instead of creating a full-blown ASN.1 object from each chunk, the chunks first merged together while they are still Python bytes/str, then a single ASN.1 object is created from the merged contents. |
| if isinstance(substrate, bytes): | ||
| return not bool(substrate) | ||
| else: | ||
| return not bool(asStream(substrate).peek(1)) |
There was a problem hiding this comment.
How about reducing the while function into:
return not (substrate and asStream(substrate).peek(1))
?
| # untagged Any container, recover inner header substrate | ||
| length += len(fullSubstrate) - len(substrate) | ||
| substrate = fullSubstrate | ||
| substrate.seek(fullPosition, os.SEEK_SET) |
There was a problem hiding this comment.
Oh, backtracking! Perhaps that's not always be possible to seek back, is it?
There was a problem hiding this comment.
You're right, probably we need to keep the extra bytes elsewhere (like the original "fullSubstrate" but smaller in memory).
There was a problem hiding this comment.
In the different implementation (#176), I envelop the non-seekable objects in BufferedReader.
There was a problem hiding this comment.
I am curious which streams are not seekable since we depend on that... From Python docs it seems some are not.
| @@ -1600,11 +1613,13 @@ def testErrorCondition(self): | |||
|
|
|||
| def testRawDump(self): | |||
| decode = decoder.Decoder(decoder.tagMap, decoder.typeMap) | |||
| substrate = ints2octs((31, 8, 2, 1, 1, 131, 3, 2, 1, 12)) | |||
| stream = decoder.asStream(substrate, ) | |||
There was a problem hiding this comment.
This looks good to me from design perspective!
One thing I'd like to suggest is to keep the "streaming" spirit in en/decoder implementation. That is assume no backtracking, indefinitely long sequences of chunks, presence of other consumers of the same stream that will pick it up where we drop off.
The other thing is performance - it would be great to have at least performance parity with previous implementation...
|
The other pull request (#176) is superior to this one:
I suggest to close this. |
|
I implemented the functionality so that instead of copying strings, it passes a BytesIO object whenever possible. It should solve the #174 issue.
Changes in API:
Problems:
substrateFuncseems difficult to support. Is that a public feature?The request is a draft at least until: