Clean IMGB during installation #2956
Conversation
giggsoff
force-pushed
the
clean-imgb-installer
branch
from
8fac5ca to
8e719c3
Compare
| else | ||
| # clean first bytes of partition | ||
| # to avoid keeping of stale data | ||
| dd if=/dev/zero of="$IMGFILE" bs=512 count=1 conv=notrunc seek="$(( SEC_START * 512 ))" oflag=seek_bytes |
There was a problem hiding this comment.
you still use bs=512, so multiplication and "seek_bytes" flag are excessive. but this is minor
There was a problem hiding this comment.
Good point, thank you, modified.
|
Just to be sure I understand the intention correctly: are you trying to wipe out the partition table? If yes - then do we care about secondary gpt header (at the end of the disk)? |
Actually I try to invalidate squashfs partition (at least to broke header) we potentially have in IMGB to avoid its mount and introspection (in baseosmgr) from pillar's logic. |
giggsoff
force-pushed
the
clean-imgb-installer
branch
from
8e719c3 to
86514dd
Compare
|
Does it make sense to use wipefs instead of raw dd? (just it might be you did not consider it). |
The logic here assume that in general IMGFILE is a file (raw image) that we try to modify using offsets, not a device with partitions which we can use to point wipefs command onto. |
pkg/mkimage-raw-efi/make-raw
Outdated
| else | ||
| # clean first block of partition | ||
| # to avoid mounting of stale data | ||
| dd if=/dev/zero of="$IMGFILE" bs=512 count=1 conv=notrunc seek="$SEC_START" |
There was a problem hiding this comment.
Why seek using sectors here and bytes on line 210? Is there a reason for them to differ?
There was a problem hiding this comment.
The main difference in block size. We use large block size on line 210 and should use bytes offset to seek and calculate it using SEC_START * 512. Here we use bs (block size) 512, and seek SEC_START * 512 in bytes equivalent with SEC_START * bs, so we can use seek in blocks (SEC_START) and avoid unnecessary calculation.
There was a problem hiding this comment.
It might save 100us in CPU time, but it uses tens of seconds of human brain processing time each time anybody will read this code since they need to determine that it is actually writing to the same place.
I know which one I think is more expensive.
There was a problem hiding this comment.
Ha! This is exactly the opposite what I feel (I prefer memory and storage offsets in blocks, pages, not bytes) :) Because Petr already changed this line from what you, Erik, suggest here, to this current line, pointed by me. But anyway, this is very minor to me, both ways work.
There was a problem hiding this comment.
And for me was a surprise to see the special flag in dd 'seek_bytes', because when you often work with these commands you think in blocks and then you see bs=X and then seek=$BS * N, not noticing the 'seek_bytes', and then you brain stops here thinking 'hey there is an error, your block size is X, why you do this multiplication?"
There was a problem hiding this comment.
this is why I hanged for a couple of seconds on this particular line :)
There was a problem hiding this comment.
That's funny! One line, different view perspective, different path of thinking
There was a problem hiding this comment.
@rouming my only request is that the two dd lines use the same approach.
pkg/mkimage-raw-efi/make-raw
Outdated
| @@ -216,8 +222,8 @@ do_imga() { | |||
| } | |||
|
|
|||
| do_imgb() { | |||
| # for now we are not initializing IMGB - hence passing /dev/null | |||
| do_rootfs $1 IMGB /dev/null | |||
| # for now we are not initializing IMGB - hence not passing the source | |||
There was a problem hiding this comment.
Instead of "not initializing" how about "wiping"?
There was a problem hiding this comment.
make sense, modified
giggsoff
force-pushed
the
clean-imgb-installer
branch
from
86514dd to
c92fd7d
Compare
|
Will the eden install/onboarding tests exercise this? Or do we need to do some manual install with a USB stick? |
In case of usage the same drive for EVE-OS installation (for example SD card in ROL) we may keep the old data in IMGB which is not expected, so let's clean it. Signed-off-by: Petr Fedchenkov <giggsoff@gmail.com>
giggsoff
force-pushed
the
clean-imgb-installer
branch
from
c92fd7d to
39f9489
Compare
Right now we use installer only in ROL (during EdenGCP). |
In case of usage the same drive for EVE-OS installation (for example SD card in ROL) we may keep the old data in IMGB which is not expected, so let's clean it.
Signed-off-by: Petr Fedchenkov giggsoff@gmail.com