WIP: Add Position() to Firmware interface #317
Conversation
karlodwyer
force-pushed
the
position_visitor
branch
from
41c3b7b
to
a4cc3eb
Compare
|
Hello, first I've not looked into the code yet, but regarding your point about |
The problem is: This circular dependency could be broken if we will move types (like Another obvious approach is just to duplicate the visitor between |
Codecov Report
@@ Coverage Diff @@
## master #317 +/- ##
==========================================
+ Coverage 39.21% 41.47% +2.26%
==========================================
Files 47 48 +1
Lines 3501 3597 +96
==========================================
+ Hits 1373 1492 +119
+ Misses 1912 1873 -39
- Partials 216 232 +16
Continue to review full report at Codecov.
|
There was a problem hiding this comment.
This is an interesting approach, however I'd much rather have the visitor in pkg/visitors.
The way I would avoid the cyclic dependency is to call updatePositions not from uefi.Parse, but from pkg/utk/utk.go, in the Run function right before other visitors are executed. This also ensures that if we are calling utk on an extracted directory, the positions are also updated. Right now I think it'll only work if you call utk on a bios image.
| @@ -156,6 +167,11 @@ func (f *FlashImage) ApplyChildren(v Visitor) error { | |||
| return nil | |||
| } | |||
|
|
|||
| // Position returns the absolution position of the node in the firmware image. | |||
| return f.Apply(v) | ||
| } | ||
|
|
||
| // Visit applies the Table visitor to any Firmware type. |
| @@ -258,6 +261,11 @@ func (s *Section) ApplyChildren(v Visitor) error { | |||
| return nil | |||
| } | |||
|
|
|||
| // Position returns the absolution position of the node in the firmware image. | |||
|
I am closing this, for lack of interest. |
|
I'm interested, see #321 |
Signed-off-by: karlodwyer <[email protected]>
Signed-off-by: karlodwyer <[email protected]>
Signed-off-by: karlodwyer <[email protected]>
Signed-off-by: karlodwyer <[email protected]>
Signed-off-by: karlodwyer <[email protected]>
orangecms
force-pushed
the
position_visitor
branch
from
f55ff01
to
1c0079f
Compare
|
Codecov ReportAttention: Patch coverage is
❗ Your organization needs to install the Codecov GitHub app to enable full functionality. Additional details and impacted files@@ Coverage Diff @@
## main #317 +/- ##
==========================================
+ Coverage 34.21% 35.08% +0.86%
==========================================
Files 209 210 +1
Lines 14086 14182 +96
==========================================
+ Hits 4820 4976 +156
+ Misses 8377 8302 -75
- Partials 889 904 +15
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
|
I don't think this one is going to get finished. |
Original Issue here
@xaionaro asked on the opensource firmware slack for suggestions around implementing this idea. The approach suggested was to publish a PR and work it out form there, so he and I talked a bit about this.
Some things to note:
PositionUpdaterfrom uefi, but we are open for discussion on how to fix it.Assembleprobably need more testing to give us more assurance that we aren't regressing anything.