Kevin Wolf <[email protected]> writes: > Am 07.03.2019 um 11:05 hat Markus Armbruster geschrieben: >> Markus Armbruster <[email protected]> writes: >> >> > From: Alex Bennée <[email protected]> >> > >> > We reject undersized images. As of the previous commit, even with a >> > decent error message. Still, this is a potentially confusing >> > stumbling block when you move from using -bios to using -drive >> > if=pflash,file=blob,format=raw,readonly for loading your firmware >> > code. To mitigate that we automatically pad in the read-only case and >> > warn the user when we have performed magic to enable things to Just >> > Work (tm). >> > >> > Signed-off-by: Alex Bennée <[email protected]> >> > Reviewed-by: Laszlo Ersek <[email protected]> >> > Signed-off-by: Markus Armbruster <[email protected]> >> >> I think this convenience feature is a bad idea, and this patch should >> not be applied. Here are my reasons. >> >> 1. As I explained in my disccussion of v5[*], there is no single true >> way to pad. This patch pads with 0xFF. When working with physical >> devices, you'd sometimes pad that way, but at other times, you'd pad >> differently. >> >> 2. Except this patch doesn't *actually* pad with 0xFF. The block layer >> silently pads with zeros up to the next multiple of 512. Evidence: >> >> $ yes | dd of=4090b.img bs=1 count=4090 >> 4090+0 records in >> 4090+0 records out >> 4090 bytes (4.1 kB, 4.0 KiB) copied, 0.0186459 s, 219 kB/s >> $ qemu-io -f raw -c 'read -v 4000 96' 4090b.img >> 00000fa0: 79 0a 79 0a 79 0a 79 0a 79 0a 79 0a 79 0a 79 0a >> y.y.y.y.y.y.y.y. >> 00000fb0: 79 0a 79 0a 79 0a 79 0a 79 0a 79 0a 79 0a 79 0a >> y.y.y.y.y.y.y.y. >> 00000fc0: 79 0a 79 0a 79 0a 79 0a 79 0a 79 0a 79 0a 79 0a >> y.y.y.y.y.y.y.y. >> 00000fd0: 79 0a 79 0a 79 0a 79 0a 79 0a 79 0a 79 0a 79 0a >> y.y.y.y.y.y.y.y. >> 00000fe0: 79 0a 79 0a 79 0a 79 0a 79 0a 79 0a 79 0a 79 0a >> y.y.y.y.y.y.y.y. >> 00000ff0: 79 0a 79 0a 79 0a 79 0a 79 0a 00 00 00 00 00 00 >> y.y.y.y.y....... >> read 96/96 bytes at offset 4000 >> 96 bytes, 1 ops; 0.0001 sec (694.444 KiB/sec and 7407.4074 ops/sec)
By the way, when you write to the padding, the block layer grows the file to the next multiple of 512. >> >> This patch then pads some more with 0xFF to the flash memory size. >> >> Because of that the way the magic padding works makes no sense, to be >> frank. Going back to v3's zero padding would at least be explainable >> without blushing. >> >> I consider the block layer's padding a misfeature here, but right now >> we got to play the hand we've been dealt. > > That will be solved as soon as the block layer is consistently converted > to byte granularity. We've already converted a lot, but bdrv_getlength() > is still sector (512 bytes) granularity. > > You could argue that file-posix should just reject files that are not > sector aligned, but that's probably not right either because image > formats don't necessarily have that alignment for their files. Yes, it could well turn out to be overly restrictive. > Maybe disk device should reject being attached to nodes that aren't a > multiple of their physical and logical sector size. A 512-byte aligned > image is probably suitable for most disks, but might not be for a virtual > native 4k disk. Makes sense to me. Could perhaps be done in or near blkconf_blocksizes(). >> 3. Convenience magic has bitten us in the posterior so many times. Just >> say no unless there's a really compelling use case. Where's the use >> case for this one? We've rejected undersized images for ages, and >> nobody complained. Why add convenience magic now? > > I agree. Okay. Let's take just the error reporting improvment then (PATCH 1 and its fixup). Since we all seem to agree on upgrading the warning to an error, do that. Don't take the convenience feature now (PATCH 2 and its fixup). We can always revisit it later. This should give us a good chance at getting it done in time for the soft freeze. I'll prepare v7. Thanks!
