Skip to content

Blackrock add block validation #1740

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 13 commits into
base: master
Choose a base branch
from

Conversation

h-mayorquin
Copy link
Contributor

This is a PR I made when working together @luiztauffer to figure out how to open a malformed file. According to the blackrock spec heach data block has a flag indicating the start of the data block:

image

This PR adds a check and provides detailed debugging information including file offset, block index, expected vs actual values, and corruption indicators.

I also improved the missing file error message which now specifically list which NSX files are missing and did other minor improvements like using f-string formatting for filename
construction, clearer variable naming (replacing ambiguous spec with spec_version and
dh with packet_header), etc.

Copy link
Contributor

@zm711 zm711 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This seems fine to me.

zm711
zm711 previously approved these changes Jul 2, 2025
Copy link
Contributor

@zm711 zm711 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good by me. I'll wait to see if @samuelgarcia or @alejoe91 want to take a quick look too.

@zm711 zm711 added this to the 0.14.2 milestone Jul 2, 2025
Copy link
Contributor

@cboulay cboulay left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've never used the header_flag so I can't speak to its validity or utility here. The rest of the changes are intended to simply be for readability but a couple of errors were introduced. Please see the inline comments and revise.

@zm711 zm711 dismissed their stale review July 3, 2025 16:03

fixes needed

Copy link
Contributor

@cboulay cboulay left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Concerns addressed. Thanks.

zm711
zm711 previously approved these changes Jul 3, 2025
Copy link
Contributor

@zm711 zm711 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I will still give Sam a chance to read this next week. At least at the maintenance meeting we can briefly discuss before merge.

@zm711 zm711 dismissed their stale review July 7, 2025 14:26

We want a, b, and c variants in this PR.

@zm711 zm711 modified the milestones: 0.14.2, 0.15.0 Jul 7, 2025
@zm711
Copy link
Contributor

zm711 commented Jul 7, 2025

Just to recap. Sam at the maintenance meeting said that he would prefer to ensure all file variants are handled since this PR would be at a release boundary. So we will confirm the header flag for other versions and incorporate that (or since this is put off we can discuss merging later this week--as long as we have a plan for the other variants).

@h-mayorquin
Copy link
Contributor Author

Just to recap. Sam at the maintenance meeting said that he would prefer to ensure all file variants are handled since this PR would be at a release boundary. So we will confirm the header flag for other versions and incorporate that (or since this is put off we can discuss merging later this week--as long as we have a plan for the other variants).

This request does not make sense. I just check and the other variant is empty:

https://github.com/h-mayorquin/python-neo/blob/2240a29c4366fb3ed5dacadb54837004651ee35e/neo/rawio/blackrockrawio.py#L967-L973

No validation can be performed there.

@zm711
Copy link
Contributor

zm711 commented Jul 21, 2025

Okay so that means everything is actually covered? I think the issue was that no one had worked on this recently enough to know if we needed to cover the other variants.

@h-mayorquin
Copy link
Contributor Author

Okay so that means everything is actually covered? I think the issue was that no one had worked on this recently enough to know if we needed to cover the other variants.

Yes, my review indicates that everything that needs to be covered is covered.

Copy link
Contributor

@zm711 zm711 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@samuelgarcia I'll leave the merge to you. But if the other variants don't matter this should be ready finally.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants